[patch 0/3] three cpu hotplug fixes

[patch 0/3] three cpu hotplug fixes

[Heiko Carstens]

Three fixes for some very rare to find cpu hotplug bugs.

[patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Heiko Carstens]

[-- Attachment #1: 001_printk_preempt.diff --]
[-- Type: text/plain, Size: 1118 bytes --]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

wake_up_klogd() may get called from preemtible context but uses
__raw_get_cpu_var() to write to a per cpu variable. If it gets preempted between
getting the address and writing to it, the cpu in question could be offline if
the process gets scheduled back and hence writes to the per cpu data of an offline
cpu.

No idea why that behaviour was introduced with fa33507a "printk: robustify
printk, fix #2" which was supposed to fix a "using smp_processor_id() in
preemptible" warning.

Let's use get_cpu_var() instead which disables preemption and makes sure that
the outlined scenario cannot happen.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/printk.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1087,8 +1087,10 @@ int printk_needs_cpu(int cpu)
 
 void wake_up_klogd(void)
 {
-	if (waitqueue_active(&log_wait))
-		__raw_get_cpu_var(printk_pending) = 1;
+	if (waitqueue_active(&log_wait)) {
+		get_cpu_var(printk_pending) = 1;
+		put_cpu_var(printk_pending);
+	}
 }
 
 /**

Re: [patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Peter Zijlstra]

On Fri, 2010-11-26 at 13:00 +0100, Heiko Carstens wrote:
> plain text document attachment (001_printk_preempt.diff)
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> wake_up_klogd() may get called from preemtible context but uses
> __raw_get_cpu_var() to write to a per cpu variable. If it gets preempted between
> getting the address and writing to it, the cpu in question could be offline if
> the process gets scheduled back and hence writes to the per cpu data of an offline
> cpu.
> 
> No idea why that behaviour was introduced with fa33507a "printk: robustify
> printk, fix #2" which was supposed to fix a "using smp_processor_id() in
> preemptible" warning.
> 
> Let's use get_cpu_var() instead which disables preemption and makes sure that
> the outlined scenario cannot happen.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  kernel/printk.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1087,8 +1087,10 @@ int printk_needs_cpu(int cpu)
>  
>  void wake_up_klogd(void)
>  {
> -	if (waitqueue_active(&log_wait))
> -		__raw_get_cpu_var(printk_pending) = 1;
> +	if (waitqueue_active(&log_wait)) {
> +		get_cpu_var(printk_pending) = 1;
> +		put_cpu_var(printk_pending);
> +	}
>  }
>  
>  /**
> 

But but but, the cpu can still be offlined between writing this state
and the next tick happening, right?

Re: [patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Heiko Carstens]

On Fri, Nov 26, 2010 at 01:10:08PM +0100, Peter Zijlstra wrote:
> On Fri, 2010-11-26 at 13:00 +0100, Heiko Carstens wrote:
> > plain text document attachment (001_printk_preempt.diff)
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > wake_up_klogd() may get called from preemtible context but uses
> > __raw_get_cpu_var() to write to a per cpu variable. If it gets preempted between
> > getting the address and writing to it, the cpu in question could be offline if
> > the process gets scheduled back and hence writes to the per cpu data of an offline
> > cpu.
> > 
> > No idea why that behaviour was introduced with fa33507a "printk: robustify
> > printk, fix #2" which was supposed to fix a "using smp_processor_id() in
> > preemptible" warning.
> > 
> > Let's use get_cpu_var() instead which disables preemption and makes sure that
> > the outlined scenario cannot happen.
> > 
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > ---
> >  kernel/printk.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > --- a/kernel/printk.c
> > +++ b/kernel/printk.c
> > @@ -1087,8 +1087,10 @@ int printk_needs_cpu(int cpu)
> >  
> >  void wake_up_klogd(void)
> >  {
> > -	if (waitqueue_active(&log_wait))
> > -		__raw_get_cpu_var(printk_pending) = 1;
> > +	if (waitqueue_active(&log_wait)) {
> > +		get_cpu_var(printk_pending) = 1;
> > +		put_cpu_var(printk_pending);
> > +	}
> >  }
> >  
> >  /**
> > 
> 
> But but but, the cpu can still be offlined between writing this state
> and the next tick happening, right?

Yes, that's what the second patch would fix as a side effect.

Re: [patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Peter Zijlstra]

On Fri, 2010-11-26 at 13:13 +0100, Heiko Carstens wrote:
> > But but but, the cpu can still be offlined between writing this state
> > and the next tick happening, right?
> 
> Yes, that's what the second patch would fix as a side effect. 

Ah, right, ok took them both.

Re: [patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 13:00 +0100, Heiko Carstens a écrit :
> pièce jointe document texte brut (001_printk_preempt.diff)
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> wake_up_klogd() may get called from preemtible context but uses
> __raw_get_cpu_var() to write to a per cpu variable. If it gets preempted between
> getting the address and writing to it, the cpu in question could be offline if
> the process gets scheduled back and hence writes to the per cpu data of an offline
> cpu.
> 
> No idea why that behaviour was introduced with fa33507a "printk: robustify
> printk, fix #2" which was supposed to fix a "using smp_processor_id() in
> preemptible" warning.
> 
> Let's use get_cpu_var() instead which disables preemption and makes sure that
> the outlined scenario cannot happen.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  kernel/printk.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1087,8 +1087,10 @@ int printk_needs_cpu(int cpu)
>  
>  void wake_up_klogd(void)
>  {
> -	if (waitqueue_active(&log_wait))
> -		__raw_get_cpu_var(printk_pending) = 1;
> +	if (waitqueue_active(&log_wait)) {
> +		get_cpu_var(printk_pending) = 1;
> +		put_cpu_var(printk_pending);
> +	}
>  }
>  
>  


Please use :

this_cpu_write(printk_pending, 1);

It is faster on x86, and does the right thing too.

Re: [patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Heiko Carstens]

On Fri, Nov 26, 2010 at 01:35:09PM +0100, Eric Dumazet wrote:
> Le vendredi 26 novembre 2010 à 13:00 +0100, Heiko Carstens a écrit :
> >  void wake_up_klogd(void)
> >  {
> > -	if (waitqueue_active(&log_wait))
> > -		__raw_get_cpu_var(printk_pending) = 1;
> > +	if (waitqueue_active(&log_wait)) {
> > +		get_cpu_var(printk_pending) = 1;
> > +		put_cpu_var(printk_pending);
> > +	}
> >  }
> 
> Please use :
> 
> this_cpu_write(printk_pending, 1);
> 
> It is faster on x86, and does the right thing too.

Ah, right. I wasn't aware that such a thing even exists.
Updated patch below:

Subject: [PATCH] printk: fix wake_up_klogd() vs cpu hotplug

From: Heiko Carstens <heiko.carstens@de.ibm.com>

wake_up_klogd() may get called from preemtible context but uses
__raw_get_cpu_var() to write to a per cpu variable. If it gets preempted between
getting the address and writing to it, the cpu in question could be offline if
the process gets scheduled back and hence writes to the per cpu data of an offline
cpu.

No idea why that behaviour was introduced with fa33507a "printk: robustify
printk, fix #2" which was supposed to fix a "using smp_processor_id() in
preemptible" warning.

Let's use this_cpu_write() instead which disables preemption and makes sure that
the outlined scenario cannot happen.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/printk.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1088,7 +1088,7 @@ int printk_needs_cpu(int cpu)
 void wake_up_klogd(void)
 {
 	if (waitqueue_active(&log_wait))
-		__raw_get_cpu_var(printk_pending) = 1;
+		this_cpu_write(printk_pending, 1);
 }
 
 /**

Re: [patch 1/3] printk: fix wake_up_klogd() vs cpu hotplug

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 13:42 +0100, Heiko Carstens a écrit :
> On Fri, Nov 26, 2010 at 01:35:09PM +0100, Eric Dumazet wrote:
> > Le vendredi 26 novembre 2010 à 13:00 +0100, Heiko Carstens a écrit :
> > >  void wake_up_klogd(void)
> > >  {
> > > -	if (waitqueue_active(&log_wait))
> > > -		__raw_get_cpu_var(printk_pending) = 1;
> > > +	if (waitqueue_active(&log_wait)) {
> > > +		get_cpu_var(printk_pending) = 1;
> > > +		put_cpu_var(printk_pending);
> > > +	}
> > >  }
> > 
> > Please use :
> > 
> > this_cpu_write(printk_pending, 1);
> > 
> > It is faster on x86, and does the right thing too.
> 
> Ah, right. I wasn't aware that such a thing even exists.
> Updated patch below:
> 
> Subject: [PATCH] printk: fix wake_up_klogd() vs cpu hotplug
> 
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> wake_up_klogd() may get called from preemtible context but uses
> __raw_get_cpu_var() to write to a per cpu variable. If it gets preempted between
> getting the address and writing to it, the cpu in question could be offline if
> the process gets scheduled back and hence writes to the per cpu data of an offline
> cpu.
> 
> No idea why that behaviour was introduced with fa33507a "printk: robustify
> printk, fix #2" which was supposed to fix a "using smp_processor_id() in
> preemptible" warning.
> 
> Let's use this_cpu_write() instead which disables preemption and makes sure that
> the outlined scenario cannot happen.
> 


> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  kernel/printk.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1088,7 +1088,7 @@ int printk_needs_cpu(int cpu)
>  void wake_up_klogd(void)
>  {
>  	if (waitqueue_active(&log_wait))
> -		__raw_get_cpu_var(printk_pending) = 1;
> +		this_cpu_write(printk_pending, 1);
>  }
>  
>  /**

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

[tip:sched/urgent] printk: Fix wake_up_klogd() vs cpu hotplug

[tip-bot for Heiko Carstens]

Commit-ID:  49f4138346b3cec2706adff02658fe27ceb1e46f
Gitweb:     http://git.kernel.org/tip/49f4138346b3cec2706adff02658fe27ceb1e46f
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Fri, 26 Nov 2010 13:42:47 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 26 Nov 2010 15:03:11 +0100

printk: Fix wake_up_klogd() vs cpu hotplug

wake_up_klogd() may get called from preemptible context but uses
__raw_get_cpu_var() to write to a per cpu variable. If it gets preempted
between getting the address and writing to it, the cpu in question could be
offline if the process gets scheduled back and hence writes to the per cpu data
of an offline cpu.

This buggy behaviour was introduced with fa33507a "printk: robustify
printk, fix #2" which was supposed to fix a "using smp_processor_id() in
preemptible" warning.

Let's use this_cpu_write() instead which disables preemption and makes sure
that the outlined scenario cannot happen.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20101126124247.GC7023@osiris.boeblingen.de.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/printk.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index 9a2264f..cf7588e 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1088,7 +1088,7 @@ int printk_needs_cpu(int cpu)
 void wake_up_klogd(void)
 {
 	if (waitqueue_active(&log_wait))
-		__raw_get_cpu_var(printk_pending) = 1;
+		this_cpu_write(printk_pending, 1);
 }
 
 /**

[patch 2/3] nohz: fix printk_needs_cpu() return value on offline cpus

[Heiko Carstens]

[-- Attachment #1: 002_printk_needs_cpu.diff --]
[-- Type: text/plain, Size: 2302 bytes --]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

This patch fixes a hang observed with 2.6.32 kernels where timers got
enqueued on offline cpus.

printk_needs_cpu() may return 1 if called on offline cpus. When a cpu gets
offlined it schedules the idle process which, before killing its own cpu,
will call tick_nohz_stop_sched_tick().
That function in turn will call printk_needs_cpu() in order to check if the
local tick can be disabled. On offline cpus this function should naturally
return 0 since regardless if the tick gets disabled or not the cpu will be
dead short after. That is besides the fact that __cpu_disable() should already
have made sure that no interrupts on the offlined cpu will be delivered anyway.

In this case it prevents tick_nohz_stop_sched_tick() to call
select_nohz_load_balancer(). No idea if that really is a problem. However what
made me debug this is that on 2.6.32 the function get_nohz_load_balancer() is
used within __mod_timer() to select a cpu on which a timer gets enqueued.
If printk_needs_cpu() returns 1 then the nohz_load_balancer cpu doesn't get
updated when a cpu gets offlined. It may contain the cpu number of an offline
cpu. In turn timers get enqueued on an offline cpu and not very surprisingly
they never expire and cause system hangs.

This has been observed 2.6.32 kernels. On current kernels __mod_timer() uses
get_nohz_timer_target() which doesn't have that problem. However there might
be other problems because of the too early exit tick_nohz_stop_sched_tick()
in case a cpu goes offline.

Easiest way to fix this is just to test if the current cpu is offline and
call printk_tick() directly which clears the condition.

Alternatively I tried a cpu hotplug notifier which would clear the condition,
however between calling the notifier function and printk_needs_cpu() something
could have called printk() again and the problem is back again. This seems to
be the safest fix.

Cc: stable@kernel.org
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 kernel/printk.c |    2 ++
 1 file changed, 2 insertions(+)

--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1082,6 +1082,8 @@ void printk_tick(void)
 
 int printk_needs_cpu(int cpu)
 {
+	if (unlikely(cpu_is_offline(cpu)))
+		printk_tick();
 	return per_cpu(printk_pending, cpu);
 }
 

Re: [patch 2/3] nohz: fix printk_needs_cpu() return value on offline cpus

[Peter Zijlstra]

On Fri, 2010-11-26 at 13:00 +0100, Heiko Carstens wrote:
> plain text document attachment (002_printk_needs_cpu.diff)
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> This patch fixes a hang observed with 2.6.32 kernels where timers got
> enqueued on offline cpus.
> 
> printk_needs_cpu() may return 1 if called on offline cpus. When a cpu gets
> offlined it schedules the idle process which, before killing its own cpu,
> will call tick_nohz_stop_sched_tick().
> That function in turn will call printk_needs_cpu() in order to check if the
> local tick can be disabled. On offline cpus this function should naturally
> return 0 since regardless if the tick gets disabled or not the cpu will be
> dead short after. That is besides the fact that __cpu_disable() should already
> have made sure that no interrupts on the offlined cpu will be delivered anyway.
> 
> In this case it prevents tick_nohz_stop_sched_tick() to call
> select_nohz_load_balancer(). No idea if that really is a problem. However what
> made me debug this is that on 2.6.32 the function get_nohz_load_balancer() is
> used within __mod_timer() to select a cpu on which a timer gets enqueued.
> If printk_needs_cpu() returns 1 then the nohz_load_balancer cpu doesn't get
> updated when a cpu gets offlined. It may contain the cpu number of an offline
> cpu. In turn timers get enqueued on an offline cpu and not very surprisingly
> they never expire and cause system hangs.
> 
> This has been observed 2.6.32 kernels. On current kernels __mod_timer() uses
> get_nohz_timer_target() which doesn't have that problem. However there might
> be other problems because of the too early exit tick_nohz_stop_sched_tick()
> in case a cpu goes offline.
> 
> Easiest way to fix this is just to test if the current cpu is offline and
> call printk_tick() directly which clears the condition.
> 
> Alternatively I tried a cpu hotplug notifier which would clear the condition,
> however between calling the notifier function and printk_needs_cpu() something
> could have called printk() again and the problem is back again. This seems to
> be the safest fix.
> 
> Cc: stable@kernel.org
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
>  kernel/printk.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1082,6 +1082,8 @@ void printk_tick(void)
>  
>  int printk_needs_cpu(int cpu)
>  {
> +	if (unlikely(cpu_is_offline(cpu)))
> +		printk_tick();
>  	return per_cpu(printk_pending, cpu);
>  }
>  

Nice,.. applied.

Re: [stable] [patch 2/3] nohz: fix printk_needs_cpu() return value on offline cpus

[Greg KH]

On Fri, Nov 26, 2010 at 01:11:52PM +0100, Peter Zijlstra wrote:
> On Fri, 2010-11-26 at 13:00 +0100, Heiko Carstens wrote:
> > plain text document attachment (002_printk_needs_cpu.diff)
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > This patch fixes a hang observed with 2.6.32 kernels where timers got
> > enqueued on offline cpus.
> > 
> > printk_needs_cpu() may return 1 if called on offline cpus. When a cpu gets
> > offlined it schedules the idle process which, before killing its own cpu,
> > will call tick_nohz_stop_sched_tick().
> > That function in turn will call printk_needs_cpu() in order to check if the
> > local tick can be disabled. On offline cpus this function should naturally
> > return 0 since regardless if the tick gets disabled or not the cpu will be
> > dead short after. That is besides the fact that __cpu_disable() should already
> > have made sure that no interrupts on the offlined cpu will be delivered anyway.
> > 
> > In this case it prevents tick_nohz_stop_sched_tick() to call
> > select_nohz_load_balancer(). No idea if that really is a problem. However what
> > made me debug this is that on 2.6.32 the function get_nohz_load_balancer() is
> > used within __mod_timer() to select a cpu on which a timer gets enqueued.
> > If printk_needs_cpu() returns 1 then the nohz_load_balancer cpu doesn't get
> > updated when a cpu gets offlined. It may contain the cpu number of an offline
> > cpu. In turn timers get enqueued on an offline cpu and not very surprisingly
> > they never expire and cause system hangs.
> > 
> > This has been observed 2.6.32 kernels. On current kernels __mod_timer() uses
> > get_nohz_timer_target() which doesn't have that problem. However there might
> > be other problems because of the too early exit tick_nohz_stop_sched_tick()
> > in case a cpu goes offline.
> > 
> > Easiest way to fix this is just to test if the current cpu is offline and
> > call printk_tick() directly which clears the condition.
> > 
> > Alternatively I tried a cpu hotplug notifier which would clear the condition,
> > however between calling the notifier function and printk_needs_cpu() something
> > could have called printk() again and the problem is back again. This seems to
> > be the safest fix.
> > 
> > Cc: stable@kernel.org
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> > ---
> >  kernel/printk.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > --- a/kernel/printk.c
> > +++ b/kernel/printk.c
> > @@ -1082,6 +1082,8 @@ void printk_tick(void)
> >  
> >  int printk_needs_cpu(int cpu)
> >  {
> > +	if (unlikely(cpu_is_offline(cpu)))
> > +		printk_tick();
> >  	return per_cpu(printk_pending, cpu);
> >  }
> >  
> 
> Nice,.. applied.

Is this going to make it into .37, or is it going to wait until .38?

thanks,

greg k-h

Re: [stable] [patch 2/3] nohz: fix printk_needs_cpu() return value on offline cpus

[Heiko Carstens]

On Tue, Dec 07, 2010 at 01:32:41PM -0800, Greg KH wrote:
> On Fri, Nov 26, 2010 at 01:11:52PM +0100, Peter Zijlstra wrote:
> > On Fri, 2010-11-26 at 13:00 +0100, Heiko Carstens wrote:
> > > plain text document attachment (002_printk_needs_cpu.diff)
> > > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > > 
> > > This patch fixes a hang observed with 2.6.32 kernels where timers got
> > > enqueued on offline cpus.
[...]
> 
> Is this going to make it into .37, or is it going to wait until .38?

I hope it will go into .37. The same should be true for the
get_next_timer_interrupt() which addresses the same problem.
Peter, Ingo?

Re: [stable] [patch 2/3] nohz: fix printk_needs_cpu() return value on offline cpus

[Peter Zijlstra]

On Wed, 2010-12-08 at 09:07 +0100, Heiko Carstens wrote:
> On Tue, Dec 07, 2010 at 01:32:41PM -0800, Greg KH wrote:
> > On Fri, Nov 26, 2010 at 01:11:52PM +0100, Peter Zijlstra wrote:
> > > On Fri, 2010-11-26 at 13:00 +0100, Heiko Carstens wrote:
> > > > plain text document attachment (002_printk_needs_cpu.diff)
> > > > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > > > 
> > > > This patch fixes a hang observed with 2.6.32 kernels where timers got
> > > > enqueued on offline cpus.
> [...]
> > 
> > Is this going to make it into .37, or is it going to wait until .38?
> 
> I hope it will go into .37. The same should be true for the
> get_next_timer_interrupt() which addresses the same problem.
> Peter, Ingo?

Ingo just send the pull request to Linus for two of the three patches,
I'll try and feed him the last one today.

[tip:sched/urgent] nohz: Fix printk_needs_cpu() return value on offline cpus

[tip-bot for Heiko Carstens]

Commit-ID:  61ab25447ad6334a74e32f60efb135a3467223f8
Gitweb:     http://git.kernel.org/tip/61ab25447ad6334a74e32f60efb135a3467223f8
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Fri, 26 Nov 2010 13:00:59 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Fri, 26 Nov 2010 15:03:12 +0100

nohz: Fix printk_needs_cpu() return value on offline cpus

This patch fixes a hang observed with 2.6.32 kernels where timers got enqueued
on offline cpus.

printk_needs_cpu() may return 1 if called on offline cpus. When a cpu gets
offlined it schedules the idle process which, before killing its own cpu, will
call tick_nohz_stop_sched_tick(). That function in turn will call
printk_needs_cpu() in order to check if the local tick can be disabled. On
offline cpus this function should naturally return 0 since regardless if the
tick gets disabled or not the cpu will be dead short after. That is besides the
fact that __cpu_disable() should already have made sure that no interrupts on
the offlined cpu will be delivered anyway.

In this case it prevents tick_nohz_stop_sched_tick() to call
select_nohz_load_balancer(). No idea if that really is a problem. However what
made me debug this is that on 2.6.32 the function get_nohz_load_balancer() is
used within __mod_timer() to select a cpu on which a timer gets enqueued. If
printk_needs_cpu() returns 1 then the nohz_load_balancer cpu doesn't get
updated when a cpu gets offlined. It may contain the cpu number of an offline
cpu. In turn timers get enqueued on an offline cpu and not very surprisingly
they never expire and cause system hangs.

This has been observed 2.6.32 kernels. On current kernels __mod_timer() uses
get_nohz_timer_target() which doesn't have that problem. However there might be
other problems because of the too early exit tick_nohz_stop_sched_tick() in
case a cpu goes offline.

Easiest way to fix this is just to test if the current cpu is offline and call
printk_tick() directly which clears the condition.

Alternatively I tried a cpu hotplug notifier which would clear the condition,
however between calling the notifier function and printk_needs_cpu() something
could have called printk() again and the problem is back again. This seems to
be the safest fix.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: stable@kernel.org
LKML-Reference: <20101126120235.406766476@de.ibm.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/printk.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index cf7588e..a23315d 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1082,6 +1082,8 @@ void printk_tick(void)
 
 int printk_needs_cpu(int cpu)
 {
+	if (unlikely(cpu_is_offline(cpu)))
+		printk_tick();
 	return per_cpu(printk_pending, cpu);
 }
 

[PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 15:02 +0000, tip-bot for Heiko Carstens a
écrit :
> Commit-ID:  61ab25447ad6334a74e32f60efb135a3467223f8
> Gitweb:     http://git.kernel.org/tip/61ab25447ad6334a74e32f60efb135a3467223f8
> Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
> AuthorDate: Fri, 26 Nov 2010 13:00:59 +0100
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Fri, 26 Nov 2010 15:03:12 +0100
> 

...

> diff --git a/kernel/printk.c b/kernel/printk.c
> index cf7588e..a23315d 100644
> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1082,6 +1082,8 @@ void printk_tick(void)
>  
>  int printk_needs_cpu(int cpu)
>  {
> +	if (unlikely(cpu_is_offline(cpu)))
> +		printk_tick();
>  	return per_cpu(printk_pending, cpu);
>  }
>  

The unlikely() was not needed, since we already have :
#define cpu_is_offline(cpu) unlikely(!cpu_online(cpu))

Anyway, this patch implies 'cpu' is always the current cpu.

If so, we can have a followup patch to use modern this_cpu api.

Thanks

[PATCH] printk: use this_cpu_{read|write} api on printk_pending

__get_cpu_var() is a bit inefficient, lets use __this_cpu_read() and
__this_cpu_write() to manipulate printk_pending.

printk_needs_cpu(cpu) is called only for the current cpu :
Use faster __this_cpu_read().

remove the redondant unlikely on (cpu_is_offline(cpu)) test

# size kernel/printk.o*
   text	   data	    bss	    dec	    hex	filename
   9942	    756	 263488	 274186	  42f0a	kernel/printk.o.new
   9990	    756	 263488	 274234	  42f3a	kernel/printk.o.old

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Christoph Lameter <cl@linux.com>
---
 kernel/printk.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index a23315d..ab3ffc5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1074,17 +1074,17 @@ static DEFINE_PER_CPU(int, printk_pending);
 
 void printk_tick(void)
 {
-	if (__get_cpu_var(printk_pending)) {
-		__get_cpu_var(printk_pending) = 0;
+	if (__this_cpu_read(printk_pending)) {
+		__this_cpu_write(printk_pending, 0);
 		wake_up_interruptible(&log_wait);
 	}
 }
 
 int printk_needs_cpu(int cpu)
 {
-	if (unlikely(cpu_is_offline(cpu)))
+	if (cpu_is_offline(cpu))
 		printk_tick();
-	return per_cpu(printk_pending, cpu);
+	return __this_cpu_read(printk_pending);
 }
 
 void wake_up_klogd(void)

Re: [PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Peter Zijlstra]

On Fri, 2010-11-26 at 17:22 +0100, Eric Dumazet wrote:
> [PATCH] printk: use this_cpu_{read|write} api on printk_pending
> 
> __get_cpu_var() is a bit inefficient, lets use __this_cpu_read() and
> __this_cpu_write() to manipulate printk_pending.
> 
> printk_needs_cpu(cpu) is called only for the current cpu :
> Use faster __this_cpu_read().
> 
> remove the redondant unlikely on (cpu_is_offline(cpu)) test
> 
> # size kernel/printk.o*
>    text    data     bss     dec     hex filename
>    9942     756  263488  274186   42f0a kernel/printk.o.new
>    9990     756  263488  274234   42f3a kernel/printk.o.old


Thanks, queued!

Re: [PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Christoph Lameter]

On Fri, 26 Nov 2010, Eric Dumazet wrote:

> --- a/kernel/printk.c
> +++ b/kernel/printk.c
> @@ -1074,17 +1074,17 @@ static DEFINE_PER_CPU(int, printk_pending);
>
>  void printk_tick(void)
>  {
> -	if (__get_cpu_var(printk_pending)) {
> -		__get_cpu_var(printk_pending) = 0;
> +	if (__this_cpu_read(printk_pending)) {
> +		__this_cpu_write(printk_pending, 0);
>  		wake_up_interruptible(&log_wait);
>  	}
>  }

The above looks ok.

>  int printk_needs_cpu(int cpu)
>  {
> -	if (unlikely(cpu_is_offline(cpu)))
> +	if (cpu_is_offline(cpu))
>  		printk_tick();
> -	return per_cpu(printk_pending, cpu);
> +	return __this_cpu_read(printk_pending);
>  }

This only works if printk_needs_cpu is always passed the current cpu. The
check for cpu_is_offline suggests that the function is also used for other
cpus. The __this_cpu_read() will then get the printk_pending stat for the
current cpu and not for the specified cpu.

Re: [PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 10:40 -0600, Christoph Lameter a écrit :

> This only works if printk_needs_cpu is always passed the current cpu. The
> check for cpu_is_offline suggests that the function is also used for other
> cpus. The __this_cpu_read() will then get the printk_pending stat for the
> current cpu and not for the specified cpu.
> 
> 

I guess you read the changelog too fast ;)

I posted this patch after noticing printk_needs_cpu() was always used
for the current cpu.

We have other functions around, that always work for the current cpu.

We pass cpu as a parameter, mostly because smp_processor_id() was a bit
expensive in old kernels, and is still expensive because of sanity
tests.

Now we have this_cpu_ api, we probably can do a cleanup to avoid now
useless parameter passing.

If we had this_cpu_is_online() function, we could avoid (int cpu) param
in printk_needs_cpu()

Re: [PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Christoph Lameter]

On Fri, 26 Nov 2010, Eric Dumazet wrote:

> I guess you read the changelog too fast ;)

No I only read the code.... ;-)

> I posted this patch after noticing printk_needs_cpu() was always used
> for the current cpu.

Ok.

> We have other functions around, that always work for the current cpu.

Ok.

> We pass cpu as a parameter, mostly because smp_processor_id() was a bit
> expensive in old kernels, and is still expensive because of sanity
> tests.

I thought sanity tests do not apply for performance testing scenarios?

smp_processor_id() is usually a simple per cpu data access and very fast.

Re: [PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Eric Dumazet]

Le vendredi 26 novembre 2010 à 11:11 -0600, Christoph Lameter a écrit :

> I thought sanity tests do not apply for performance testing scenarios?
> 

CONFIG_DEBUG_PREEMPT

> smp_processor_id() is usually a simple per cpu data access and very fast.
> 

Sure, but when its already in a register, its better to use the
register ;)

Re: [PATCH] printk: use this_cpu_{read|write} api on printk_pending

[Christoph Lameter]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 595 bytes --]

On Fri, 26 Nov 2010, Eric Dumazet wrote:

> Le vendredi 26 novembre 2010 à 11:11 -0600, Christoph Lameter a écrit :
>
> > I thought sanity tests do not apply for performance testing scenarios?
> >
>
> CONFIG_DEBUG_PREEMPT

You really enable that option for performance testing?

> > smp_processor_id() is usually a simple per cpu data access and very fast.
> >
>
> Sure, but when its already in a register, its better to use the
> register ;)

Well that increases register pressure in the caller. There may be no need
to handle the current processor number in the caller.

[tip:sched/core] printk: Use this_cpu_{read|write} api on printk_pending

[tip-bot for Eric Dumazet]

Commit-ID:  40dc11ffb35e8c4e8fa71092048e0f8de9db758c
Gitweb:     http://git.kernel.org/tip/40dc11ffb35e8c4e8fa71092048e0f8de9db758c
Author:     Eric Dumazet <eric.dumazet@gmail.com>
AuthorDate: Fri, 26 Nov 2010 17:22:16 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 8 Dec 2010 20:16:01 +0100

printk: Use this_cpu_{read|write} api on printk_pending

__get_cpu_var() is a bit inefficient, lets use __this_cpu_read() and
__this_cpu_write() to manipulate printk_pending.

printk_needs_cpu(cpu) is called only for the current cpu :
Use faster __this_cpu_read().

Remove the redundant unlikely on (cpu_is_offline(cpu)) test:

 # size kernel/printk.o*
   text	   data	    bss	    dec	    hex	filename
   9942	    756	 263488	 274186	  42f0a	kernel/printk.o.new
   9990	    756	 263488	 274234	  42f3a	kernel/printk.o.old

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Christoph Lameter <cl@linux.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <1290788536.2855.237.camel@edumazet-laptop>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/printk.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/kernel/printk.c b/kernel/printk.c
index a23315d..ab3ffc5 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1074,17 +1074,17 @@ static DEFINE_PER_CPU(int, printk_pending);
 
 void printk_tick(void)
 {
-	if (__get_cpu_var(printk_pending)) {
-		__get_cpu_var(printk_pending) = 0;
+	if (__this_cpu_read(printk_pending)) {
+		__this_cpu_write(printk_pending, 0);
 		wake_up_interruptible(&log_wait);
 	}
 }
 
 int printk_needs_cpu(int cpu)
 {
-	if (unlikely(cpu_is_offline(cpu)))
+	if (cpu_is_offline(cpu))
 		printk_tick();
-	return per_cpu(printk_pending, cpu);
+	return __this_cpu_read(printk_pending);
 }
 
 void wake_up_klogd(void)

Re: [tip:sched/core] printk: Use this_cpu_{read|write} api on printk_pending

[Christoph Lameter]

On Wed, 8 Dec 2010, tip-bot for Eric Dumazet wrote:

> printk: Use this_cpu_{read|write} api on printk_pending

One of the patches in Tejun's percpu tree includes the same modifications.

Re: [tip:sched/core] printk: Use this_cpu_{read|write} api on printk_pending

[Eric Dumazet]

Le mercredi 08 décembre 2010 à 15:47 -0600, Christoph Lameter a écrit :
> On Wed, 8 Dec 2010, tip-bot for Eric Dumazet wrote:
> 
> > printk: Use this_cpu_{read|write} api on printk_pending
> 
> One of the patches in Tejun's percpu tree includes the same modifications.
> 
> 

Its a bit strange, I sent mine a long time ago when reviewing Heiko
Carstens patch. You even commented on it.

https://patchwork.kernel.org/patch/359182/

What happened then, you included my patch on one of yours ?

Re: [tip:sched/core] printk: Use this_cpu_{read|write} api on printk_pending

[Christoph Lameter]

On Thu, 9 Dec 2010, Eric Dumazet wrote:

> Its a bit strange, I sent mine a long time ago when reviewing Heiko
> Carstens patch. You even commented on it.
>
> https://patchwork.kernel.org/patch/359182/
>
> What happened then, you included my patch on one of yours ?

I just went through the tree and changed all __get_cpu_var. Out came a
single core patch that also covered this earlier case.

[patch 3/3] nohz/s390: fix arch_needs_cpu() return value on offline cpus

[Heiko Carstens]

[-- Attachment #1: 003_arch_needs_cpu.diff --]
[-- Type: text/plain, Size: 1766 bytes --]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

This fixes the same problem as described in the patch "nohz: fix
printk_needs_cpu() return value on offline cpus" for the arch_needs_cpu()
primitive.
This specific bug was indrocuded with 3c5d92a0 "nohz: Introduce arch_needs_cpu".

In this case a cpu hotplug notifier is used to fix the issue in order to keep
the normal/fast path small. All we need to do is to clear the condition that
makes arch_needs_cpu() return 1 since it is just a performance improvement which
is supposed to keep the local tick running for a short period if a cpu goes
idle. Nothing special needs to be done except for clearing the condition.

Cc: stable@kernel.org
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/kernel/vtime.c |   19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

--- a/arch/s390/kernel/vtime.c
+++ b/arch/s390/kernel/vtime.c
@@ -19,6 +19,7 @@
 #include <linux/kernel_stat.h>
 #include <linux/rcupdate.h>
 #include <linux/posix-timers.h>
+#include <linux/cpu.h>
 
 #include <asm/s390_ext.h>
 #include <asm/timer.h>
@@ -566,6 +567,23 @@ void init_cpu_vtimer(void)
 	__ctl_set_bit(0,10);
 }
 
+static int __cpuinit s390_nohz_notify(struct notifier_block *self,
+				      unsigned long action, void *hcpu)
+{
+	struct s390_idle_data *idle;
+	long cpu = (long) hcpu;
+
+	idle = &per_cpu(s390_idle, cpu);
+	switch (action) {
+	case CPU_DYING:
+	case CPU_DYING_FROZEN:
+		idle->nohz_delay = 0;
+	default:
+		break;
+	}
+	return NOTIFY_OK;
+}
+
 void __init vtime_init(void)
 {
 	/* request the cpu timer external interrupt */
@@ -574,5 +592,6 @@ void __init vtime_init(void)
 
 	/* Enable cpu timer interrupts on the boot cpu. */
 	init_cpu_vtimer();
+	cpu_notifier(s390_nohz_notify, 0);
 }
 

