[ANNOUNCE] Kernel Janitor's TODO list

[ANNOUNCE] Kernel Janitor's TODO list

[Arnaldo Carvalho de Melo]

Hi,

	The kernel Janitor's TODO list is updated at
http://bazar.conectiva.com.br/~acme/TODO, lots of things to do to get rid
of old cruft, make sure that resources are properly used, etc, please take
a look and help! Please send additions and corrections to me and I'll try
to keep it updated.

- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[David Woodhouse]

acme@conectiva.com.br said:
>  Please send additions and corrections to me and I'll try to keep it
> updated.

Anything which uses sleep_on() has a 90% chance of being broken. Fix
them all, because we want to remove sleep_on() and friends in 2.5.

--
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Arnaldo Carvalho de Melo]

Em Sun, Jan 28, 2001 at 03:20:18PM +0000, David Woodhouse escreveu:
> 
> acme@conectiva.com.br said:
> >  Please send additions and corrections to me and I'll try to keep it
> > updated.
> 
> Anything which uses sleep_on() has a 90% chance of being broken. Fix
> them all, because we want to remove sleep_on() and friends in 2.5.

TODO updated, availabe at http://bazar.conectiva.com.br/~acme/TODO

- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Michael H. Warfield]

On Sun, Jan 28, 2001 at 03:20:18PM +0000, David Woodhouse wrote:

> acme@conectiva.com.br said:
> >  Please send additions and corrections to me and I'll try to keep it
> > updated.

> Anything which uses sleep_on() has a 90% chance of being broken. Fix
> them all, because we want to remove sleep_on() and friends in 2.5.

	And friends meaning "interruptible_sleep_on"?  Great...  I've
got a driver with about a half a dozen of them.  Point me at the Doco
to fix please?

> --
> dwmw2

	Mike
-- 
 Michael H. Warfield    |  (770) 985-6132   |  mhw@WittsEnd.com
  (The Mad Wizard)      |  (678) 463-0932   |  http://www.wittsend.com/mhw/
  NIC whois:  MHW9      |  An optimist believes we live in the best of all
 PGP Key: 0xDF1DD471    |  possible worlds.  A pessimist is sure of it!

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Andrew Morton]

Arnaldo Carvalho de Melo wrote:
> 
> Please send additions and corrections to me and I'll try
> to keep it updated.

Here - have about 300 bugs:

	http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html

A lot of the timer deletion races are hard to fix because of
the deadlock problem.

-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Arnaldo Carvalho de Melo]

Em Mon, Jan 29, 2001 at 03:13:19AM +1100, Andrew Morton escreveu:
> Arnaldo Carvalho de Melo wrote:
> > 
> > Please send additions and corrections to me and I'll try
> > to keep it updated.
> 
> Here - have about 300 bugs:
> 
> 	http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> 
> A lot of the timer deletion races are hard to fix because of
> the deadlock problem.

added, please keep sending, and as somebody pointed out: it is good to have
explanations about what have to be done, so interested people can
contribute, specially people that are looking to start helping and don't
know where to start, now we can say "hey, pick one of these 300 bugs and
start doing something!" ;)

- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Arnaldo Carvalho de Melo]

Em Sun, Jan 28, 2001 at 12:28:50PM -0200, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jan 29, 2001 at 03:13:19AM +1100, Andrew Morton escreveu:
> > Arnaldo Carvalho de Melo wrote:
> > > 
> > > Please send additions and corrections to me and I'll try
> > > to keep it updated.
> > 
> > Here - have about 300 bugs:
> > 
> > 	http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> > 
> > A lot of the timer deletion races are hard to fix because of
> > the deadlock problem.
> 
> added, please keep sending, and as somebody pointed out: it is good to have
> explanations about what have to be done, so interested people can
> contribute, specially people that are looking to start helping and don't
> know where to start, now we can say "hey, pick one of these 300 bugs and
> start doing something!" ;)

I forgot to add: "Like Andrew did in the above URL" 8)

- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Rusty Russell]

In message <3A74451F.DA29FD17@uow.edu.au> you write:
> 	http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> 
> A lot of the timer deletion races are hard to fix because of
> the deadlock problem.

Hmmm...

	For 2.5, changing the timer interface to disallow mod_timer or
add_timer (equivalent) on self, and making the timerfn return num
jiffies to next run (0 = don't rerun) would solve this, right?
I don't see a maintainable way of solving this otherwise,

	Of course, kfree'ing the timer struct and returning non-zero
would be a *bug*...

Rusty.
--
Premature optmztion is rt of all evl. --DK
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Daniel Phillips]

Rusty Russell wrote:
> 
> In message <3A74451F.DA29FD17@uow.edu.au> you write:
> >       http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> >
> > A lot of the timer deletion races are hard to fix because of
> > the deadlock problem.
> 
> Hmmm...
> 
>         For 2.5, changing the timer interface to disallow mod_timer or
> add_timer (equivalent) on self, and making the timerfn return num
> jiffies to next run (0 = don't rerun) would solve this, right?
> I don't see a maintainable way of solving this otherwise,

It seems silly not to provide direct support for such a simple, useful
mechanism as a periodic timer.  This can be accomplished easily by
adding a field 'periodic' to struct timer_list.  If 'periodic' is
non-zero then run_timer_list uses it to set the 'expires' field and
re-inserts the timer.

For what it's worth, this is backward compatible with the existing
strategy.  The timer_list->function is still in complete control of
things if it wants to be, but forbidding it from re-adding itself sounds
like an awfully good idea.

--
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Daniel Phillips]

