[PATCH 0/3] stop secondaries for reboot/shutdown

[PATCH 0/3] stop secondaries for reboot/shutdown

[Nicholas Piggin]

I'm rebasing and resending this series because the hard
lockup messages are being seen in the field.

Since last time, one of the patches was merged, and patches
were split and reordered a bit. No major changes.

This still hasn't been tested with the FSP firmware update.

Thanks,
Nick

Nicholas Piggin (3):
  powerpc: use NMI IPI for smp_send_stop
  powerpc: hard disable irqs in smp_send_stop loop
  powerpc/powernv: Always stop secondaries before reboot/shutdown

 arch/powerpc/include/asm/opal.h             |  2 +-
 arch/powerpc/kernel/smp.c                   | 13 +++++++++++--
 arch/powerpc/platforms/powernv/opal-flash.c | 28 +---------------------------
 arch/powerpc/platforms/powernv/setup.c      | 15 +++++----------
 4 files changed, 18 insertions(+), 40 deletions(-)

-- 
2.16.3

[PATCH v2 1/3] powerpc: use NMI IPI for smp_send_stop

[Nicholas Piggin]

Use the NMI IPI rather than smp_call_function for smp_send_stop.
Have stopped CPUs hard disable interrupts rather than just soft
disable.

This function is used in crash/panic/shutdown paths to bring other
CPUs down as quickly and reliably as possible, and minimizing their
potential to cause trouble.

Avoiding the Linux smp_call_function infrastructure and (if supported)
using true NMI IPIs makes this more robust.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/smp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index cfc08b099c49..db88660bf6bd 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -565,7 +565,11 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
 }
 #endif
 
+#ifdef CONFIG_NMI_IPI
+static void stop_this_cpu(struct pt_regs *regs)
+#else
 static void stop_this_cpu(void *dummy)
+#endif
 {
 	/* Remove this CPU */
 	set_cpu_online(smp_processor_id(), false);
@@ -577,7 +581,11 @@ static void stop_this_cpu(void *dummy)
 
 void smp_send_stop(void)
 {
+#ifdef CONFIG_NMI_IPI
+	smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
+#else
 	smp_call_function(stop_this_cpu, NULL, 0);
+#endif
 }
 
 struct thread_info *current_set[NR_CPUS];
-- 
2.16.3

[PATCH v2 2/3] powerpc: hard disable irqs in smp_send_stop loop

[Nicholas Piggin]

The hard lockup watchdog can fire under local_irq_disable
on platforms with irq soft masking.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/smp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index db88660bf6bd..e16ec7b3b427 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -574,9 +574,10 @@ static void stop_this_cpu(void *dummy)
 	/* Remove this CPU */
 	set_cpu_online(smp_processor_id(), false);
 
-	local_irq_disable();
+	hard_irq_disable();
+	spin_begin();
 	while (1)
-		;
+		spin_cpu_relax();
 }
 
 void smp_send_stop(void)
-- 
2.16.3

[PATCH v2 3/3] powerpc/powernv: Always stop secondaries before reboot/shutdown

[Nicholas Piggin]

Currently powernv reboot and shutdown requests just leave secondaries
to do their own things. This is undesirable because they can trigger
any number of watchdogs while waiting for reboot, but also we don't
know what else they might be doing -- they might be causing trouble,
trampling memory, etc.

The opal scheduled flash update code already ran into watchdog problems
due to flashing taking a long time, and it was fixed with 2196c6f1ed
("powerpc/powernv: Return secondary CPUs to firmware before FW update"),
which returns secondaries to opal. It's been found that regular reboots
can take over 10 seconds, which can result in the hard lockup watchdog
firing,

  reboot: Restarting system
  [  360.038896709,5] OPAL: Reboot request...
  Watchdog CPU:0 Hard LOCKUP
  Watchdog CPU:44 detected Hard LOCKUP other CPUS:16
  Watchdog CPU:16 Hard LOCKUP
  watchdog: BUG: soft lockup - CPU#16 stuck for 3s! [swapper/16:0]

This patch removes the special case for flash update, and calls
smp_send_stop in all cases before calling reboot/shutdown.

smp_send_stop could return CPUs to OPAL, the main reason not to is
that the request could come from a NMI that interrupts OPAL code,
so re-entry to OPAL can cause a number of problems. Putting
secondaries into simple spin loops improves the chances of a
successful reboot.

Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/include/asm/opal.h             |  2 +-
 arch/powerpc/platforms/powernv/opal-flash.c | 28 +---------------------------
 arch/powerpc/platforms/powernv/setup.c      | 15 +++++----------
 3 files changed, 7 insertions(+), 38 deletions(-)

diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index dde60089d0d4..7159e1a6a61a 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -325,7 +325,7 @@ struct rtc_time;
 extern unsigned long opal_get_boot_time(void);
 extern void opal_nvram_init(void);
 extern void opal_flash_update_init(void);
-extern void opal_flash_term_callback(void);
+extern void opal_flash_update_print_message(void);
 extern int opal_elog_init(void);
 extern void opal_platform_dump_init(void);
 extern void opal_sys_param_init(void);
diff --git a/arch/powerpc/platforms/powernv/opal-flash.c b/arch/powerpc/platforms/powernv/opal-flash.c
index 1cb0b895a236..b37015101bf6 100644
--- a/arch/powerpc/platforms/powernv/opal-flash.c
+++ b/arch/powerpc/platforms/powernv/opal-flash.c
@@ -303,26 +303,9 @@ static int opal_flash_update(int op)
 	return rc;
 }
 
-/* Return CPUs to OPAL before starting FW update */
-static void flash_return_cpu(void *info)
-{
-	int cpu = smp_processor_id();
-
-	if (!cpu_online(cpu))
-		return;
-
-	/* Disable IRQ */
-	hard_irq_disable();
-
-	/* Return the CPU to OPAL */
-	opal_return_cpu();
-}
-
 /* This gets called just before system reboots */
-void opal_flash_term_callback(void)
+void opal_flash_update_print_message(void)
 {
-	struct cpumask mask;
-
 	if (update_flash_data.status != FLASH_IMG_READY)
 		return;
 
@@ -333,15 +316,6 @@ void opal_flash_term_callback(void)
 
 	/* Small delay to help getting the above message out */
 	msleep(500);
-
-	/* Return secondary CPUs to firmware */
-	cpumask_copy(&mask, cpu_online_mask);
-	cpumask_clear_cpu(smp_processor_id(), &mask);
-	if (!cpumask_empty(&mask))
-		smp_call_function_many(&mask,
-				       flash_return_cpu, NULL, false);
-	/* Hard disable interrupts */
-	hard_irq_disable();
 }
 
 /*
diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
index 5f963286232f..ef8c9ce53a61 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -201,17 +201,12 @@ static void pnv_prepare_going_down(void)
 	 */
 	opal_event_shutdown();
 
-	/* Soft disable interrupts */
-	local_irq_disable();
+	/* Print flash update message if one is scheduled. */
+	opal_flash_update_print_message();
 
-	/*
-	 * Return secondary CPUs to firwmare if a flash update
-	 * is pending otherwise we will get all sort of error
-	 * messages about CPU being stuck etc.. This will also
-	 * have the side effect of hard disabling interrupts so
-	 * past this point, the kernel is effectively dead.
-	 */
-	opal_flash_term_callback();
+	smp_send_stop();
+
+	hard_irq_disable();
 }
 
 static void  __noreturn pnv_restart(char *cmd)
-- 
2.16.3

Re: [PATCH 0/3] stop secondaries for reboot/shutdown

[ppaidipe]

On 2018-04-01 16:06, Nicholas Piggin wrote:
> I'm rebasing and resending this series because the hard
> lockup messages are being seen in the field.
> 
> Since last time, one of the patches was merged, and patches
> were split and reordered a bit. No major changes.
> 
> This still hasn't been tested with the FSP firmware update.
> 

I have tested this series on a FSP platform with firmware update
and multiple reboot/shutdown tests. Not seen any issues.

Tested-by: Pridhiviraj Paidipeddi <ppaidipe@linux.vnet.ibm.com>

Thanks
Pridhiviraj

> Thanks,
> Nick
> 
> Nicholas Piggin (3):
>   powerpc: use NMI IPI for smp_send_stop
>   powerpc: hard disable irqs in smp_send_stop loop
>   powerpc/powernv: Always stop secondaries before reboot/shutdown
> 
>  arch/powerpc/include/asm/opal.h             |  2 +-
>  arch/powerpc/kernel/smp.c                   | 13 +++++++++++--
>  arch/powerpc/platforms/powernv/opal-flash.c | 28 
> +---------------------------
>  arch/powerpc/platforms/powernv/setup.c      | 15 +++++----------
>  4 files changed, 18 insertions(+), 40 deletions(-)

Re: [PATCH v2 1/3] powerpc: use NMI IPI for smp_send_stop

[Balbir Singh]

On Sun,  1 Apr 2018 20:36:13 +1000
Nicholas Piggin <npiggin@gmail.com> wrote:

> Use the NMI IPI rather than smp_call_function for smp_send_stop.
> Have stopped CPUs hard disable interrupts rather than just soft
> disable.
> 
> This function is used in crash/panic/shutdown paths to bring other
> CPUs down as quickly and reliably as possible, and minimizing their
> potential to cause trouble.
> 
> Avoiding the Linux smp_call_function infrastructure and (if supported)
> using true NMI IPIs makes this more robust.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  arch/powerpc/kernel/smp.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index cfc08b099c49..db88660bf6bd 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -565,7 +565,11 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
>  }
>  #endif
>  
> +#ifdef CONFIG_NMI_IPI
> +static void stop_this_cpu(struct pt_regs *regs)
> +#else
>  static void stop_this_cpu(void *dummy)
> +#endif
>  {
>  	/* Remove this CPU */
>  	set_cpu_online(smp_processor_id(), false);
> @@ -577,7 +581,11 @@ static void stop_this_cpu(void *dummy)
>  
>  void smp_send_stop(void)
>  {
> +#ifdef CONFIG_NMI_IPI
> +	smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);

I wonder if the delay_us should be a function of number of cpus and the
callee should figure this out on its own? May be not in this series, but
in the longer run.


Balbir Singh.

Re: [PATCH v2 1/3] powerpc: use NMI IPI for smp_send_stop

[Nicholas Piggin]

On Tue, 3 Apr 2018 10:13:26 +1000
Balbir Singh <bsingharora@gmail.com> wrote:

> On Sun,  1 Apr 2018 20:36:13 +1000
> Nicholas Piggin <npiggin@gmail.com> wrote:
> 
> > Use the NMI IPI rather than smp_call_function for smp_send_stop.
> > Have stopped CPUs hard disable interrupts rather than just soft
> > disable.
> > 
> > This function is used in crash/panic/shutdown paths to bring other
> > CPUs down as quickly and reliably as possible, and minimizing their
> > potential to cause trouble.
> > 
> > Avoiding the Linux smp_call_function infrastructure and (if supported)
> > using true NMI IPIs makes this more robust.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  arch/powerpc/kernel/smp.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index cfc08b099c49..db88660bf6bd 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -565,7 +565,11 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
> >  }
> >  #endif
> >  
> > +#ifdef CONFIG_NMI_IPI
> > +static void stop_this_cpu(struct pt_regs *regs)
> > +#else
> >  static void stop_this_cpu(void *dummy)
> > +#endif
> >  {
> >  	/* Remove this CPU */
> >  	set_cpu_online(smp_processor_id(), false);
> > @@ -577,7 +581,11 @@ static void stop_this_cpu(void *dummy)
> >  
> >  void smp_send_stop(void)
> >  {
> > +#ifdef CONFIG_NMI_IPI
> > +	smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);  
> 
> I wonder if the delay_us should be a function of number of cpus and the
> callee should figure this out on its own? May be not in this series, but
> in the longer run.

It possibly should. The big per-cpu/core serialized delay is in firmware
(worst case we have to do a special wakeup on each core and bring it out
of deep stop), and that gets done before the delay starts. So we count
delay from after all threads are woken and given their sreset command, so
this is probably okay even for very big systems.

But it should at least be a #define constant if not something more
sophisticated like you suggest. We have a few delays like that including
in xmon and other crash paths that could use some more thought.

Thanks,
Nick


> 
> 
> Balbir Singh.

Re: [PATCH v2 3/3] powerpc/powernv: Always stop secondaries before reboot/shutdown

[Vasant Hegde]

On 04/01/2018 04:06 PM, Nicholas Piggin wrote:
> Currently powernv reboot and shutdown requests just leave secondaries
> to do their own things. This is undesirable because they can trigger
> any number of watchdogs while waiting for reboot, but also we don't
> know what else they might be doing -- they might be causing trouble,
> trampling memory, etc.
> 
> The opal scheduled flash update code already ran into watchdog problems
> due to flashing taking a long time, and it was fixed with 2196c6f1ed
> ("powerpc/powernv: Return secondary CPUs to firmware before FW update"),
> which returns secondaries to opal. It's been found that regular reboots
> can take over 10 seconds, which can result in the hard lockup watchdog
> firing,
> 
>    reboot: Restarting system
>    [  360.038896709,5] OPAL: Reboot request...
>    Watchdog CPU:0 Hard LOCKUP
>    Watchdog CPU:44 detected Hard LOCKUP other CPUS:16
>    Watchdog CPU:16 Hard LOCKUP
>    watchdog: BUG: soft lockup - CPU#16 stuck for 3s! [swapper/16:0]
> 
> This patch removes the special case for flash update, and calls
> smp_send_stop in all cases before calling reboot/shutdown.
> 
> smp_send_stop could return CPUs to OPAL, the main reason not to is
> that the request could come from a NMI that interrupts OPAL code,
> so re-entry to OPAL can cause a number of problems. Putting
> secondaries into simple spin loops improves the chances of a
> successful reboot.
> 
> Cc: Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

Nick,

Patch looks good to me. We have tested this patchset on FSP based system and 
everything looks fine.

Reviewed-by:  Vasant Hegde <hegdevasant@linux.vnet.ibm.com>

-Vasant

> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/include/asm/opal.h             |  2 +-
>   arch/powerpc/platforms/powernv/opal-flash.c | 28 +---------------------------
>   arch/powerpc/platforms/powernv/setup.c      | 15 +++++----------
>   3 files changed, 7 insertions(+), 38 deletions(-)
> 
> diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
> index dde60089d0d4..7159e1a6a61a 100644
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -325,7 +325,7 @@ struct rtc_time;
>   extern unsigned long opal_get_boot_time(void);
>   extern void opal_nvram_init(void);
>   extern void opal_flash_update_init(void);
> -extern void opal_flash_term_callback(void);
> +extern void opal_flash_update_print_message(void);
>   extern int opal_elog_init(void);
>   extern void opal_platform_dump_init(void);
>   extern void opal_sys_param_init(void);
> diff --git a/arch/powerpc/platforms/powernv/opal-flash.c b/arch/powerpc/platforms/powernv/opal-flash.c
> index 1cb0b895a236..b37015101bf6 100644
> --- a/arch/powerpc/platforms/powernv/opal-flash.c
> +++ b/arch/powerpc/platforms/powernv/opal-flash.c
> @@ -303,26 +303,9 @@ static int opal_flash_update(int op)
>   	return rc;
>   }
> 
> -/* Return CPUs to OPAL before starting FW update */
> -static void flash_return_cpu(void *info)
> -{
> -	int cpu = smp_processor_id();
> -
> -	if (!cpu_online(cpu))
> -		return;
> -
> -	/* Disable IRQ */
> -	hard_irq_disable();
> -
> -	/* Return the CPU to OPAL */
> -	opal_return_cpu();
> -}
> -
>   /* This gets called just before system reboots */
> -void opal_flash_term_callback(void)
> +void opal_flash_update_print_message(void)
>   {
> -	struct cpumask mask;
> -
>   	if (update_flash_data.status != FLASH_IMG_READY)
>   		return;
> 
> @@ -333,15 +316,6 @@ void opal_flash_term_callback(void)
> 
>   	/* Small delay to help getting the above message out */
>   	msleep(500);
> -
> -	/* Return secondary CPUs to firmware */
> -	cpumask_copy(&mask, cpu_online_mask);
> -	cpumask_clear_cpu(smp_processor_id(), &mask);
> -	if (!cpumask_empty(&mask))
> -		smp_call_function_many(&mask,
> -				       flash_return_cpu, NULL, false);
> -	/* Hard disable interrupts */
> -	hard_irq_disable();
>   }
> 
>   /*
> diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c
> index 5f963286232f..ef8c9ce53a61 100644
> --- a/arch/powerpc/platforms/powernv/setup.c
> +++ b/arch/powerpc/platforms/powernv/setup.c
> @@ -201,17 +201,12 @@ static void pnv_prepare_going_down(void)
>   	 */
>   	opal_event_shutdown();
> 
> -	/* Soft disable interrupts */
> -	local_irq_disable();
> +	/* Print flash update message if one is scheduled. */
> +	opal_flash_update_print_message();
> 
> -	/*
> -	 * Return secondary CPUs to firwmare if a flash update
> -	 * is pending otherwise we will get all sort of error
> -	 * messages about CPU being stuck etc.. This will also
> -	 * have the side effect of hard disabling interrupts so
> -	 * past this point, the kernel is effectively dead.
> -	 */
> -	opal_flash_term_callback();
> +	smp_send_stop();
> +
> +	hard_irq_disable();
>   }
> 
>   static void  __noreturn pnv_restart(char *cmd)
> 

Re: [v2,1/3] powerpc: use NMI IPI for smp_send_stop

[Michael Ellerman]

On Sun, 2018-04-01 at 10:36:13 UTC, Nicholas Piggin wrote:
> Use the NMI IPI rather than smp_call_function for smp_send_stop.
> Have stopped CPUs hard disable interrupts rather than just soft
> disable.
> 
> This function is used in crash/panic/shutdown paths to bring other
> CPUs down as quickly and reliably as possible, and minimizing their
> potential to cause trouble.
> 
> Avoiding the Linux smp_call_function infrastructure and (if supported)
> using true NMI IPIs makes this more robust.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/6bed3237624e3faad1592543952907

cheers