Re: [patch 3/3] nohz/s390: fix arch_needs_cpu() return value on offline cpus

[Peter Zijlstra]

On Fri, 2010-11-26 at 13:01 +0100, Heiko Carstens wrote:
> plain text document attachment (003_arch_needs_cpu.diff)
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> This fixes the same problem as described in the patch "nohz: fix
> printk_needs_cpu() return value on offline cpus" for the arch_needs_cpu()
> primitive.
> This specific bug was indrocuded with 3c5d92a0 "nohz: Introduce arch_needs_cpu".
> 
> In this case a cpu hotplug notifier is used to fix the issue in order to keep
> the normal/fast path small. All we need to do is to clear the condition that
> makes arch_needs_cpu() return 1 since it is just a performance improvement which
> is supposed to keep the local tick running for a short period if a cpu goes
> idle. Nothing special needs to be done except for clearing the condition.
> 
> Cc: stable@kernel.org
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

OK, and s390 seems to be the only architecture making use of that
interface.

Did you audit all the other *_needs_cpu() interfaces for this same
problem?

Anyway, 

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

Will you take this through the s390?

> ---
>  arch/s390/kernel/vtime.c |   19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> --- a/arch/s390/kernel/vtime.c
> +++ b/arch/s390/kernel/vtime.c
> @@ -19,6 +19,7 @@
>  #include <linux/kernel_stat.h>
>  #include <linux/rcupdate.h>
>  #include <linux/posix-timers.h>
> +#include <linux/cpu.h>
>  
>  #include <asm/s390_ext.h>
>  #include <asm/timer.h>
> @@ -566,6 +567,23 @@ void init_cpu_vtimer(void)
>  	__ctl_set_bit(0,10);
>  }
>  
> +static int __cpuinit s390_nohz_notify(struct notifier_block *self,
> +				      unsigned long action, void *hcpu)
> +{
> +	struct s390_idle_data *idle;
> +	long cpu = (long) hcpu;
> +
> +	idle = &per_cpu(s390_idle, cpu);
> +	switch (action) {
> +	case CPU_DYING:
> +	case CPU_DYING_FROZEN:
> +		idle->nohz_delay = 0;
> +	default:
> +		break;
> +	}
> +	return NOTIFY_OK;
> +}
> +
>  void __init vtime_init(void)
>  {
>  	/* request the cpu timer external interrupt */
> @@ -574,5 +592,6 @@ void __init vtime_init(void)
>  
>  	/* Enable cpu timer interrupts on the boot cpu. */
>  	init_cpu_vtimer();
> +	cpu_notifier(s390_nohz_notify, 0);
>  }
>  
> 