Daniel Phillips wrote:
> 
> Rusty Russell wrote:
> >
> > In message <3A74451F.DA29FD17@uow.edu.au> you write:
> > >       http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> > >
> > > A lot of the timer deletion races are hard to fix because of
> > > the deadlock problem.
> >
> > Hmmm...
> >
> >         For 2.5, changing the timer interface to disallow mod_timer or
> > add_timer (equivalent) on self, and making the timerfn return num
> > jiffies to next run (0 = don't rerun) would solve this, right?
> > I don't see a maintainable way of solving this otherwise,
> 
> It seems silly not to provide direct support for such a simple, useful
> mechanism as a periodic timer.  This can be accomplished easily by
> adding a field 'periodic' to struct timer_list.  If 'periodic' is
> non-zero then run_timer_list uses it to set the 'expires' field and
> re-inserts the timer.
> 
> For what it's worth, this is backward compatible with the existing
> strategy.  The timer_list->function is still in complete control of
> things if it wants to be, but forbidding it from re-adding itself sounds
> like an awfully good idea.

Whoops, this post from Alexy makes it quite clear why I can't do that:

	http://www.wcug.wwu.edu/lists/netdev/200005/msg00050.html
	Timers are self-destructable as rule. See? Normal usage
	for timer is to have it allocated inside an object and
	timer event detroys the object together with timer.

I did a quick scan through timer usage, and sure enough, I found
self-destructive behaviour as Alexy describes, for example, in
ax25_std_heartbeat_expiry.  Your suggestion is good and simple, but
requires every timer_list->function to be changed, a couple of hundred
places to check.

It would be nice to have a nice easy transition instead of a
jump-off-the-cliff and change all usage approach.  Hmm, a hack is
coming... I'll add a new, improved function field beside the old one,
call it ->timer_event, and it can force rescheduling as you suggested. 
If ->timer_event is non-null it gets called instead of ->function, and
the timer may be requeued.  For good measure, I'll leave my ->period
field in there because it just makes sense.  Then I can write a generic
->timer_event that just returns the ->period.

/me: hack, hack, hack

--
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

[RFC] New Improved Stronger Whiter Timers (was: Kernel Janitor)

[Daniel Phillips]

On Tue, 30 Jan 2001, Rusty Russell wrote:
> In message <3A74451F.DA29FD17@uow.edu.au> you write:
> > 	http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> > 
> > A lot of the timer deletion races are hard to fix because of
> > the deadlock problem.
> 
> Hmmm...
> 
> 	For 2.5, changing the timer interface to disallow mod_timer or
> add_timer (equivalent) on self, and making the timerfn return num
> jiffies to next run (0 = don't rerun) would solve this, right?
> I don't see a maintainable way of solving this otherwise,
> 
> 	Of course, kfree'ing the timer struct and returning non-zero
> would be a *bug*...

Here's a patch that implements your idea together with a hack by me to
make it work with existing code: ->event is non-null for the new
behaviour, null for the broken behavior.  The new behaviour is that
timers don't re-add themselves anymore, run_timer_list does, so when
you kill a timer it stays dead.

--- 2.4.1.clean/include/linux/timer.h	Tue Jan 30 08:24:55 2001
+++ 2.4.1/include/linux/timer.h	Tue Jan 30 20:59:01 2001
@@ -22,6 +22,7 @@
 	unsigned long expires;
 	unsigned long data;
 	void (*function)(unsigned long);
+	unsigned long (*event)(unsigned long data);
 };
 
 extern void add_timer(struct timer_list * timer);
@@ -49,6 +50,9 @@
 static inline void init_timer(struct timer_list * timer)
 {
 	timer->list.next = timer->list.prev = NULL;
+	timer->function = NULL;
+	timer->event = NULL;
+	timer->data = 0;
 }
 
 static inline int timer_pending (const struct timer_list * timer)
--- 2.4.1.clean/kernel/timer.c	Sun Dec 10 18:53:19 2000
+++ 2.4.1/kernel/timer.c	Tue Jan 30 20:59:01 2001
@@ -301,18 +301,25 @@
 		curr = head->next;
 		if (curr != head) {
 			struct timer_list *timer;
-			void (*fn)(unsigned long);
-			unsigned long data;
+			unsigned long data, requeue;
 
 			timer = list_entry(curr, struct timer_list, list);
- 			fn = timer->function;
  			data= timer->data;
 
 			detach_timer(timer);
 			timer->list.next = timer->list.prev = NULL;
 			timer_enter(timer);
 			spin_unlock_irq(&timerlist_lock);
-			fn(data);
+			if (timer->event)
+			{
+				if ((requeue = timer->event(data)))
+				{
+					timer->expires += requeue;
+					internal_add_timer(timer);
+				}
+			}
+			else
+				timer->function(data); /* bad old way */
 			spin_lock_irq(&timerlist_lock);
 			timer_exit();
 			goto repeat;

-------------------
Here's some test code.  It kprintS a 10-step countdown during init:

--- 2.4.1.clean/fs/buffer.c	Mon Jan 15 21:42:32 2001
+++ 2.4.1/fs/buffer.c	Tue Jan 30 20:59:01 2001
@@ -2792,9 +2792,22 @@
 	}
 }
 
