[patch 1/4] timer: Use deferrable base independent of base::nohz_active

[patch 1/4] timer: Use deferrable base independent of base::nohz_active

[Thomas Gleixner]

[-- Attachment #1: timers--Use_deferrable_base_unconditional_of_nohz_active.patch --]
[-- Type: text/plain, Size: 2601 bytes --]

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

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 <anna-maria@linutronix.de>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Cc: rt@linutronix.de

---
 kernel/time/timer.c |   16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -823,11 +823,10 @@ static inline struct timer_base *get_tim
 	struct timer_base *base = per_cpu_ptr(&timer_bases[BASE_STD], cpu);
 
 	/*
-	 * If the timer is deferrable and nohz is active then we need to use
-	 * the deferrable base.
+	 * If the timer is deferrable and NO_HZ_COMMON is set then we need
+	 * to use the deferrable base.
 	 */
-	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active &&
-	    (tflags & TIMER_DEFERRABLE))
+	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE))
 		base = per_cpu_ptr(&timer_bases[BASE_DEF], cpu);
 	return base;
 }
@@ -837,11 +836,10 @@ static inline struct timer_base *get_tim
 	struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
 
 	/*
-	 * If the timer is deferrable and nohz is active then we need to use
-	 * the deferrable base.
+	 * If the timer is deferrable and NO_HZ_COMMON is set then we need
+	 * to use the deferrable base.
 	 */
-	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active &&
-	    (tflags & TIMER_DEFERRABLE))
+	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && (tflags & TIMER_DEFERRABLE))
 		base = this_cpu_ptr(&timer_bases[BASE_DEF]);
 	return base;
 }
@@ -1684,7 +1682,7 @@ static __latent_entropy void run_timer_s
 	base->must_forward_clk = false;
 
 	__run_timers(base);
-	if (IS_ENABLED(CONFIG_NO_HZ_COMMON) && base->nohz_active)
+	if (IS_ENABLED(CONFIG_NO_HZ_COMMON))
 		__run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
 }
 

Re: [patch 1/4] timer: Use deferrable base independent of base::nohz_active

[Frederic Weisbecker]

On Fri, Dec 22, 2017 at 03:51:12PM +0100, Thomas Gleixner wrote:
> From: Anna-Maria Gleixner <anna-maria@linutronix.de>
> 
> 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 <anna-maria@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: stable@vger.kernel.org
> Cc: rt@linutronix.de

Nice catch!

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Thanks!

[patch 2/4] nohz: Prevent erroneous tick stop invocations

[Thomas Gleixner]

[-- Attachment #1: nohz--Prevent-erroneous-tick-stop-invocations.patch --]
[-- Type: text/plain, Size: 1619 bytes --]

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.

  2) If need_resched() is not set, but softirqs are 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 these cases 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 making the conditions proper and only allow invokation when
in idle or nohz full mode and neither need_resched() nor
local_softirq_pending() are set.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
---
 kernel/softirq.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -382,7 +382,8 @@ static inline void tick_irq_exit(void)
 	int cpu = smp_processor_id();
 
 	/* Make sure that timer wheel updates are propagated */
-	if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
+	if ((idle_cpu(cpu) || tick_nohz_full_cpu(cpu)) &&
+	    !need_resched() && !local_softirq_pending()) {
 		if (!in_interrupt())
 			tick_nohz_irq_exit();
 	}

Re: [patch 2/4] nohz: Prevent erroneous tick stop invocations

[Frederic Weisbecker]

On Fri, Dec 22, 2017 at 03:51:13PM +0100, Thomas Gleixner wrote:
> 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.

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

Re: [patch 2/4] nohz: Prevent erroneous tick stop invocations

[Thomas Gleixner]

On Tue, 26 Dec 2017, Frederic Weisbecker wrote:

> On Fri, Dec 22, 2017 at 03:51:13PM +0100, Thomas Gleixner wrote:
> > 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.
> 
> 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?

--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -382,7 +382,8 @@ static inline void tick_irq_exit(void)
 	int cpu = smp_processor_id();
 
 	/* Make sure that timer wheel updates are propagated */