Re: [patch 3/3] nohz/s390: fix arch_needs_cpu() return value on offline cpus

[Heiko Carstens]

On Fri, Nov 26, 2010 at 01:14:07PM +0100, Peter Zijlstra wrote:
> On Fri, 2010-11-26 at 13:01 +0100, Heiko Carstens wrote:
> > plain text document attachment (003_arch_needs_cpu.diff)
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > This fixes the same problem as described in the patch "nohz: fix
> > printk_needs_cpu() return value on offline cpus" for the arch_needs_cpu()
> > primitive.
> > This specific bug was indrocuded with 3c5d92a0 "nohz: Introduce arch_needs_cpu".
> > 
> > In this case a cpu hotplug notifier is used to fix the issue in order to keep
> > the normal/fast path small. All we need to do is to clear the condition that
> > makes arch_needs_cpu() return 1 since it is just a performance improvement which
> > is supposed to keep the local tick running for a short period if a cpu goes
> > idle. Nothing special needs to be done except for clearing the condition.
> > 
> > Cc: stable@kernel.org
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> OK, and s390 seems to be the only architecture making use of that
> interface.
> 
> Did you audit all the other *_needs_cpu() interfaces for this same
> problem?

There's only rcu_needs_cpu() left and rcu indeed has a cpu hotplug notifier.
So there _shouldn't_ be any problems with that.

> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> 
> Will you take this through the s390?

Yes. Thanks!

Re: [patch 3/3] nohz/s390: fix arch_needs_cpu() return value on offline cpus

[Heiko Carstens]

On Fri, Nov 26, 2010 at 01:14:07PM +0100, Peter Zijlstra wrote:
> On Fri, 2010-11-26 at 13:01 +0100, Heiko Carstens wrote:
> > plain text document attachment (003_arch_needs_cpu.diff)
> > From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > 
> > This fixes the same problem as described in the patch "nohz: fix
> > printk_needs_cpu() return value on offline cpus" for the arch_needs_cpu()
> > primitive.
> > This specific bug was indrocuded with 3c5d92a0 "nohz: Introduce arch_needs_cpu".
> > 
> > In this case a cpu hotplug notifier is used to fix the issue in order to keep
> > the normal/fast path small. All we need to do is to clear the condition that
> > makes arch_needs_cpu() return 1 since it is just a performance improvement which
> > is supposed to keep the local tick running for a short period if a cpu goes
> > idle. Nothing special needs to be done except for clearing the condition.
> > 
> > Cc: stable@kernel.org
> > Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> OK, and s390 seems to be the only architecture making use of that
> interface.
> 
> Did you audit all the other *_needs_cpu() interfaces for this same
> problem?