+struct timer_list foo_timer;
+
+unsigned long foo_event (unsigned long data)
+{
+	printk ("Foo %i\n", foo_timer.data);
+	return --foo_timer.data? HZ: 0;
+}
+
 static int __init bdflush_init(void)
 {
 	DECLARE_MUTEX_LOCKED(sem);
+	init_timer (&foo_timer);
+	foo_timer.event = foo_event;
+	foo_timer.data = 10;
+	add_timer (&foo_timer);
+
 	kernel_thread(bdflush, &sem, CLONE_FS | CLONE_FILES | CLONE_SIGNAL);
 	down(&sem);
 	kernel_thread(kupdate, &sem, CLONE_FS | CLONE_FILES | CLONE_SIGNAL);

-- 
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [RFC] New Improved Stronger Whiter Timers (was: Kernel Janitor)

[Daniel Phillips]

On Tue, 30 Jan 2001, Manfred Spraul wrote:
> This one is an UP and SMP race:
> 
>   spin_unlock_irq(&timerlist_lock); 
> + if (timer->event) 
> + { 
> + if ((requeue = timer->event(data))) 
> + { 
> + timer->expires += requeue; 
> + internal_add_timer(timer); 
> + } 
> + } 
> + else 
> + timer->function(data); /* bad old way */ 
>   spin_lock_irq(&timerlist_lock); 
> 
> internal_add_timer assumes that the timerlist_lock is acquired.

Yes, oops, here is the fix.

--- ../2.4.1.clean/include/linux/timer.h	Tue Jan 30 08:24:55 2001
+++ ./include/linux/timer.h	Tue Jan 30 21:52:58 2001
@@ -22,6 +22,7 @@
 	unsigned long expires;
 	unsigned long data;
 	void (*function)(unsigned long);
+	unsigned long (*event)(unsigned long data);
 };
 
 extern void add_timer(struct timer_list * timer);
@@ -49,6 +50,9 @@
 static inline void init_timer(struct timer_list * timer)
 {
 	timer->list.next = timer->list.prev = NULL;
+	timer->function = NULL;
+	timer->event = NULL;
+	timer->data = 0;
 }
 
 static inline int timer_pending (const struct timer_list * timer)
--- ../2.4.1.clean/kernel/timer.c	Sun Dec 10 18:53:19 2000
+++ ./kernel/timer.c	Tue Jan 30 22:09:09 2001
@@ -301,19 +301,30 @@
 		curr = head->next;
 		if (curr != head) {
 			struct timer_list *timer;
-			void (*fn)(unsigned long);
-			unsigned long data;
+			unsigned long data, requeue;
 
 			timer = list_entry(curr, struct timer_list, list);
- 			fn = timer->function;
  			data= timer->data;
 
 			detach_timer(timer);
 			timer->list.next = timer->list.prev = NULL;
 			timer_enter(timer);
 			spin_unlock_irq(&timerlist_lock);
-			fn(data);
-			spin_lock_irq(&timerlist_lock);
+			if (timer->event)
+			{
+				requeue = timer->event(data);
+				spin_lock_irq(&timerlist_lock);
+				if (requeue)
+				{
+					timer->expires += requeue;
+					internal_add_timer(timer);
+				}
+			}
+			else
+			{
+				timer->function(data); /* bad old way */
+				spin_lock_irq(&timerlist_lock);
+			}
 			timer_exit();
 			goto repeat;
 		}

-- 
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Manfred Spraul]

> 
> Anything which uses sleep_on() has a 90% chance of being broken. Fix 
> them all, because we want to remove sleep_on() and friends in 2.5. 
>

Then you can add 'calling schedule() with disabled local interrupts()'
to your list.

--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Arnaldo Carvalho de Melo]

Em Sun, Jan 28, 2001 at 05:14:37PM +0100, Manfred Spraul escreveu:
> > 
> > Anything which uses sleep_on() has a 90% chance of being broken. Fix 
> > them all, because we want to remove sleep_on() and friends in 2.5. 
> >
> 
> Then you can add 'calling schedule() with disabled local interrupts()'
> to your list.

any example of code doing this now? That way we can at least point it to
interested people and say "look at driver foobar in kernel x.y.z and see
how its wrong"

- Arnaldo
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Manfred Spraul]

Arnaldo Carvalho de Melo wrote:
> 
> Em Sun, Jan 28, 2001 at 05:14:37PM +0100, Manfred Spraul escreveu:
> > >
> > > Anything which uses sleep_on() has a 90% chance of being broken. Fix
> > > them all, because we want to remove sleep_on() and friends in 2.5.
> > >
> >
> > Then you can add 'calling schedule() with disabled local interrupts()'
> > to your list.
> 
> any example of code doing this now? That way we can at least point it to
> interested people and say "look at driver foobar in kernel x.y.z and see
> how its wrong"
>

It isn't wrong to call schedule() with disabled interrupts - it's a
feature ;-)
Those 10% sleep_on() users that aren't broken use it:

 for(;;) {
	cli();
	if(condition)
		break;
	sleep_on(&my_wait_queue);
	sti();
 }