-	if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
+	if (((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) &&
+	if ((idle_cpu(cpu) || tick_nohz_full_cpu(cpu)) &&
+	    !local_softirq_pending()) {
 		if (!in_interrupt())
 			tick_nohz_irq_exit();
 	}

Re: [patch 2/4] nohz: Prevent erroneous tick stop invocations

[Thomas Gleixner]

On Wed, 27 Dec 2017, Thomas Gleixner wrote:
> On Tue, 26 Dec 2017, Frederic Weisbecker wrote:
> 
> > On Fri, Dec 22, 2017 at 03:51:13PM +0100, Thomas Gleixner wrote:
> > > 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.
> > 
> > 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?
> 
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -382,7 +382,8 @@ static inline void tick_irq_exit(void)
>  	int cpu = smp_processor_id();
>  
>  	/* Make sure that timer wheel updates are propagated */
> -	if ((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) {
> +	if (((idle_cpu(cpu) && !need_resched()) || tick_nohz_full_cpu(cpu)) &&
> +	if ((idle_cpu(cpu) || tick_nohz_full_cpu(cpu)) &&
> +	    !local_softirq_pending()) {
>  		if (!in_interrupt())
>  			tick_nohz_irq_exit();
>  	}

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.

Thanks,

	tglx

Re: [patch 2/4] nohz: Prevent erroneous tick stop invocations

[Thomas Gleixner]

On Wed, 27 Dec 2017, Thomas Gleixner wrote:
> 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 <tglx@linutronix.de>
Date: Fri, 22 Dec 2017 15:51:13 +0100

From: Thomas Gleixner <tglx@linutronix.de>

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 <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: stable@vger.kernel.org
Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
Cc: Anna-Maria Gleixner <anna-maria@linutronix.de>

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

--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -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)
 {
@@ -666,8 +671,8 @@ static ktime_t tick_nohz_stop_sched_tick
 	} while (read_seqretry(&jiffies_lock, seq));
 	ts->last_jiffies = basejiff;
 
-	if (rcu_needs_cpu(basemono, &next_rcu) ||
-	    arch_needs_cpu() || irq_work_needs_cpu()) {
+	if (rcu_needs_cpu(basemono, &next_rcu) || arch_needs_cpu() ||
+	    irq_work_needs_cpu() || local_timer_softirq_pending()) {
 		next_tick = basemono + TICK_NSEC;
 	} else {
 		/*

Re: [patch 2/4] nohz: Prevent erroneous tick stop invocations

[Frederic Weisbecker]

On Wed, Dec 27, 2017 at 09:58:08PM +0100, Thomas Gleixner wrote:
> On Wed, 27 Dec 2017, Thomas Gleixner wrote:
> > 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 <tglx@linutronix.de>
> Date: Fri, 22 Dec 2017 15:51:13 +0100
> 
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> 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 <tglx@linutronix.de>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Sebastian Siewior <bigeasy@linutronix.de>
> Cc: stable@vger.kernel.org
> Cc: Paul McKenney <paulmck@linux.vnet.ibm.com>
> Cc: Anna-Maria Gleixner <anna-maria@linutronix.de>
> 
> ---
>  kernel/time/tick-sched.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -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)
>  {
> @@ -666,8 +671,8 @@ static ktime_t tick_nohz_stop_sched_tick
>  	} while (read_seqretry(&jiffies_lock, seq));
>  	ts->last_jiffies = basejiff;
>  
> -	if (rcu_needs_cpu(basemono, &next_rcu) ||
> -	    arch_needs_cpu() || irq_work_needs_cpu()) {
> +	if (rcu_needs_cpu(basemono, &next_rcu) || arch_needs_cpu() ||
> +	    irq_work_needs_cpu() || local_timer_softirq_pending()) {

Much better. This may need a comment though because it's not immediately
obvious why we have this check while softirqs are processed just before
tick_irq_exit().

Thanks.

Acked-by: Frederic Weisbecker <frederic@kernel.org>

[patch 3/4] timer: Invoke timer_start_debug() where it makes sense

[Thomas Gleixner]

[-- Attachment #1: timer--Invoke_timer_start_debug--_where_it_makes_sense.patch --]
[-- Type: text/plain, Size: 978 bytes --]

From: Thomas Gleixner <tglx@linutronix.de>

The timer start debug function is called before the proper timer base is
set. As a consequence the trace data contains the stale CPU and flags
values.

Call the debug function after setting the new base and flags.

Fixes: 500462a9de65 ("timers: Switch to a non-cascading wheel")
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: stable@vger.kernel.org
Cc: rt@linutronix.de

---
 kernel/time/timer.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1007,8 +1007,6 @@ static inline int
 	if (!ret && (options & MOD_TIMER_PENDING_ONLY))
 		goto out_unlock;
 
-	debug_activate(timer, expires);
-
 	new_base = get_target_base(base, timer->flags);
 
 	if (base != new_base) {
@@ -1032,6 +1030,8 @@ static inline int
 		}
 	}
 
+	debug_activate(timer, expires);
+
 	timer->expires = expires;
 	/*
 	 * If 'idx' was calculated above and the base time did not advance