Re: PATCH: Race in 2.6.0-test2 timer code

[Andrea Arcangeli <andrea@suse.de>]

On Wed, Jul 30, 2003 at 09:40:14AM +0200, Ingo Molnar wrote:
> 
> On Wed, 30 Jul 2003, Andrea Arcangeli wrote:
> 
> > [...] I'd feel much safer with the whole timer API being smp safe w/o
> > requiring driver developers to add complicated external brainer locking.
> > This will provide a much more friendly abstraction.
> 
> i agree with the goal, but your patch does not achieve this. Your patch
> does not make double-add_timer() safe for example. As far as i can see
> your 2.6 patch only introduces additional overhead.

I never did any 2.6 patch, so it maybe a different thing what you've
seen, not what I applied to 2.4. Infact even the 2.4 patch isn't from
me.

This is the fix I applied to my 2.4 tree and it definitely fixes the
problem, including add_timer against add_timer. And it fixes the bug in
practice, it usually took a few hours to crash, not it can run for days
stable and I never heard complains again.

--- linux-2.4.19.SuSE-orig/kernel/timer.c	2003-07-02 13:53:55.000000000 -0700
+++ linux-2.4.19.SuSE-timer/kernel/timer.c	2003-07-02 18:32:08.000000000 -0700
@@ -144,17 +144,22 @@ void add_timer(timer_t *timer)
 
 	CHECK_BASE(base);
 	CHECK_BASE(timer->base);
-	spin_lock_irqsave(&base->lock, flags);
-	if (timer_pending(timer))
-		goto bug;
-	internal_add_timer(base, timer);
-	timer->base = base;
-	spin_unlock_irqrestore(&base->lock, flags);
-	return;
-bug:
-	spin_unlock_irqrestore(&base->lock, flags);
-	printk("bug: kernel timer added twice at %p.\n",
-			__builtin_return_address(0));
+
+	local_irq_save(flags);
+	while (unlikely(test_and_set_bit(0, &timer->lock)))
+		while (test_bit(0, &timer->lock));
+
+	spin_lock(&base->lock);
+	if (timer_pending(timer)) {
+		printk("bug: kernel timer added twice at %p.\n",
+				__builtin_return_address(0));
+	} else {
+		internal_add_timer(base, timer);
+		timer->base = base;
+	}
+	spin_unlock(&base->lock);
+	clear_bit(0, &timer->lock);
+	local_irq_restore(flags);
 }

 static inline int detach_timer(timer_t *timer)
@@ -244,16 +249,24 @@ int del_timer(timer_t * timer)
 	CHECK_BASE(timer->base);
 	if (!timer->base)
 		return 0;
+
+	local_irq_save(flags);
+	while (unlikely(test_and_set_bit(0, &timer->lock)))
+		while (test_bit(0, &timer->lock));
+
 repeat:
  	base = timer->base;
-	spin_lock_irqsave(&base->lock, flags);
+	spin_lock(&base->lock);
 	if (base != timer->base) {
-		spin_unlock_irqrestore(&base->lock, flags);
+		spin_unlock(&base->lock);
 		goto repeat;
 	}
 	ret = detach_timer(timer);
 	timer->list.next = timer->list.prev = NULL;
-	spin_unlock_irqrestore(&base->lock, flags);
+	spin_unlock(&base->lock);
+
+	clear_bit(0, &timer->lock);
+	local_irq_restore(flags);

 	return ret;
 }
@@ -274,34 +287,48 @@ void sync_timers(void)

 int del_timer_sync(timer_t * timer)
 {
+	unsigned long flags;
 	tvec_base_t * base;
 	int ret = 0;

 	CHECK_BASE(timer->base);
 	if (!timer->base)
 		return 0;
+
+	local_irq_save(flags);
+	while (unlikely(test_and_set_bit(0, &timer->lock)))
+		while (test_bit(0, &timer->lock));
+
 	for (;;) {
-		unsigned long flags;
 		int running;
-
 repeat:
 	 	base = timer->base;
-		spin_lock_irqsave(&base->lock, flags);
+		spin_lock(&base->lock);
 		if (base != timer->base) {
-			spin_unlock_irqrestore(&base->lock, flags);
+			spin_unlock(&base->lock);
 			goto repeat;
 		}
 		ret += detach_timer(timer);
 		timer->list.next = timer->list.prev = 0;
 		running = timer_is_running(base, timer);
-		spin_unlock_irqrestore(&base->lock, flags);
+		spin_unlock(&base->lock);

 		if (!running)
 			break;
+
+		clear_bit(0, &timer->lock);
+		local_irq_restore(flags);

 		timer_synchronize(base, timer);
+
+		local_irq_save(flags);
+		while (unlikely(test_and_set_bit(0, &timer->lock)))
+			while (test_bit(0, &timer->lock));
 	}

+	clear_bit(0, &timer->lock);
+	local_irq_restore(flags);
+
 	return ret;
 }
 #endif

Andrea