timer/nohz: Fix timer/nohz woes

From: Anna-Maria Gleixner <[email protected]>

During boot and before base::nohz_active is set in the timer bases, deferrable timers are enqueued into the standard timer base. This works correctly as long as base::nohz_active is false.

Once it base::nohz_active is set and a timer which was enqueued before that is accessed the lock selector code choses the lock of the deferred base. This causes unlocked access to the standard base and in case the timer is removed it does not clear the pending flag in the standard base bitmap which causes get_next_timer_interrupt() to return bogus values.

To prevent that, the deferrable timers must be enqueued in the deferrable base, even when base::nohz_active is not set. Those deferrable timers also need to be expired unconditional.

Fixes 500462a9de65 ("timers: Switch to a non-cascading wheel")

Signed-off-by Anna-Maria Gleixner <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

Cc [email protected]

Cc [email protected]

The conditions in irq_exit() to invoke tick_nohz_irq_exit() are:

  if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu))

This is too permissive in various aspects:

1) If need_resched() is set, then the tick cannot be stopped whether the CPU is idle or in nohz full mode.

Quoted 6 lines · Thomas Gleixner

The conditions in irq_exit() to invoke tick_nohz_irq_exit() are:

  if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu))

This is too permissive in various aspects:

Quoted 2 lines

1) If need_resched() is set, then the tick cannot be stopped whether the CPU is idle or in nohz full mode.

That's not exactly true. In nohz full mode the tick is not restarted on the switch from idle to a single task. And if an idle interrupt wakes up a single task and enqueues a timer, we want that timer to be programmed even though we have need_resched().

Quoted 7 lines · Frederic Weisbecker

Quoted 6 lines · Thomas Gleixner

The conditions in irq_exit() to invoke tick_nohz_irq_exit() are:

  if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu))

This is too permissive in various aspects:

Quoted 7 lines

Quoted 2 lines

1) If need_resched() is set, then the tick cannot be stopped whether the CPU is idle or in nohz full mode.

That's not exactly true. In nohz full mode the tick is not restarted on the switch from idle to a single task. And if an idle interrupt wakes up a single task and enqueues a timer, we want that timer to be programmed even though we have need_resched().

Hrmm, so the check for softirq_pending() should be sufficient, right?

Bah, no. We need to move that into the nohz logic somehow to prevent that repetitive expiry yesterday reprogramming. Lemme think about it some more.

Quoted 2 lines · Thomas Gleixner

Bah, no. We need to move that into the nohz logic somehow to prevent that repetitive expiry yesterday reprogramming. Lemme think about it some more.

The patch below should be the proper cure.

Thanks,

tglx

8<-------------------
Subject: nohz: Prevent a timer interrupt storm in tick_nohz_stop_sched_tick()
From: Thomas Gleixner <[email protected]>
Date: Fri, 22 Dec 2017 15:51:13 +0100

From: Thomas Gleixner <[email protected]>

The conditions in irq_exit() to invoke tick_nohz_irq_exit() which subsequently invokes tick_nohz_stop_sched_tick() are:

  if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu))

If need_resched() is not set, but a timer softirq is pending then this is an indication that the softirq code punted and delegated the execution to softirqd. need_resched() is not true because the current interrupted task takes precedence over softirqd.

Invoking tick_nohz_irq_exit() in this case can cause an endless loop of timer interrupts because the timer wheel contains an expired timer, but softirqs are not yet executed. So it returns an immediate expiry request, which causes the timer to fire immediately again. Lather, rinse and repeat....

Prevent that by adding a check for a pending timer soft interrupt to the conditions in tick_nohz_stop_sched_tick() which avoid calling get_next_timer_interrupt(). That keeps the tick sched timer on the tick and prevents a repetitive programming of an already expired timer.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Sebastian Siewior <[email protected]>

Cc [email protected]

Cc Paul McKenney <[email protected]>

Cc Anna-Maria Gleixner <[email protected]>

---
 kernel/time/tick-sched.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

kernel/time/tick-sched.c +5 -0

@@ -650,6 +650,11 @@ static void tick_nohz_restart(struct tic
 	ts->next_tick = 0;
 }
 