E.g. TIOCMIWAIT in drivers/char/serial.c - a nearly correct sleep_on()
user.

But I doubt that 10% of the sleep_on() users are non-broken...

If you remove sleep_on(), then you can disallow calling schedule() with
disabled local interrupts.

--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[David Woodhouse]

On Sun, 28 Jan 2001, Manfred Spraul wrote:

> It isn't wrong to call schedule() with disabled interrupts - it's a
> feature ;-)
> Those 10% sleep_on() users that aren't broken use it:
> 
>  for(;;) {
> 	cli();
> 	if(condition)
> 		break;
> 	sleep_on(&my_wait_queue);
> 	sti();
>  }

That's valid iff the wake_up() can only happen from an ISR.

> E.g. TIOCMIWAIT in drivers/char/serial.c - a nearly correct sleep_on()
> user.

TIOCMIWAIT does restore_flags() before interruptible_sleep_on(). It's 
broken too.

Anyway, if you're feeling pedantic, consider what happens if shutdown() is
called from rs_close() just before sleep_on() is called. Regardless of 
whether interrupts are disabled.

> But I doubt that 10% of the sleep_on() users are non-broken...

There are cases where you don't care if you miss a wakeup because you have
a timeout. So it's only suboptimal rather than broken. I did produce a 
patch to BUG() in sleep_on if the BKL isn't held, at one point. It was 
quite interesting.

> If you remove sleep_on(), then you can disallow calling schedule() with
> disabled local interrupts.

The remaining valid users of sleep_on are mainly filesystems - much fs
code gets called with the BKL held. I expect that to change during 2.5, at 
which point sleep_on can be terminated with extreme prejudice. 

-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Manfred Spraul]

David Woodhouse wrote:
> 
> TIOCMIWAIT does restore_flags() before interruptible_sleep_on(). It's
> broken too.
>
Yes, and I found a second bug: it doesn't sti() immediately after
interruptible_sleep_on(), thus cli() doesn't reacquire the global irq
lock --> the atomic copy won't be atomic on SMP.


And one more point for the Janitor's list:
Get rid of superflous irqsave()/irqrestore()'s - in 90% of the cases
either spin_lock_irq() or spin_lock() is sufficient. That's both faster
and better readable.

spin_lock_irq(): you know that the function is called with enabled
interrupts.
spin_lock(): can be used in hardware interrupt handlers when only one
hardware interrupt uses that spinlocks (most hardware drivers), or when
all hardware interrupt handler set the SA_INTERRUPT flag (e.g. rtc and
timer interrupt)

There is one more rule when you can use spin_lock_irq():
if you know that the function might sleep. E.g. compare make_request
from 2.2.18 and __make_request() from 2.4.
Since __get_request_wait() can sleep, the callers of make_request()
cannot rely on disabled interrupts, thus spin_lock_irq instead of
spin_lock_irqsave().

--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Roman Zippel]

Hi,

On Sun, 28 Jan 2001, Manfred Spraul wrote:

> And one more point for the Janitor's list:
> Get rid of superflous irqsave()/irqrestore()'s - in 90% of the cases
> either spin_lock_irq() or spin_lock() is sufficient. That's both faster
> and better readable.
> 
> spin_lock_irq(): you know that the function is called with enabled
> interrupts.
> spin_lock(): can be used in hardware interrupt handlers when only one
> hardware interrupt uses that spinlocks (most hardware drivers), or when
> all hardware interrupt handler set the SA_INTERRUPT flag (e.g. rtc and
> timer interrupt)

This is not a bug and only helps to make drivers nonportable. Please,
don't do this.

bye, Roman

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Timur Tabi]

This is driving me crazy!  There is absolutely no documentation anywhere that
tells you when to use or not use sleep_on or spin_lock_whatever or any of these
calls.  How is anyone supposed to know how to use these functions?!  The post I
quoted below just proves that a lot of people think they know but apparently
don't!  In fact, I predict that an argument between the two posters and a few
others will soon ensue over who is right.

What makes it more frustrating is that some people on this list talk as if
things things are common knowledge.  I've been following this mailing list for
months, and until today I had no idea sleep_on was bad.  All the documentation
I've read to date freely uses sleep_on in the sample code.  In fact, I still
don't even know WHY it's bad.  Not only that, but what am I supposed to use
instead? 

This is what I find most frustrating about Linux.  If I were a Windows driver
programmer, I could walk into any bookstore and pick up any of a dozen books
that explains everything, leaving no room for doubt.


** Reply to message from Roman Zippel <zippel@fh-brandenburg.de> on Sun, 28 Jan
2001 19:51:57 +0100 (MET)

> Hi,
> 
> On Sun, 28 Jan 2001, Manfred Spraul wrote:
> 
> > And one more point for the Janitor's list:
> > Get rid of superflous irqsave()/irqrestore()'s - in 90% of the cases
> > either spin_lock_irq() or spin_lock() is sufficient. That's both faster
> > and better readable.
> > 
> > spin_lock_irq(): you know that the function is called with enabled
> > interrupts.
> > spin_lock(): can be used in hardware interrupt handlers when only one
> > hardware interrupt uses that spinlocks (most hardware drivers), or when
> > all hardware interrupt handler set the SA_INTERRUPT flag (e.g. rtc and
> > timer interrupt)
> 
> This is not a bug and only helps to make drivers nonportable. Please,
> don't do this.
> 
> bye, Roman
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> Please read the FAQ at http://www.tux.org/lkml/