Yes I did, but guess what.. get_next_timer_interrupt() bites us as well *sigh*.

Here is yet another nohz vs cpu hotplug patch which should also go into
stable.


Subject: [PATCH] nohz: fix get_next_timer_interrupt() vs cpu hotplug

From: Heiko Carstens <heiko.carstens@de.ibm.com>

This fixes a bug as seen on 2.6.32 based kernels where timers got enqueued
on offline cpus.

If a cpu goes offline it might still have pending timers. These will be
migrated during CPU_DEAD handling after the cpu is offline.
However while the cpu is going offline it will schedule the idle task
which will then call tick_nohz_stop_sched_tick().
That function in turn will call get_next_timer_intterupt() to figure out
if the tick of the cpu can be stopped or not. If it turns out that the
next tick is just one jiffy off (delta_jiffies == 1)
tick_nohz_stop_sched_tick() incorrectly assumes that the tick should not
stop and takes an early exit and thus it won't update the load balancer
cpu.
Just afterwards the cpu will be killed and the load balancer cpu could
be the offline cpu.
On 2.6.32 based kernel get_nohz_load_balancer() gets called to decide on
which cpu a timer should be enqueued (see __mod_timer()). Which leads
to the possibility that timers get enqueued on an offline cpu. These will
never expire and can cause a system hang.

