[PATCH] dynticks: don't unlock spinlock twice

[PATCH] dynticks: don't unlock spinlock twice

[Andres Salomon]

[-- Attachment #1: Type: text/plain, Size: 1415 bytes --]

During boot with dynticks enabled, we would sometimes get:

[   35.271900] Switched to high resolution mode on CPU 0
[   35.304468] BUG: spinlock already unlocked on CPU#0, swapper/1
[   35.338099]  lock: c06428a0, .magic: dead4ead, .owner: <none>/-1,
.owner_cpu:
 -1
[   35.373597]  [<c04d7cf0>] _raw_spin_unlock+0x28/0x67
[   35.406647]  [<c05ba279>] _spin_unlock+0x5/0x23
[   35.439369]  [<c04255f7>] tick_sched_timer+0x4e/0xa7
[   35.472388]  [<c04255a9>] tick_sched_timer+0x0/0xa7
[   35.504833]  [<c0422528>] hrtimer_run_queues+0x199/0x1ec
[   35.537617]  [<c0416b72>] run_timer_softirq+0x12/0x166
[   35.570019]  [<c04144d9>] __do_softirq+0x40/0x85

[   35.601542]  [<c0405494>] do_softirq+0x53/0xa9
...

This appears to be caused by run_hrtimer_queue() (called by
hrtimer_run_queues) calling spin_unlock_irq(&cpu_base->lock) before
calling timer->function(timer).  The callback function
(tick_sched_timer) expects cpu_base->lock to be held when it is called,
and attempts to unlock it.  Since it doesn't seem like anything within
tick_sched_timer really needs to hold the lock (afaict), the attached
patch simply removes the lock handling from tick_sched_timer.  Things
called by tick_sched_timer may grab the base->lock, but that's fine (and
their responsibility).  Let me know if there's some reason why the lock
should be held, and I can rework this.

Signed-off-by: Andres Salomon <dilinger@debian.org>

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1415 bytes --]

diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 51556b9..b43bccb 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -422,13 +422,12 @@ static inline void tick_nohz_switch_to_nohz(void) { }
 #ifdef CONFIG_HIGH_RES_TIMERS
 /*
  * We rearm the timer until we get disabled by the idle code
- * Called with interrupts disabled and timer->base->cpu_base->lock held.
+ * Called with interrupts disabled and timer->base->cpu_base->lock *not* held.
  */
 static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
 {
 	struct tick_sched *ts =
 		container_of(timer, struct tick_sched, sched_timer);
-	struct hrtimer_cpu_base *base = timer->base->cpu_base;
 	struct pt_regs *regs = get_irq_regs();
 	ktime_t now = ktime_get();
 
@@ -454,13 +453,12 @@ static enum hrtimer_restart tick_sched_timer(struct hrtimer *timer)
 		}
 		/*
 		 * update_process_times() might take tasklist_lock, hence
-		 * drop the base lock. sched-tick hrtimers are per-CPU and
-		 * never accessible by userspace APIs, so this is safe to do.
+		 * we don't attempt to grab the base lock here.
+		 * sched-tick hrtimers are per-CPU and never accessible by
+		 * userspace APIs, so this is safe to do.
 		 */
-		spin_unlock(&base->lock);
 		update_process_times(user_mode(regs));
 		profile_tick(CPU_PROFILING);
-		spin_lock(&base->lock);
 	}
 
 	/* Do not restart, when we are in the idle loop */

Re: [PATCH] dynticks: don't unlock spinlock twice

[Thomas Gleixner]

