ARM: CPU hotplug: Delegate complete() to surviving CPU

Commit Message

Paul E. McKenney Dec. 12, 2017, 5:20 p.m. UTC

The ARM implementation of arch_cpu_idle_dead() invokes complete(), but
does so after RCU has stopped watching the outgoing CPU, which results
in lockdep complaints because complete() invokes functions containing RCU
readers.  This patch therefore uses Thomas Gleixner's trick of delegating
the complete() call to a surviving CPU via smp_call_function_single().

Reported-by: Peng Fan <[e-mail]>
Reported-by: Russell King - ARM Linux <[e-mail]>
Signed-off-by: Paul E. McKenney <[e-mail]>
Tested-by: Tested-by: Fabio Estevam <[e-mail]>
Cc: Russell King <[e-mail]>
Cc: Ingo Molnar <[e-mail]>
Cc: "Peter Zijlstra (Intel)" <[e-mail]>
Cc: Michal Hocko <[e-mail]>
Cc: Thomas Gleixner <[e-mail]>
Cc: <[e-mail]>

Comments

Russell King (Oracle) Dec. 12, 2017, 5:37 p.m. UTC | #1

On Tue, Dec 12, 2017 at 09:20:59AM -0800, Paul E. McKenney wrote:
> The ARM implementation of arch_cpu_idle_dead() invokes complete(), but
> does so after RCU has stopped watching the outgoing CPU, which results
> in lockdep complaints because complete() invokes functions containing RCU
> readers.  This patch therefore uses Thomas Gleixner's trick of delegating
> the complete() call to a surviving CPU via smp_call_function_single().
> 
> Reported-by: Peng Fan <[e-mail]>
> Reported-by: Russell King - ARM Linux <[e-mail]>
> Signed-off-by: Paul E. McKenney <[e-mail]>
> Tested-by: Tested-by: Fabio Estevam <[e-mail]>
> Cc: Russell King <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: "Peter Zijlstra (Intel)" <[e-mail]>
> Cc: Michal Hocko <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: <[e-mail]>