-- 
Timur Tabi - ttabi@interactivesi.com
Interactive Silicon - http://www.interactivesi.com

When replying to a mailing-list message, please direct the reply to the mailing list only.  Don't send another copy to me.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[John Levon]

On Mon, 29 Jan 2001, Timur Tabi wrote:

> This is driving me crazy!  There is absolutely no documentation anywhere that
> tells you when to use or not use sleep_on or spin_lock_whatever or any of these
> calls.  

huh ?

http://www.kernelnewbies.org/books.php3

/usr/src/linux-2.4/Documentation/DocBook

/usr/src/linux/*

try the last one on Windows. Please get your facts at least remotely near
the truth before you rant on linux-kernel again

john

-- 
"To be fair i do look quite like a monkey ..."
	- Peter Reid

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[David D.W. Downey]

On Mon, 29 Jan 2001, John Levon wrote:

> huh ?
> 
> http://www.kernelnewbies.org/books.php3
> 
> /usr/src/linux-2.4/Documentation/DocBook
> 
> /usr/src/linux/*
> 
> try the last one on Windows. Please get your facts at least remotely near
> the truth before you rant on linux-kernel again
> 
> john
> 
> 

Umm, john.. He IS right. I've been following the linux kernel list for
QUITE some time, though actively asking questions on it for the last 2
months. I've read through the docs in /usr/src/linux/Documentation/* and i
hear one more person tell me to "read the source" I'll go nuts. 

SOURCE CODE ONLY MAKES SENSE TO THOSE THAT EITHER WROTE IT OR WORK WITH IT
EVERYDAY!

And don't tell me "Well, then you shouldn't be touching the kernel if
you're not a developer" because that's crap too.


Simply put, with all bitterness and finger pointing aside, WHERE do we
find information on various kernel functions, their general usage (as in
the WHY, not only the HOW) and reasonings on why not to use some
vs. others.

Remember, most of you guys have been coding for years, or working on the
kernel for years. Some of us don't have that level of expertise, are
trying to get it, and feel like we're being told that information is a
private domain we aren't allowed in to.


Me personally, I'd be happy with a list of all the finctions in the linux
kernel, a brief description of their usage and a singl elink on where to
find more info about that particular function.


Just my 2 cents worth.

David D.W. Downey


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[davej]

On Mon, 29 Jan 2001, David D.W. Downey wrote:

> Simply put, with all bitterness and finger pointing aside, WHERE do we
> find information on various kernel functions, their general usage (as in
> the WHY, not only the HOW) and reasonings on why not to use some
> vs. others.

/usr/src/linux/Documentation

> Me personally, I'd be happy with a list of all the finctions in the linux
> kernel, a brief description of their usage and a singl elink on where to
> find more info about that particular function.

make pdfdocs
acroread Documentation/DocBook/kernel-api.pdf

(Check out the other .pdf's in that dir too)

You may also make sgmldocs/psdocs/htmldocs

regards,

Davej.

-- 
| Dave Jones.        http://www.suse.de/~davej
| SuSE Labs

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Timur Tabi]

** Reply to message from davej@suse.de on Mon, 29 Jan 2001 20:44:55 +0000 (GMT)


> make pdfdocs

[ttabi@one DocBook]$ make pdfdocs
Makefile:140: /Rules.make: No such file or directory

There's no Rules.make anywhere on my hard drive.


-- 
Timur Tabi - ttabi@interactivesi.com
Interactive Silicon - http://www.interactivesi.com

When replying to a mailing-list message, please direct the reply to the mailing list only.  Don't send another copy to me.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Rasmus Andersen]

On Mon, Jan 29, 2001 at 02:51:18PM -0600, Timur Tabi wrote:
> ** Reply to message from davej@suse.de on Mon, 29 Jan 2001 20:44:55 +0000 (GMT)
> 
> 
> > make pdfdocs
> 
> [ttabi@one DocBook]$ make pdfdocs
> Makefile:140: /Rules.make: No such file or directory

You have to be in the top level directory, not the DocBook one.

> 
> There's no Rules.make anywhere on my hard drive.

Made by 'make config'?

-- 
Regards,
        Rasmus(rasmus@jaquet.dk)

"God prevent we should ever be twenty years without a revolution." 
  -- Thomas Jefferson
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Peter Samuelson]

[Timur Tabi]
> [ttabi@one DocBook]$ make pdfdocs
> Makefile:140: /Rules.make: No such file or directory
> 
> There's no Rules.make anywhere on my hard drive.

There had better be one in '../..'.  Do the 'make pdfdocs' from the top
level of the kernel source tree.

Peter
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Ingo Oeser]

On Mon, Jan 29, 2001 at 10:27:50AM -0800, David D.W. Downey wrote:
> And don't tell me "Well, then you shouldn't be touching the kernel if
> you're not a developer" because that's crap too.

No this is a good advise. "Never touch anything you don't
understand." If you like to develop for Linux, you should
understand the API you use and if you don't understand it, either
learn more about it (e.g. reading the source ;-)) or stop using
it.

Regards

Ingo Oeser, reading it since 1996
-- 
10.+11.03.2001 - 3. Chemnitzer LinuxTag <http://www.tu-chemnitz.de/linux/tag>
         <<<<<<<<<<<<       come and join the fun       >>>>>>>>>>>>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[David Woodhouse]

pgpkeys@hislinuxbox.com said:
>  Remember, most of you guys have been coding for years, or working on
> the kernel for years. Some of us don't have that level of expertise,
> are trying to get it, and feel like we're being told that information
> is a private domain we aren't allowed in to.

Note that this is _precisely_ the reason I'm advocating the removal of 
sleep_on(). When I was young and stupid (ok, "younger and stupider") I used 
sleep_on() in my code. I pondered briefly the fact that I really couldn't 
convince myself that it was safe, but because it was used in so many other 
places, I decided I had to be missing something, and used it anyway.

I was wrong. I was copying broken code. And now I want to remove all those 
bad examples - for the benefit of those who are looking at them now and are 
tempted to copy them.

--
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Timur Tabi]

** Reply to message from David Woodhouse <dwmw2@infradead.org> on Tue, 30 Jan
2001 11:11:27 +0000


> Note that this is _precisely_ the reason I'm advocating the removal of 
> sleep_on(). When I was young and stupid (ok, "younger and stupider") I used 
> sleep_on() in my code. I pondered briefly the fact that I really couldn't 
> convince myself that it was safe, but because it was used in so many other 
> places, I decided I had to be missing something, and used it anyway.
> 
> I was wrong. I was copying broken code. And now I want to remove all those 
> bad examples - for the benefit of those who are looking at them now and are 
> tempted to copy them.

What is wrong with sleep_on()?


-- 
Timur Tabi - ttabi@interactivesi.com
Interactive Silicon - http://www.interactivesi.com

When replying to a mailing-list message, please direct the reply to the mailing list only.  Don't send another copy to me.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Daniel Phillips]

Timur Tabi wrote:
> 
> ** Reply to message from David Woodhouse
> 
> > Note that this is _precisely_ the reason I'm advocating the removal of
> > sleep_on(). When I was young and stupid (ok, "younger and stupider") I used
> > sleep_on() in my code. I pondered briefly the fact that I really couldn't
> > convince myself that it was safe, but because it was used in so many other
> > places, I decided I had to be missing something, and used it anyway.
> >
> > I was wrong. I was copying broken code. And now I want to remove all those
> > bad examples - for the benefit of those who are looking at them now and are
> > tempted to copy them.
> 
> What is wrong with sleep_on()?

If you have a task that looks like:

    loop:
        <do something important>
        sleep_on(q)

And you do wakeup(q) hoping to get something important done, then if the
task isn't sleeping at the time of the wakeup it will ignore the wakeup
and go to sleep, which imay not be what you wanted.

--
Daniel
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Timur Tabi]

** Reply to message from Daniel Phillips <phillips@innominate.de> on Wed, 31
Jan 2001 01:06:08 +0100


> > What is wrong with sleep_on()?
> 
> If you have a task that looks like:
> 
>     loop:
>         <do something important>
>         sleep_on(q)
> 
> And you do wakeup(q) hoping to get something important done, then if the
> task isn't sleeping at the time of the wakeup it will ignore the wakeup
> and go to sleep, which imay not be what you wanted.

Ok, so how should this code have been written?


-- 
Timur Tabi - ttabi@interactivesi.com
Interactive Silicon - http://www.interactivesi.com

When replying to a mailing-list message, please direct the reply to the mailing list only.  Don't send another copy to me.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[David Woodhouse]

On Tue, 30 Jan 2001, Timur Tabi wrote:

> > If you have a task that looks like:
> > 
> >     loop:
> >         <do something important>
> >         sleep_on(q)
> > 
> > And you do wakeup(q) hoping to get something important done, then if the
> > task isn't sleeping at the time of the wakeup it will ignore the wakeup
> > and go to sleep, which imay not be what you wanted.
> 
> Ok, so how should this code have been written?

DECLARE_WAIT_QUEUE(wait, current);

while(1) {
	do_something_important()

	set_current_state(TASK_INTERRUPTIBLE);
	add_wait_queue(&q, &wait);

	/* Now if something arrives, we'll be 'woken' immediately -
	   - that is; our state will be set to TASK_RUNNING */

	if (!stuff_to_do()) {
		/* If the 'stuff' arrives here, we get woken anyway,
			so the schedule() returns immediately. You 
			can use schedule_timeout() here if you need
			a timeout, obviously */
		schedule();
	}

	set_current_state(TASK_RUNNING);
	remove_wait_queue(&q, &wait);

	if (signal_pending(current)) {
		/* You've been signalled. Deal with it. If you don't 
			want signals to wake you, use TASK_UNINTERRUPTIBLE
			above instead of TASK_INTERRUPTIBLE. Be aware
			that you'll add one to the load average all the
			time your task is sleeping then. */
		return -EINTR;
	}
}	