On Fri, 2007-03-02 at 21:52 -0500, Andres Salomon wrote:
> During boot with dynticks enabled, we would sometimes get:
> 
> [   35.271900] Switched to high resolution mode on CPU 0
> [   35.304468] BUG: spinlock already unlocked on CPU#0, swapper/1
> [   35.338099]  lock: c06428a0, .magic: dead4ead, .owner: <none>/-1,
> .owner_cpu:
>  -1
> [   35.373597]  [<c04d7cf0>] _raw_spin_unlock+0x28/0x67
> [   35.406647]  [<c05ba279>] _spin_unlock+0x5/0x23
> [   35.439369]  [<c04255f7>] tick_sched_timer+0x4e/0xa7
> [   35.472388]  [<c04255a9>] tick_sched_timer+0x0/0xa7
> [   35.504833]  [<c0422528>] hrtimer_run_queues+0x199/0x1ec
> [   35.537617]  [<c0416b72>] run_timer_softirq+0x12/0x166
> [   35.570019]  [<c04144d9>] __do_softirq+0x40/0x85
> 
> [   35.601542]  [<c0405494>] do_softirq+0x53/0xa9
> ...
> 
> This appears to be caused by run_hrtimer_queue() (called by
> hrtimer_run_queues) calling spin_unlock_irq(&cpu_base->lock) before
> calling timer->function(timer).  The callback function
> (tick_sched_timer) expects cpu_base->lock to be held when it is called,
> and attempts to unlock it.  Since it doesn't seem like anything within
> tick_sched_timer really needs to hold the lock (afaict), the attached
> patch simply removes the lock handling from tick_sched_timer.  Things
> called by tick_sched_timer may grab the base->lock, but that's fine (and
> their responsibility).  Let me know if there's some reason why the lock
> should be held, and I can rework this.

The BUG is somewhere else. the tick timer should never be seen in
hrtimer_run_queues. The tick timer is operated solely from the timer
interrupt. 

The question is, how the tick timer gets enqueued in the softirq queue.
Can you isolate the codepath, where this happens ?

	tglx



	

Re: [PATCH] dynticks: don't unlock spinlock twice

[Andres Salomon]

Thomas Gleixner wrote:
> On Fri, 2007-03-02 at 21:52 -0500, Andres Salomon wrote:
[...]
> 
> The BUG is somewhere else. the tick timer should never be seen in
> hrtimer_run_queues. The tick timer is operated solely from the timer
> interrupt. 
> 
> The question is, how the tick timer gets enqueued in the softirq queue.
> Can you isolate the codepath, where this happens ?
> 

Only 4 functions call timer->function; hrtimer_enequeue_reprogram,
hrtimer_interrupt, run_hrtimer_softirq, and run_hrtimer_queue.

- hrtimer_enqueue_reprogram will not call CB_IRQSAFE_NO_SOFTIRQ; this
function is not the culprit.  It does move CB_SOFTIRQs onto the
cb_pending list.

- hrtimer_interrupt is called from a hardirq context, and only runs
CB_IRQSAFE*.  CB_SOFTIRQs are moved onto the cb_pending list.  Since
irqs are disabled, this function is not the culprit.

- run_hrtimer_softirq is called from the HRTIMER_SOFTIRQ, and only
operates on softirqs (via the cb_pending list).  CB_IRQSAFE_NO_SOFTIRQ
callbacks are never added to the cb_pending list, so this function is
not the culprit.

- run_hrtimer_queue is called from TIMER_SOFTIRQ, and runs on all
timers.  This function is what shows up in the backtrace, and should not
be calling tick_sched_timer.

Now, the question is; what *should* it be doing?  Since it is only ever
run from a softirq context, should it be checking for
!CB_IRQSAFE_NO_SOFTIRQ before running the timers?  Should it only be
processing timers that have been added to cb_pending (which are
guaranteed to be softirq-friendly)?   Is there even a reason why we have
a run_hrtimer_softirq that processes cb_pending, as well as a
run_timer_softirq that calls hrtimer_run_queues?  Why should the timer
softirq (which is raised by, amongst other things, update_process_times,
which is called by our friend tick_sched_timer) care about the hrtimer
queues?

[PATCH] highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

[Thomas Gleixner]

> > The question is, how the tick timer gets enqueued in the softirq queue.
> > Can you isolate the codepath, where this happens ?