As I just described in response to Fabio's testing, this doesn't solve
anything if CONFIG_BL_SWITCHER is enabled.  We could lose the unlock of
a spinlock in the GIC code for sending the IPI.  As I already said
previously in our discussion (but I guess you just don't believe me):

"2. there's some optional locking in the GIC driver that cause problems
   for the cpu dying path.

The concensus last time around was that the IPI solution is a non-
starter, so the seven year proven-reliable solution (disregarding the
RCU warning) persists because I don't think anyone came up with a
better solution."

Using smp_call_function_single() invokes the IPI paths.

Baruch Siach Dec. 12, 2017, 5:40 p.m. UTC | #2

Hi Paul,

On Tue, Dec 12, 2017 at 09:20:59AM -0800, Paul E. McKenney wrote:
> The ARM implementation of arch_cpu_idle_dead() invokes complete(), but
> does so after RCU has stopped watching the outgoing CPU, which results
> in lockdep complaints because complete() invokes functions containing RCU
> readers.  This patch therefore uses Thomas Gleixner's trick of delegating
> the complete() call to a surviving CPU via smp_call_function_single().
> 
> Reported-by: Peng Fan <[e-mail]>
> Reported-by: Russell King - ARM Linux <[e-mail]>
> Signed-off-by: Paul E. McKenney <[e-mail]>
> Tested-by: Tested-by: Fabio Estevam <[e-mail]>

Fabio reported only once, though he might have tested twice.

baruch

> Cc: Russell King <[e-mail]>
> Cc: Ingo Molnar <[e-mail]>
> Cc: "Peter Zijlstra (Intel)" <[e-mail]>
> Cc: Michal Hocko <[e-mail]>
> Cc: Thomas Gleixner <[e-mail]>
> Cc: <[e-mail]>
> 
> diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> index b4fbf00ee4ad..75f85e20aafa 100644
> --- a/arch/arm/kernel/smp.c
> +++ b/arch/arm/kernel/smp.c
> @@ -267,6 +267,14 @@ void __cpu_die(unsigned int cpu)
>  }
>  
>  /*
> + * Invoke complete() on behalf of the outgoing CPU.
> + */
> +static void arch_cpu_idle_dead_complete(void *arg)
> +{
> +	complete(&cpu_died);
> +}
> +
> +/*
>   * Called from the idle thread for the CPU which has been shutdown.
>   *
>   * Note that we disable IRQs here, but do not re-enable them
> @@ -293,9 +301,11 @@ void arch_cpu_idle_dead(void)
>  	/*
>  	 * Tell __cpu_die() that this CPU is now safe to dispose of.  Once
>  	 * this returns, power and/or clocks can be removed at any point
> -	 * from this CPU and its cache by platform_cpu_kill().
> +	 * from this CPU and its cache by platform_cpu_kill().  We cannot
> +	 * call complete() this late, so we delegate it to an online CPU.
>  	 */
> -	complete(&cpu_died);
> +	smp_call_function_single(cpumask_first(cpu_online_mask),
> +				 arch_cpu_idle_dead_complete, NULL, 0);
>  
>  	/*
>  	 * Ensure that the cache lines associated with that completion are

Paul E. McKenney Dec. 12, 2017, 7:31 p.m. UTC | #3

On Tue, Dec 12, 2017 at 07:40:46PM +0200, Baruch Siach wrote:
> Hi Paul,
> 
> On Tue, Dec 12, 2017 at 09:20:59AM -0800, Paul E. McKenney wrote:
> > The ARM implementation of arch_cpu_idle_dead() invokes complete(), but
> > does so after RCU has stopped watching the outgoing CPU, which results
> > in lockdep complaints because complete() invokes functions containing RCU
> > readers.  This patch therefore uses Thomas Gleixner's trick of delegating
> > the complete() call to a surviving CPU via smp_call_function_single().
> > 
> > Reported-by: Peng Fan <[e-mail]>
> > Reported-by: Russell King - ARM Linux <[e-mail]>
> > Signed-off-by: Paul E. McKenney <[e-mail]>
> > Tested-by: Tested-by: Fabio Estevam <[e-mail]>
> 
> Fabio reported only once, though he might have tested twice.

Actually, he did test twice.  Apparently this patch has problems
with CONFIG_BL_SWITCHER=y kernels.

But yes, I guess I did get a bit carried away with the Tested-by's,
didn't I?  ;-)

							Thanx, Paul

> baruch
> 
> > Cc: Russell King <[e-mail]>
> > Cc: Ingo Molnar <[e-mail]>
> > Cc: "Peter Zijlstra (Intel)" <[e-mail]>
> > Cc: Michal Hocko <[e-mail]>
> > Cc: Thomas Gleixner <[e-mail]>
> > Cc: <[e-mail]>
> > 
> > diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
> > index b4fbf00ee4ad..75f85e20aafa 100644
> > --- a/arch/arm/kernel/smp.c
> > +++ b/arch/arm/kernel/smp.c
> > @@ -267,6 +267,14 @@ void __cpu_die(unsigned int cpu)
> >  }
> >  
> >  /*
> > + * Invoke complete() on behalf of the outgoing CPU.
> > + */
> > +static void arch_cpu_idle_dead_complete(void *arg)
> > +{
> > +	complete(&cpu_died);
> > +}
> > +
> > +/*
> >   * Called from the idle thread for the CPU which has been shutdown.
> >   *
> >   * Note that we disable IRQs here, but do not re-enable them
> > @@ -293,9 +301,11 @@ void arch_cpu_idle_dead(void)
> >  	/*
> >  	 * Tell __cpu_die() that this CPU is now safe to dispose of.  Once
> >  	 * this returns, power and/or clocks can be removed at any point
> > -	 * from this CPU and its cache by platform_cpu_kill().
> > +	 * from this CPU and its cache by platform_cpu_kill().  We cannot
> > +	 * call complete() this late, so we delegate it to an online CPU.
> >  	 */
> > -	complete(&cpu_died);
> > +	smp_call_function_single(cpumask_first(cpu_online_mask),
> > +				 arch_cpu_idle_dead_complete, NULL, 0);
> >  
> >  	/*
> >  	 * Ensure that the cache lines associated with that completion are
> 
> -- 
>      http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
> =}------------------------------------------------ooO--U--Ooo------------{=
>    - [e-mail] - tel: [phone], http://www.tkos.co.il -
>

Paul E. McKenney Dec. 12, 2017, 7:36 p.m. UTC | #4

On Tue, Dec 12, 2017 at 05:37:59PM +0000, Russell King - ARM Linux wrote:
> On Tue, Dec 12, 2017 at 09:20:59AM -0800, Paul E. McKenney wrote:
> > The ARM implementation of arch_cpu_idle_dead() invokes complete(), but
> > does so after RCU has stopped watching the outgoing CPU, which results
> > in lockdep complaints because complete() invokes functions containing RCU
> > readers.  This patch therefore uses Thomas Gleixner's trick of delegating
> > the complete() call to a surviving CPU via smp_call_function_single().
> > 
> > Reported-by: Peng Fan <[e-mail]>
> > Reported-by: Russell King - ARM Linux <[e-mail]>
> > Signed-off-by: Paul E. McKenney <[e-mail]>
> > Tested-by: Tested-by: Fabio Estevam <[e-mail]>
> > Cc: Russell King <[e-mail]>
> > Cc: Ingo Molnar <[e-mail]>
> > Cc: "Peter Zijlstra (Intel)" <[e-mail]>
> > Cc: Michal Hocko <[e-mail]>
> > Cc: Thomas Gleixner <[e-mail]>
> > Cc: <[e-mail]>
> 
> As I just described in response to Fabio's testing, this doesn't solve
> anything if CONFIG_BL_SWITCHER is enabled.  We could lose the unlock of
> a spinlock in the GIC code for sending the IPI.  As I already said
> previously in our discussion (but I guess you just don't believe me):

Sorry, Russell, but most days I don't even believe myself.  So it is
nothing personal, just one of the occupational hazards of being me.

> "2. there's some optional locking in the GIC driver that cause problems
>    for the cpu dying path.
> 
> The concensus last time around was that the IPI solution is a non-
> starter, so the seven year proven-reliable solution (disregarding the
> RCU warning) persists because I don't think anyone came up with a
> better solution."
> 
> Using smp_call_function_single() invokes the IPI paths.

OK, another approach is to have the dying CPU simply set an in-memory
flag, which a surviving CPU polls for.  There are of course any number
of ways of doing the polling loop.

So what bad thing happens when you use that approach?

							Thanx, Paul

Patch

diff --git a/arch/arm/kernel/smp.c b/arch/arm/kernel/smp.c
index b4fbf00ee4ad..75f85e20aafa 100644
--- a/arch/arm/kernel/smp.c
+++ b/arch/arm/kernel/smp.c
@@ -267,6 +267,14 @@  void __cpu_die(unsigned int cpu)
 }
 
 /*
+ * Invoke complete() on behalf of the outgoing CPU.
+ */
+static void arch_cpu_idle_dead_complete(void *arg)
+{
+	complete(&cpu_died);
+}
+
+/*
  * Called from the idle thread for the CPU which has been shutdown.
  *
  * Note that we disable IRQs here, but do not re-enable them
@@ -293,9 +301,11 @@  void arch_cpu_idle_dead(void)
 	/*
 	 * Tell __cpu_die() that this CPU is now safe to dispose of.  Once
 	 * this returns, power and/or clocks can be removed at any point
-	 * from this CPU and its cache by platform_cpu_kill().
+	 * from this CPU and its cache by platform_cpu_kill().  We cannot
+	 * call complete() this late, so we delegate it to an online CPU.
 	 */
-	complete(&cpu_died);
+	smp_call_function_single(cpumask_first(cpu_online_mask),
+				 arch_cpu_idle_dead_complete, NULL, 0);
 
 	/*
 	 * Ensure that the cache lines associated with that completion are