Kernel Janitor's TODO list

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Em Sun, Jan 28, 2001 at 03:20:18PM +0000, David Woodhouse escreveu:

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

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?

Mike

 Michael H. Warfield    |  (770) 985-6132   |  [email protected]
  (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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

In message <[email protected]> you write:

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

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.

@@ -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)

@@ -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:

@@ -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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Yes, oops, here is the fix.

@@ -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)

@@ -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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Em Mon, Jan 29, 2001 at 03:13:19AM +1100, Andrew Morton escreveu:

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/

Em Sun, Jan 28, 2001 at 12:28:50PM -0200, Arnaldo Carvalho de Melo escreveu:

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 [email protected]
Please read the FAQ at http://www.tux.org/lkml/