The TIMER_SOFTIRQ runs the hrtimers during bootup until a usable
clocksource and clock event sources are registered. The switch to high
resolution mode happens inside of the TIMER_SOFTIRQ, but runs the
softirq afterwards. That way the tick emulation timer, which was set up
in the switch to highres might be executed in the softirq context, which
is a BUG. The rbtree has not to be touched by the softirq after the
highres switch.

This BUG was observed by Andres Salomon, who provided the information to
debug it.

Return early from the softirq, when the switch was sucessful. 

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 476cb0c..b2dd223 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -540,19 +540,19 @@ static inline int hrtimer_enqueue_reprog
 /*
  * Switch to high resolution mode
  */
-static void hrtimer_switch_to_hres(void)
+static int hrtimer_switch_to_hres(void)
 {
 	struct hrtimer_cpu_base *base = &__get_cpu_var(hrtimer_bases);
 	unsigned long flags;
 
 	if (base->hres_active)
-		return;
+		return 1;
 
 	local_irq_save(flags);
 
 	if (tick_init_highres()) {
 		local_irq_restore(flags);
-		return;
+		return 0;
 	}
 	base->hres_active = 1;
 	base->clock_base[CLOCK_REALTIME].resolution = KTIME_HIGH_RES;
@@ -565,13 +565,14 @@ static void hrtimer_switch_to_hres(void)
 	local_irq_restore(flags);
 	printk(KERN_INFO "Switched to high resolution mode on CPU %d\n",
 	       smp_processor_id());
+	return 1;
 }
 
 #else
 
 static inline int hrtimer_hres_active(void) { return 0; }
 static inline int hrtimer_is_hres_enabled(void) { return 0; }
-static inline void hrtimer_switch_to_hres(void) { }
+static inline int hrtimer_switch_to_hres(void) { return 0; }
 static inline void hrtimer_force_reprogram(struct hrtimer_cpu_base *base) { }
 static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
 					    struct hrtimer_clock_base *base)
@@ -1173,7 +1174,8 @@ void hrtimer_run_queues(void)
 	 * deadlock vs. xtime_lock.
 	 */
 	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled()))
-		hrtimer_switch_to_hres();
+		if (hrtimer_switch_to_hres())
+			return;
 
 	hrtimer_get_softirq_time(cpu_base);
 

 

Re: [PATCH] highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

[Andres Salomon]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

Thomas Gleixner wrote:
>>> The question is, how the tick timer gets enqueued in the softirq queue.
>>> Can you isolate the codepath, where this happens ?
> 
> The TIMER_SOFTIRQ runs the hrtimers during bootup until a usable
> clocksource and clock event sources are registered. The switch to high
> resolution mode happens inside of the TIMER_SOFTIRQ, but runs the
> softirq afterwards. That way the tick emulation timer, which was set up
> in the switch to highres might be executed in the softirq context, which
> is a BUG. The rbtree has not to be touched by the softirq after the
> highres switch.
> 

And an additional request, just to make it explicit that we should not
have any NO_SOFTIRQ callbacks in the tree; BUG out if we encounter such
a thing.

Signed-off-by: Andres Salomon <dilinger@debian.org>