This has been observed 2.6.32 kernels. On current kernels __mod_timer() uses
get_nohz_timer_target() which doesn't have that problem. However there might
be other problems because of the too early exit tick_nohz_stop_sched_tick()
in case a cpu goes offline.

The easiest and probably safest fix seems to be to let
get_next_timer_interrupt() just lie and let it say there isn't any pending
timer if the current cpu is offline.
I also thought of moving migrate_[hr]timers() from CPU_DEAD to CPU_DYING,
but seeing that there already have been fixes at least in the hrtimer code
in this area I'm afraid that this could add new subtle bugs.

Cc: stable@kernel.org
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

 kernel/timer.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/kernel/timer.c b/kernel/timer.c
index 68a9ae7..f95277f 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1252,6 +1252,12 @@ unsigned long get_next_timer_interrupt(unsigned long now)
 	struct tvec_base *base = __get_cpu_var(tvec_bases);
 	unsigned long expires;
 
+	/*
+	 * Pretend that there is no timer pending if the cpu is offline.
+	 * Possible pending timers will be migrated later to an active cpu.
+	 */
+	if (cpu_is_offline(smp_processor_id()))
+		return now + NEXT_TIMER_MAX_DELTA;
 	spin_lock(&base->lock);
 	if (time_before_eq(base->next_timer, base->timer_jiffies))
 		base->next_timer = __next_timer_interrupt(base);