+static inline bool local_timer_softirq_pending(void)
+{
+	return local_softirq_pending & TIMER_SOFTIRQ;
+}
+
 static ktime_t tick_nohz_stop_sched_tick(struct tick_sched *ts,
 					 ktime_t now, int cpu)
 {

Paul was observing weird stalls which are hard to reproduce and decode. We were finally able to reproduce and decode the wreckage on RT.

The following series addresses the issues and hopefully nails the root cause completely.

Please review carefully and expose it to the dreaded rcu torture tests which seem to be the only way to trigger it.

Quoted 6 lines · Thomas Gleixner

Paul was observing weird stalls which are hard to reproduce and decode. We were finally able to reproduce and decode the wreckage on RT.

The following series addresses the issues and hopefully nails the root cause completely.

Quoted 2 lines

Please review carefully and expose it to the dreaded rcu torture tests which seem to be the only way to trigger it.

Best Christmas present ever, thank you!!!

Just started up three concurrent 10-hour runs of the infamous rcutorture TREE01 scenario, and will let you know how it goes!

Quoted 7 lines · Paul E. McKenney

Quoted 6 lines · Thomas Gleixner

Paul was observing weird stalls which are hard to reproduce and decode. We were finally able to reproduce and decode the wreckage on RT.

The following series addresses the issues and hopefully nails the root cause completely.

Quoted 7 lines

Quoted 2 lines

Please review carefully and expose it to the dreaded rcu torture tests which seem to be the only way to trigger it.

Best Christmas present ever, thank you!!!

Just started up three concurrent 10-hour runs of the infamous rcutorture TREE01 scenario, and will let you know how it goes!

Well, I messed up the first test and then reran it. Which had the benefit of giving me a baseline. The rerun (with all four patches) produced failures, so I ran it again with an additional patch of mine. I score these tests by recording the time at first failure, or, if there is no failure, the duration of the test. Summing the values gives the score. And here are the scores, where 30 is a perfect score:

1. Baseline: 3.0+2.5+10=15.5

2. Four patches from Anna-Marie and Thomas: 10+2.7+1.7=14.4

3. Ditto plus the patch below: 10+4.3+10=24.3

Please note that these are nowhere near anything even resembling statistical significance. However, they are encouraging. I will do more runs, but also do shorter five-hour runs to increase the amount of data per unit time. Please note also that my patch by itself never did provide that great of an improvement, so there might be some sort of combination effect going on here. Or maybe it is just luck, who knows?

Please note that I have not yet ported my diagnostic patches on top of these, however, the stacks have the usual schedule_timeout() entries. This is not too surprising from a software-engineering viewpoint:
Locating several bugs at a given point of time usually indicates that there are more to be found. So in a sense we are lucky that the same test triggers at least one of those additional bugs.

Thanx, Paul

------------------------------------------------------------------------

commit accb0edb85526a05b934eac49658d05ea0216fc4

Author: Paul E. McKenney <[email protected]>
Date:   Thu Dec 7 13:18:44 2017 -0800

timers: Ensure that timer_base ->clk accounts for time offline

The timer_base ->must_forward_clk is set to indicate that the next timer operation on that timer_base must check for passage of time. One instance of time passage is when the timer wheel goes idle, and another is when the corresponding CPU is offline. Note that it is not appropriate to set ->is_idle because that could result in IPIing an offline CPU. Therefore, this commit instead sets ->must_forward_clk at CPU-offline time.

Signed-off-by Paul E. McKenney <[email protected]>

kernel/time/timer.c +1 -0

index ffebcf878fba..94cce780c574 100644
@@ -1875,6 +1875,7 @@ int timers_dead_cpu(unsigned int cpu)
 
 		BUG_ON(old_base->running_timer);
 
+		old_base->must_forward_clk = true;
 		for (i = 0; i < WHEEL_SIZE; i++)
 			migrate_timer_list(new_base, old_base->vectors + i);
 

Quoted 16 lines · Paul E. McKenney

Quoted 7 lines · Paul E. McKenney

Quoted 6 lines · Thomas Gleixner

Paul was observing weird stalls which are hard to reproduce and decode. We were finally able to reproduce and decode the wreckage on RT.

The following series addresses the issues and hopefully nails the root cause completely.

Quoted 7 lines

Quoted 2 lines

Please review carefully and expose it to the dreaded rcu torture tests which seem to be the only way to trigger it.

Best Christmas present ever, thank you!!!

Just started up three concurrent 10-hour runs of the infamous rcutorture TREE01 scenario, and will let you know how it goes!

Quoted 6 lines

Well, I messed up the first test and then reran it. Which had the benefit of giving me a baseline. The rerun (with all four patches) produced failures, so I ran it again with an additional patch of mine. I score these tests by recording the time at first failure, or, if there is no failure, the duration of the test. Summing the values gives the score. And here are the scores, where 30 is a perfect score:

Sigh. They were five-hour tests, not ten-hour tests.

1. Baseline: 3.0+2.5+5=10.5

2. Four patches from Anna-Marie and Thomas: 5+2.7+1.7=9.4

3. Ditto plus the patch below: 5+4.3+5=14.3

Oh, and the reason for my suspecting that #2 is actually an improvement over #1 is that my patch by itself produced a very small improvement in reliability. This leads to the hypothesis that #2 really is helping out in some way or another.

But after more than 1,000 hours of test runs, split roughly evenly among the above three scenarios, there is no statistically significant difference in error rate among them. This means that there is some other bug lurking somewhere, and having the same appearance (lost timer). Were you guys ever able to reproduce this via rcutorture?

Quoted 5 lines · Paul E. McKenney

But after more than 1,000 hours of test runs, split roughly evenly among the above three scenarios, there is no statistically significant difference in error rate among them. This means that there is some other bug lurking somewhere, and having the same appearance (lost timer). Were you guys ever able to reproduce this via rcutorture?

No.

We'll setup more testing on Monday. Which of the tests fails or at least exposes the highest failure rate?