Alternatively, you could up() a semaphore for each task that's do be done, 
and down() it again each time you remove one from the queue. 

-- 
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[David Woodhouse]

ttabi@interactivesi.com said:
>  What is wrong with sleep_on()?

Are you asking me? If so, why did I not receive a copy in my inbox? If I 
want to filter duplicates locally, I can. I don't.

It's almost impossible to use it safely, and the few ways you _can_ use it
safely are frowned upon, because they mostly involve using the BKL, usage of
which is slowly being phased out in favour of finer-grained locking.

This kind of code is far too common:

 if (!event) {
	/* BUT WHAT IF THE EVENT ARRIVES _NOW_? */
 	sleep_on(&event_wait);
 }

--
dwmw2


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Andi Kleen]

On Mon, Jan 29, 2001 at 11:01:31AM -0600, Timur Tabi wrote:
> What makes it more frustrating is that some people on this list talk as if
> things things are common knowledge.  I've been following this mailing list for
> months, and until today I had no idea sleep_on was bad.  All the documentation
> I've read to date freely uses sleep_on in the sample code.  In fact, I still

When Linux documentation uses sleep_on it is probably broken and should be 
fixed. Unix (not linux) documentation uses sleep_on commonly, but Unix has
different wait queue semantics and it is usually safe there. 