Re: [patch 3/3] nohz/s390: fix arch_needs_cpu() return value on offline cpus

[Peter Zijlstra]

On Wed, 2010-12-01 at 10:11 +0100, Heiko Carstens wrote:
> 
> Subject: [PATCH] nohz: fix get_next_timer_interrupt() vs cpu hotplug
> 
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> This fixes a bug as seen on 2.6.32 based kernels where timers got enqueued
> on offline cpus.
> 
> If a cpu goes offline it might still have pending timers. These will be
> migrated during CPU_DEAD handling after the cpu is offline.
> However while the cpu is going offline it will schedule the idle task
> which will then call tick_nohz_stop_sched_tick().
> That function in turn will call get_next_timer_intterupt() to figure out
> if the tick of the cpu can be stopped or not. If it turns out that the
> next tick is just one jiffy off (delta_jiffies == 1)
> tick_nohz_stop_sched_tick() incorrectly assumes that the tick should not
> stop and takes an early exit and thus it won't update the load balancer
> cpu.
> Just afterwards the cpu will be killed and the load balancer cpu could
> be the offline cpu.
> On 2.6.32 based kernel get_nohz_load_balancer() gets called to decide on
> which cpu a timer should be enqueued (see __mod_timer()). Which leads
> to the possibility that timers get enqueued on an offline cpu. These will
> never expire and can cause a system hang.
> 
> This has been observed 2.6.32 kernels. On current kernels __mod_timer() uses
> get_nohz_timer_target() which doesn't have that problem. However there might
> be other problems because of the too early exit tick_nohz_stop_sched_tick()
> in case a cpu goes offline.
> 
> The easiest and probably safest fix seems to be to let
> get_next_timer_interrupt() just lie and let it say there isn't any pending
> timer if the current cpu is offline.
> I also thought of moving migrate_[hr]timers() from CPU_DEAD to CPU_DYING,
> but seeing that there already have been fixes at least in the hrtimer code
> in this area I'm afraid that this could add new subtle bugs.
> 
> Cc: stable@kernel.org
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> --- 