[-- Attachment #2: hrtimer4.patch --]
[-- Type: text/x-patch, Size: 476 bytes --]

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 476cb0c..4dcdd09 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -1130,6 +1130,7 @@ static inline void run_hrtimer_queue(struct hrtimer_cpu_base *cpu_base,
 		if (base->softirq_time.tv64 <= timer->expires.tv64)
 			break;
 
+		BUG_ON(timer->cb_mode == HRTIMER_CB_IRQSAFE_NO_SOFTIRQ);
 		timer_stats_account_hrtimer(timer);
 
 		fn = timer->function;

Re: [PATCH] highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

[Thomas Gleixner]

On Sun, 2007-03-04 at 08:12 -0500, Andres Salomon wrote:
> Thomas Gleixner wrote:
> >>> The question is, how the tick timer gets enqueued in the softirq queue.
> >>> Can you isolate the codepath, where this happens ?
> > 
> > The TIMER_SOFTIRQ runs the hrtimers during bootup until a usable
> > clocksource and clock event sources are registered. The switch to high
> > resolution mode happens inside of the TIMER_SOFTIRQ, but runs the
> > softirq afterwards. That way the tick emulation timer, which was set up
> > in the switch to highres might be executed in the softirq context, which
> > is a BUG. The rbtree has not to be touched by the softirq after the
> > highres switch.
> > 
> 
> And an additional request, just to make it explicit that we should not
> have any NO_SOFTIRQ callbacks in the tree; BUG out if we encounter such
> a thing.
> 
> Signed-off-by: Andres Salomon <dilinger@debian.org>

Acked-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH] highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

[Ingo Molnar]

* Andres Salomon <dilinger@debian.org> wrote:

> Thomas Gleixner wrote:
> >>> The question is, how the tick timer gets enqueued in the softirq queue.
> >>> Can you isolate the codepath, where this happens ?
> > 
> > The TIMER_SOFTIRQ runs the hrtimers during bootup until a usable
> > clocksource and clock event sources are registered. The switch to high
> > resolution mode happens inside of the TIMER_SOFTIRQ, but runs the
> > softirq afterwards. That way the tick emulation timer, which was set up
> > in the switch to highres might be executed in the softirq context, which
> > is a BUG. The rbtree has not to be touched by the softirq after the
> > highres switch.
> > 
> 
> And an additional request, just to make it explicit that we should not 
> have any NO_SOFTIRQ callbacks in the tree; BUG out if we encounter 
> such a thing.

please change it to WARN_ON_ONCE()...

'bug out' might mean: 'dead box'/'no resume'/'no bootup'.

	Ingo

Re: [PATCH] highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

[Andres Salomon]

Ingo Molnar wrote:
> * Andres Salomon <dilinger@debian.org> wrote:
> 
>> Thomas Gleixner wrote:
>>>>> The question is, how the tick timer gets enqueued in the softirq queue.
>>>>> Can you isolate the codepath, where this happens ?
>>> The TIMER_SOFTIRQ runs the hrtimers during bootup until a usable
>>> clocksource and clock event sources are registered. The switch to high
>>> resolution mode happens inside of the TIMER_SOFTIRQ, but runs the
>>> softirq afterwards. That way the tick emulation timer, which was set up
>>> in the switch to highres might be executed in the softirq context, which
>>> is a BUG. The rbtree has not to be touched by the softirq after the
>>> highres switch.
>>>
>> And an additional request, just to make it explicit that we should not 
>> have any NO_SOFTIRQ callbacks in the tree; BUG out if we encounter 
>> such a thing.
> 
> please change it to WARN_ON_ONCE()...
> 
> 'bug out' might mean: 'dead box'/'no resume'/'no bootup'.
> 
> 	Ingo


Certainly; note that hrtimers.c has quite a few BUG calls which could
render a box dead, though.

Patch to follow..

Re: [PATCH] highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

[Ingo Molnar]

* Andres Salomon <dilinger@debian.org> wrote:

> >> And an additional request, just to make it explicit that we should 
> >> not have any NO_SOFTIRQ callbacks in the tree; BUG out if we 
> >> encounter such a thing.
> > 
> > please change it to WARN_ON_ONCE()...
> > 
> > 'bug out' might mean: 'dead box'/'no resume'/'no bootup'.
>
> Certainly; note that hrtimers.c has quite a few BUG calls which could 
> render a box dead, though.

yep, bad habbit from the days when the vga console was always there to 
print stuff on.

	Ingo

Re: [PATCH] highres: do not run the TIMER_SOFTIRQ after switching to highres mode

[Linus Torvalds]

On Tue, 6 Mar 2007, Thomas Gleixner wrote:
> 
> /me pleads guilty !
> 
> Replacement patch below.

Well, I already applied the original one that came through Andrew, so I 
really just wanted to note the coding style in general, and your fixed 
patch no longer applied ;)

Also, you do end up testing for "not error" with a

