fix __mod_timer vs __run_timers deadlock.

When __mod_timer() changes timer's base it waits for the completion
of timer->function. It is just stupid: the caller of __mod_timer() can held locks which would prevent completion of the timer's handler.

Solution: do not change the base of the currently running timer.

Quoted 7 lines · Oleg Nesterov

When __mod_timer() changes timer's base it waits for the completion of timer->function. It is just stupid: the caller of __mod_timer() can held locks which would prevent completion of the timer's handler.

Solution: do not change the base of the currently running timer.

OK, fingers crossed. Juergen, it would be great if you could test Oleg's patch sometime.

Quoted 7 lines · Andrew Morton

Quoted 6 lines · Oleg Nesterov

When __mod_timer() changes timer's base it waits for the completion of timer->function. It is just stupid: the caller of __mod_timer() can held locks which would prevent completion of the timer's handler.

Quoted 4 lines

Quoted 1 line

Solution: do not change the base of the currently running timer.

OK, fingers crossed. Juergen, it would be great if you could test Oleg's patch sometime.

I had one more lockup yesterday but that probably was caused by something else because Azureus is running fine for 24 hours now.

Well, there may be other issues brought by this new timer code though. I'm running G5s regulary without a lockup or anything for weeks, so it would be interesting if you could try to find out what's involved in that other lockup you had.

Quoted 4 lines · Benjamin Herrenschmidt

Well, there may be other issues brought by this new timer code though. I'm running G5s regulary without a lockup or anything for weeks, so it would be interesting if you could try to find out what's involved in that other lockup you had.

It seems like it would not be to hard to create a timer test suite that just hammers the timer subsystem, creating and deleting and modifying zillions of timers, changing the system time, etc. Combined with running with HZ=10000 or something it seems like you could shake out bugs a lot faster than just running & waiting for a race to show up.

I've seen timer related issues (the set_rtc_mmss issue that George Anzinger fixed) while testing the RT patchset that I could only ever reproduce once.

Quoted 4 lines · Benjamin Herrenschmidt

Well, there may be other issues brought by this new timer code though. I'm running G5s regulary without a lockup or anything for weeks, so it would be interesting if you could try to find out what's involved in that other lockup you had.

It seems like it would not be to hard to create a timer test suite that just hammers the timer subsystem, creating and deleting and modifying zillions of timers, changing the system time, etc. Combined with running with HZ=10000 or something it seems like you could shake out bugs a lot faster than just running & waiting for a race to show up.

I've seen timer related issues (the set_rtc_mmss issue that George Anzinger fixed) while testing the RT patchset that I could only ever reproduce once.

Quoted 14 lines · Juergen Kreileder

Quoted 11 lines · Andrew Morton

Quoted 7 lines · Oleg Nesterov

When __mod_timer() changes timer's base it waits for the completion of timer->function. It is just stupid: the caller of __mod_timer() can held locks which would prevent completion of the timer's handler.

Solution: do not change the base of the currently running timer.

OK, fingers crossed. Juergen, it would be great if you could test Oleg's patch sometime.

Quoted 2 lines

I had one more lockup yesterday but that probably was caused by something else because Azureus is running fine for 24 hours now.

Well, there may be other issues brought by this new timer code though. I'm running G5s regulary without a lockup or anything for weeks, so it would be interesting if you could try to find out what's involved in that other lockup you had.

BTW, xmon doesn't work for me. 'echo x > /proc/sysrq-trigger' gives me a :mon> prompt but I can't enter any commands.

Quoted 2 lines · Juergen Kreileder

BTW, xmon doesn't work for me. 'echo x > /proc/sysrq-trigger' gives me a :mon> prompt but I can't enter any commands.

We don't have polled interrupts-off input methods for USB keyboards.

Quoted 5 lines · On Tue, 3 May 2005 12:48:40 +1000 Paul Mackerras <[email protected]> wrote:

Quoted 2 lines · Juergen Kreileder

BTW, xmon doesn't work for me. 'echo x > /proc/sysrq-trigger' gives me a :mon> prompt but I can't enter any commands.