Thanks Heiko, I queued this one as well.

[tip:sched/urgent] nohz: Fix get_next_timer_interrupt() vs cpu hotplug

[tip-bot for Heiko Carstens]

Commit-ID:  dbd87b5af055a0cc9bba17795c9a2b0d17795389
Gitweb:     http://git.kernel.org/tip/dbd87b5af055a0cc9bba17795c9a2b0d17795389
Author:     Heiko Carstens <heiko.carstens@de.ibm.com>
AuthorDate: Wed, 1 Dec 2010 10:11:09 +0100
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 8 Dec 2010 20:15:07 +0100

nohz: Fix get_next_timer_interrupt() vs cpu hotplug

This fixes a bug as seen on 2.6.32 based kernels where timers got
enqueued on offline cpus.

If a cpu goes offline it might still have pending timers. These will
be migrated during CPU_DEAD handling after the cpu is offline.
However while the cpu is going offline it will schedule the idle task
which will then call tick_nohz_stop_sched_tick().

That function in turn will call get_next_timer_intterupt() to figure
out if the tick of the cpu can be stopped or not. If it turns out that
the next tick is just one jiffy off (delta_jiffies == 1)
tick_nohz_stop_sched_tick() incorrectly assumes that the tick should
not stop and takes an early exit and thus it won't update the load
balancer cpu.

Just afterwards the cpu will be killed and the load balancer cpu could
be the offline cpu.

On 2.6.32 based kernel get_nohz_load_balancer() gets called to decide
on which cpu a timer should be enqueued (see __mod_timer()). Which
leads to the possibility that timers get enqueued on an offline cpu.
These will never expire and can cause a system hang.

This has been observed 2.6.32 kernels. On current kernels
__mod_timer() uses get_nohz_timer_target() which doesn't have that
problem. However there might be other problems because of the too
early exit tick_nohz_stop_sched_tick() in case a cpu goes offline.

The easiest and probably safest fix seems to be to let
get_next_timer_interrupt() just lie and let it say there isn't any
pending timer if the current cpu is offline.

I also thought of moving migrate_[hr]timers() from CPU_DEAD to
CPU_DYING, but seeing that there already have been fixes at least in
the hrtimer code in this area I'm afraid that this could add new
subtle bugs.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <20101201091109.GA8984@osiris.boeblingen.de.ibm.com>
Cc: stable@kernel.org
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 kernel/timer.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index 7bd715f..353b922 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1252,6 +1252,12 @@ unsigned long get_next_timer_interrupt(unsigned long now)
 	struct tvec_base *base = __get_cpu_var(tvec_bases);
 	unsigned long expires;
 
+	/*
+	 * Pretend that there is no timer pending if the cpu is offline.
+	 * Possible pending timers will be migrated later to an active cpu.
+	 */
+	if (cpu_is_offline(smp_processor_id()))
+		return now + NEXT_TIMER_MAX_DELTA;
 	spin_lock(&base->lock);
 	if (time_before_eq(base->next_timer, base->timer_jiffies))
 		base->next_timer = __next_timer_interrupt(base);