dynticks: don't unlock spinlock twice

PATCH In review Andres Salomon 3 participants 9 messages Mar 3, 2007 – Mar 5, 2007

Subsystems (2)

High-resolution timers, timer wheel, clockevents Nohz, dynticks support

lore

Share & tools

lore

Andres Salomon

author

2007-03-03 02:52 UTC #0 lore raw

PATCH 1/1 dynticks: don't unlock spinlock twice

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 <[email protected]>

kernel/time/tick-sched.c +4 -6

index 51556b9..b43bccb 100644
@@ -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 */

Thomas Gleixner

2007-03-03 08:56 UTC #1 lore raw

Quoted 18 lines · Andres Salomon

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

...

Quoted 10 lines

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

Andres Salomon

author

2007-03-04 11:14 UTC #2 lore raw

Quoted 1 line · Thomas Gleixner

On Fri, 2007-03-02 at 21:52 -0500, Andres Salomon wrote:

[...]

Quoted 8 lines

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?

Thomas Gleixner

2007-03-04 13:04 UTC #3 lore raw

PATCH 1/1 highres: Do not run the TIMER_SOFTIRQ after switching to highres mode

Quoted 2 lines

Quoted 2 lines
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 <[email protected]>

kernel/hrtimer.c +7 -5

index 476cb0c..b2dd223 100644
@@ -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);

Andres Salomon

author

2007-03-04 13:12 UTC #4 lore raw

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

Quoted 11 lines · Thomas Gleixner

Quoted 2 lines
Quoted 2 lines
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 <[email protected]>

include/linux/hrtimer.h +0 -0

kernel/hrtimer.c +1 -0

index 476cb0c..4dcdd09 100644
@@ -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;

Thomas Gleixner

Acked-by

2007-03-04 13:40 UTC #5 lore raw

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

Quoted 13 lines · Andres Salomon

Quoted 11 lines · Thomas Gleixner
Quoted 2 lines
Quoted 2 lines
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.

Quoted 5 lines

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 <[email protected]>

Acked-by Thomas Gleixner <[email protected]>

Ingo Molnar

2007-03-05 07:33 UTC #6 lore raw

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

In reply to Andres Salomon

Quoted 13 lines · Andres Salomon

Quoted 11 lines · Thomas Gleixner
Quoted 2 lines
Quoted 2 lines
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.

Quoted 3 lines

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

Andres Salomon

author

2007-03-05 07:50 UTC #7 lore raw

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

Quoted 17 lines · Ingo Molnar

Quoted 11 lines · Andres Salomon
Quoted 10 lines · Thomas Gleixner
Quoted 2 lines
Quoted 2 lines
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.
Quoted 3 lines
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.

Quoted 5 lines

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..

Ingo Molnar

2007-03-05 08:00 UTC #8 lore raw

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

Quoted 10 lines · Andres Salomon

Quoted 7 lines
Quoted 3 lines
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