You're probably reading the wrong documentation, e.g. Rusty's 
kernel hacking HOWTO describes it correctly (and a lot of the other rules) 

> don't even know WHY it's bad.  Not only that, but what am I supposed to use
> instead? 

You can miss wakeups. The standard pattern is:

	get locks

	add_wait_queue(&waitqueue, &wait);
	for (;;) { 
		if (condition you're waiting for is true) 
			break; 
		unlock any non sleeping locks you need for condition
		__set_task_state(current, TASK_UNINTERRUPTIBLE); 
		schedule(); 
		__set_task_state(current, TASK_RUNNING); 
		reaquire locks
	}
	remove_wait_queue(&waitqueue, &wait); 

When you want to handle signals you can check for them before or after the
condition check. Also use TASK_INTERRUPTIBLE in this case.

When you need a timeout use schedule_timeout().

For some cases you can also use the wait_event_* macros which encapsulate
that for you, assuming condition can be tested/used lockless. 

An alternative is to use a semaphore, although that behaves a bit differently
under load.

> This is what I find most frustrating about Linux.  If I were a Windows driver
> programmer, I could walk into any bookstore and pick up any of a dozen books
> that explains everything, leaving no room for doubt.

Just why are Windows drivers so buggy then?


-Andi
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Roman Zippel]

Hi,

On Mon, 29 Jan 2001, Andi Kleen wrote:

> You can miss wakeups. The standard pattern is:
> 
> 	get locks
> 
> 	add_wait_queue(&waitqueue, &wait);
> 	for (;;) { 
> 		if (condition you're waiting for is true) 
> 			break; 
> 		unlock any non sleeping locks you need for condition
> 		__set_task_state(current, TASK_UNINTERRUPTIBLE); 
> 		schedule(); 
> 		__set_task_state(current, TASK_RUNNING); 
> 		reaquire locks
> 	}
> 	remove_wait_queue(&waitqueue, &wait); 

You still miss wakeups. :)
Always set the task state first, then check the condition. See the
wait_event*() macros you mentioned for the right order.

bye, Roman

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Andi Kleen]

On Mon, Jan 29, 2001 at 08:47:50PM +0100, Roman Zippel wrote:
> You still miss wakeups. :)

And there was another race in it, I know.  The first __set_task_state
has to be set_task_state to get the right memory write order on SMP. 




-Andi

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Andrea Arcangeli]

On Mon, Jan 29, 2001 at 09:35:53PM +0100, Andi Kleen wrote:
> On Mon, Jan 29, 2001 at 08:47:50PM +0100, Roman Zippel wrote:
> > You still miss wakeups. :)
> 
> And there was another race in it, I know.  The first __set_task_state
> has to be set_task_state to get the right memory write order on SMP. 

If the wakeup is serialized by the spinlock too (as your code looks like to
assume) you can legally use __set_task_state instead of set_task_state.  An
example of such an usage (where wakeup is serialized by the spinlock) is
lock_sock/unlock_sock.

Andrea

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Andrea Arcangeli]

On Mon, Jan 29, 2001 at 08:47:50PM +0100, Roman Zippel wrote:
> Hi,
> 
> On Mon, 29 Jan 2001, Andi Kleen wrote:
> 
> > You can miss wakeups. The standard pattern is:
> > 
> > 	get locks
> > 
> > 	add_wait_queue(&waitqueue, &wait);
> > 	for (;;) { 
> > 		if (condition you're waiting for is true) 
> > 			break; 
> > 		unlock any non sleeping locks you need for condition
> > 		__set_task_state(current, TASK_UNINTERRUPTIBLE); 
> > 		schedule(); 
> > 		__set_task_state(current, TASK_RUNNING); 
> > 		reaquire locks
> > 	}
> > 	remove_wait_queue(&waitqueue, &wait); 
> 
> You still miss wakeups. :)
> Always set the task state first, then check the condition. See the
> wait_event*() macros you mentioned for the right order.

If the wakeup happens with the spinlock acquired (as the above code seems to
assume) you don't need to set the task state as uninterruptible before checking
the condition, however the above is wrong anyways because it should do
__set_task_state _before_ releasing the lock and not after.

Andrea

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Alan Cox]