>  	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled()))
> -		hrtimer_switch_to_hres();
> +		if (!hrtimer_switch_to_hres())
> +			return;

which I guess is ok, if only because we simply don't care about what the 
exact error was. But it means that this particular code sequence ends up 
having the same problem (which is still fewer places than the original 
patch, so we're good).

I personally hate the

	if (hrtimer_switch_to_hres() == SUCCESS)
		return;

kind of syntax (it's just too long, and it's *not* obvious at all that 
SUCCESS is zero and that this is a "negative error or zero" kind of 
function, so it's actually *worse* than just doing what you did, but some 
projects seem to have that kind of approach.

We could encourage people to do

	if (hrtimer_switch_to_hres() >= 0)
		return;

which is fairly obviously a "success" case for a negative error value, but 
I'm not sure the extra typing really is worth it. Does anybody have any 
smart ideas that people might even be ok with following (just making 
things more cumbersome is anti-productive, so I don't want to have some 
stupid rule that everybody really hates)?

			Linus

[PATCH] highres: do not run the TIMER_SOFTIRQ after switching to highres mode

[Thomas Gleixner]

On Tue, 2007-03-06 at 08:44 -0800, Linus Torvalds wrote:
> This is just a coding style thing, but I thought I should really point it 
> out, because these kinds of things quite often result in nasty bugs simply 
> because the source code is so hard to read properly:

/me pleads guilty !

Replacement patch below.

	tglx

--------------------->

The TIMER_SOFTIRQ runs the hrtimers during bootup until a usable
clocksource and clock event sources are registered. The switch to high
resolution mode happens inside of the TIMER_SOFTIRQ, but runs the
softirq afterwards. That way the tick emulation timer, which was set up
in the switch to highres might be executed in the softirq context, which
is a BUG. The rbtree has not to be touched by the softirq after the
highres switch.

This BUG was observed by Andres Salomon, who provided the information to
debug it.

Return early from the softirq, when the switch was sucessful.

Remove also the superfluid hres_active check on top of
hrtimer_switch_to_hres(), as we never get there, once we switched over.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Cc: Andres Salomon <dilinger@debian.org>

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index de93a81..2e465df 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -540,19 +540,18 @@ static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
 /*
  * Switch to high resolution mode
  */
-static void hrtimer_switch_to_hres(void)
+static int hrtimer_switch_to_hres(void)
 {
 	struct hrtimer_cpu_base *base = &__get_cpu_var(hrtimer_bases);
 	unsigned long flags;
-
-	if (base->hres_active)
-		return;
+	int err;
 
 	local_irq_save(flags);
 
-	if (tick_init_highres()) {
+	err = tick_init_highres();
+	if (err < 0) {
 		local_irq_restore(flags);
-		return;
+		return err;
 	}
 	base->hres_active = 1;
 	base->clock_base[CLOCK_REALTIME].resolution = KTIME_HIGH_RES;
@@ -565,13 +564,14 @@ static void hrtimer_switch_to_hres(void)
 	local_irq_restore(flags);
 	printk(KERN_INFO "Switched to high resolution mode on CPU %d\n",
 	       smp_processor_id());
+	return 0;
 }
 
 #else
 
 static inline int hrtimer_hres_active(void) { return 0; }
 static inline int hrtimer_is_hres_enabled(void) { return 0; }
-static inline void hrtimer_switch_to_hres(void) { }
+static inline int hrtimer_switch_to_hres(void) { return -EINVAL; }
 static inline void hrtimer_force_reprogram(struct hrtimer_cpu_base *base) { }
 static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
 					    struct hrtimer_clock_base *base)
@@ -1173,7 +1173,8 @@ void hrtimer_run_queues(void)
 	 * deadlock vs. xtime_lock.
 	 */
 	if (tick_check_oneshot_change(!hrtimer_is_hres_enabled()))
-		hrtimer_switch_to_hres();
+		if (!hrtimer_switch_to_hres())
+			return;
 
 	hrtimer_get_softirq_time(cpu_base);