Quoted 1 line

We don't have polled interrupts-off input methods for USB keyboards.

There is nothing in the USB nor INPUT layers really blocking an implementation of this BTW.

I need this on Sparc64 as well, so that PROM command line input works after booting with USB keyboards.

Nothing prevents it ? well, I wouldn't be that optimistic :) The USB stuff is a bit complex, it inlcudes doing DMAs, so manipulating the iommu, dealing with URB queues (and thus allocating/releasing them) etc... and especially in the context of xmon, that mean letting the driver do a lot of these at any time whatever state the system is...

Quoted 5 lines · On Wed, 04 May 2005 09:44:53 +1000 Benjamin Herrenschmidt…

Nothing prevents it ? well, I wouldn't be that optimistic :) The USB stuff is a bit complex, it inlcudes doing DMAs, so manipulating the iommu, dealing with URB queues (and thus allocating/releasing them) etc... and especially in the context of xmon, that mean letting the driver do a lot of these at any time whatever state the system is...

I think doing calls to the USB interrupt handler would work. I'm not being crazy :)

But yeah we could do a micro-stack as well, but as you noted the transfer to/from the real USB HCI driver would be non-trivial.

I truly believe just calling the real USB HCI driver interrupt handler in a polling fashion is the way to go.

The bug was identified by Maneesh Soni.

When __mod_timer() changes timer's base it waits for the completion of timer->function. It is just stupid: the caller of __mod_timer() can held locks which would prevent completion of the timer's handler.

Solution: do not change the base of the currently running timer.

Side effect: __mod_timer() doesn't garantees anymore that timer will run on the local cpu.

Signed-off-by Oleg Nesterov <[email protected]>

kernel/timer.c renamed from kernel/timer.c~ +30 -32

@@ -212,41 +212,39 @@ int __mod_timer(struct timer_list *timer
 	timer_base_t *base;
 	tvec_base_t *new_base;
 	unsigned long flags;
-	int ret = -1;
+	int ret;
 
 	BUG_ON(!timer->function);
 	check_timer(timer);
-
-	do {
-		base = lock_timer_base(timer, &flags);
-		new_base = &__get_cpu_var(tvec_bases);
-
-		/* Ensure the timer is serialized. */
-		if (base != &new_base->t_base
-			&& base->running_timer == timer)
-			goto unlock;
-
-		ret = 0;
-		if (timer_pending(timer)) {
-			detach_timer(timer, 0);
-			ret = 1;
-		}
-
-		if (base != &new_base->t_base) {
-			timer->base = NULL;
-			/* Safe: the timer can't be seen via ->entry,
-			 * and lock_timer_base checks ->base != 0. */
-			spin_unlock(&base->lock);
-			base = &new_base->t_base;
-			spin_lock(&base->lock);
-			timer->base = base;
-		}
-
-		timer->expires = expires;
-		internal_add_timer(new_base, timer);
-unlock:
-		spin_unlock_irqrestore(&base->lock, flags);
-	} while (ret < 0);
+
+	base = lock_timer_base(timer, &flags);
+
+	ret = 0;
+	if (timer_pending(timer)) {
+		detach_timer(timer, 0);
+		ret = 1;
+	}
+
+	new_base = &__get_cpu_var(tvec_bases);
+
+	if (base != &new_base->t_base) {
+		if (unlikely(base->running_timer == timer))
+			/* Don't change timer's base while it is running.
+			 * Needed for serialization of timer wrt itself. */
+			new_base = container_of(base, tvec_base_t, t_base);
+		else {
+			timer->base = NULL;
+			/* Safe: the timer can't be seen via ->entry,
+			 * and lock_timer_base checks ->base != 0. */
+			spin_unlock(&base->lock);
+			spin_lock(&new_base->t_base.lock);
+			timer->base = &new_base->t_base;
+		}
+	}
+
+	timer->expires = expires;
+	internal_add_timer(new_base, timer);
+	spin_unlock_irqrestore(&new_base->t_base.lock, flags);
 
 	return ret;
 }