> And one more point for the Janitor's list:
> Get rid of superflous irqsave()/irqrestore()'s - in 90% of the cases
> either spin_lock_irq() or spin_lock() is sufficient. That's both faster
> and better readable.

Expect me to drop any submissions that do this. I'd rather take the two
clock hit in most cases because the effect of spin_lock_irq() being used
and people then changing which functions call each other and producing 
impossible to debug irq mishandling cases is unacceptable.

The original Linux network code did this with sti() not save/restore flags.
I've been there before, I am not going to allow a rerun of that disaster for
a few cycles

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Manfred Spraul]

Alan Cox wrote:
> 
> > And one more point for the Janitor's list:
> > Get rid of superflous irqsave()/irqrestore()'s - in 90% of the cases
> > either spin_lock_irq() or spin_lock() is sufficient. That's both faster
> > and better readable.
> 
> Expect me to drop any submissions that do this. I'd rather take the two
> clock hit in most cases because the effect of spin_lock_irq() being used
> and people then changing which functions call each other and producing
> impossible to debug irq mishandling cases is unacceptable.
>

IMHO the main problem of spin_lock_irqsave is not the lost cpu cycles,
but readability:

void public_function()
{
	spin_lock_irqsave();
	if(rare_event)
		internal_function()
	spin_unlock_irqrestore();
}

static void internal_function()
{
	...
	spin_unlock_irq();
	kmalloc(GFP_KERNEL);
	spin_lock_irq();
}

IMHO functions that are not irq safe somewhere hidden in internal
functions should never use spin_lock_irqsave().
make_request() in 2.2 falls into that category, and the irqsave() was
removed.

Obviously spin_lock_irq() instead of spin_lock_irqsave() should only be
done if the implementation doesn't support disabled interrupts, not if
currently noone calls a function with disabled interrupts.

(make_request(), down(), smp_call_function()...)

> The original Linux network code did this with sti() not save/restore flags.
> I've been there before, I am not going to allow a rerun of that disaster for
> a few cycles

I hope that during 2.5 we can add debugging into spin_lock_irq():
BUG() if it's called with disabled interrupts.
It's not yet possible due to schedule() with disabled interrupts (I
tried it a few months ago)

--
	Manfred
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Rusty Russell]

> In message <3A74451F.DA29FD17@uow.edu.au> you write:
> > 	http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> > 
> > A lot of the timer deletion races are hard to fix because of
> > the deadlock problem.

Double take: we *did* fix the problems with del_timer_sync().  We
should probably have renamed del_timer to del_time_async and make
everyone fix their code though.  The `text vanishing under timer in
module' problem is solved by the pending module cleanup for 2.5.

Rusty.
--
Premature optmztion is rt of all evl. --DK
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/

Re: [ANNOUNCE] Kernel Janitor's TODO list

[Andrew Morton]

Rusty Russell wrote:
> 
> > In message <3A74451F.DA29FD17@uow.edu.au> you write:
> > >     http://www.uwsg.iu.edu/hypermail/linux/kernel/0005.3/0269.html
> > >
> > > A lot of the timer deletion races are hard to fix because of
> > > the deadlock problem.
> 
> Double take: we *did* fix the problems with del_timer_sync().

A bit.

> We should probably have renamed del_timer to del_time_async and make
> everyone fix their code though.

That renaming is an absolute precondition.  We just use

#define del_timer_async del_timer

and as the janitors go through fixing stuff, rename known-to-be-correct
usage of del_timer to del_timer_async.  This is the only way
we can keep track of which code still needs looking at.

It's often trivial.  But sometimes not, such as in SCSI.

We also need to clean up the initialisation of timers with some
nice macros, similar to list.h, semaphore.h, etc.  

> The `text vanishing under timer in
> module' problem is solved by the pending module cleanup for 2.5.

mm..  A very common bug is this:

xxx_handler(void *something)
{
	use(something);
or:     assume(something->foo != 1);
}

xxx_close(something)
{
	del_timer(&something->timer);
	kfree(something);
or:     something->foo = 1;
}

So xxx_handler can "use" freed memory.  There really is a large amount
of breakage here.  Just pick a random user of del_timer() and ask
yourself "what if the handler is running after del_timer returns".

Generally, it doesn't happen, because it's in fact quite rare for
a timer to actually expire, and because a lot of the buggy code
is on rarely-used paths such as close() methods.  I've never seen
a bug report which could be attributed to a timer deletion race - partly
because SMP machines are rare, partly because they tend to be used
with a less exotic range of device drivers and partly because some
random subsytem went stupid and there was nothing concrete to report.

Now, there _is_ a correct solution, and that is to create a new timer
API. Probably one in which the timers are reference counted and their
storage is not managed by the users of the API.

It's a shame to create a second API (but we had two timer APIs up to
a few months back anyway...).  But it's also an opportunity.  The
proposed SMP-scalable timers could benefit from not having to be
back-compatible.  Some of the remaining locking and cross-CPU
traffic could be tossed out if a clean slate were available. 

But I don't see a way around the need for synchronous deletion and
the deadlock risk which that introduces.

The morbid amongst us can read the netdev thread from May 2000,
when timer outrage was at its peak:

	http://www.wcug.wwu.edu/lists/netdev/200005/threads.html
-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
Please read the FAQ at http://www.tux.org/lkml/