[PATCH v2 0/3] avoid secondary hold spinloop when possible

[PATCH v2 0/3] avoid secondary hold spinloop when possible

[Nicholas Piggin]

I've made this series only avoid the secondary spinloop on powernv.
pSeries is a little bit more complicated, some cases in kexec the
secondaries will be at 0x60. I haven't had time to get all that
sorted out for this merge window, so OPAL-only this time.

Nicholas Piggin (3):
  powerpc/powernv: Always stop secondaries before reboot/shutdown
  powerpc: use NMI IPI for smp_send_stop
  powerpc/powernv: Avoid waiting for secondary hold spinloop with OPAL

 arch/powerpc/include/asm/opal.h             |  2 +-
 arch/powerpc/kernel/head_64.S               | 16 +++++++++++-----
 arch/powerpc/kernel/setup_64.c              | 10 +++++++++-
 arch/powerpc/kernel/smp.c                   |  9 +++++----
 arch/powerpc/platforms/powernv/opal-flash.c | 28 +---------------------------
 arch/powerpc/platforms/powernv/setup.c      | 15 +++++----------
 6 files changed, 32 insertions(+), 48 deletions(-)

-- 
2.13.3

[PATCH v2 1/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, or they might be stuck somewhere
causing trouble.

The opal scheduled flash update code already ran into watchdog problems
due to flashing taking a long time, but it's possible for regular
reboots to trigger problems too (this is with watchdog_thresh set to 1,
but I have seen it with watchdog_thresh at the default value once too):

  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]

So remove the special case for flash update, and unconditionally do
smp_send_stop before rebooting.

Return the CPUs to Linux stop loops rather than OPAL. The reason for
this is that the path to firmware is longer, and the CPUs may have
been interrupted from firmware, which may cause problems to re-enter
it. It's better to put them into a simple spin loop to maximize the
chance of a successful reboot.

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 04c32b08ffa1..ce58f4139ff5 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -317,7 +317,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 2fa3ac80cb4e..632871d78576 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 b23d8beb0f1e..ff9422776a81 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -112,17 +112,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.13.3

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> 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, or they might be stuck somewhere
> causing trouble.
>
> The opal scheduled flash update code already ran into watchdog problems
> due to flashing taking a long time, but it's possible for regular
> reboots to trigger problems too (this is with watchdog_thresh set to 1,
> but I have seen it with watchdog_thresh at the default value once too):
>
>   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]
>
> So remove the special case for flash update, and unconditionally do
> smp_send_stop before rebooting.
>
> Return the CPUs to Linux stop loops rather than OPAL. The reason for
> this is that the path to firmware is longer, and the CPUs may have
> been interrupted from firmware, which may cause problems to re-enter
> it. It's better to put them into a simple spin loop to maximize the
> chance of a successful reboot.

I always assumed we had to send the CPUs back to OPAL for the flashing
procedure. Is it OK to leave them in Linux?

cheers

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

[Nicholas Piggin]

On Fri, 10 Nov 2017 22:08:32 +1100
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > 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, or they might be stuck somewhere
> > causing trouble.
> >
> > The opal scheduled flash update code already ran into watchdog problems
> > due to flashing taking a long time, but it's possible for regular
> > reboots to trigger problems too (this is with watchdog_thresh set to 1,
> > but I have seen it with watchdog_thresh at the default value once too):
> >
> >   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]
> >
> > So remove the special case for flash update, and unconditionally do
> > smp_send_stop before rebooting.
> >
> > Return the CPUs to Linux stop loops rather than OPAL. The reason for
> > this is that the path to firmware is longer, and the CPUs may have
> > been interrupted from firmware, which may cause problems to re-enter
> > it. It's better to put them into a simple spin loop to maximize the
> > chance of a successful reboot.  
> 
> I always assumed we had to send the CPUs back to OPAL for the flashing
> procedure. Is it OK to leave them in Linux?

According to the comment and changelog

2196c6f1ed66eef23df3b478cfe71661ae83726e

It was added just to keep secondaries from going silly. Vasant, can
you remember details?

Thanks,
Nick

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

[Michael Ellerman]

Nicholas Piggin <npiggin@gmail.com> writes:

> On Fri, 10 Nov 2017 22:08:32 +1100
> Michael Ellerman <mpe@ellerman.id.au> wrote:
>
>> Nicholas Piggin <npiggin@gmail.com> writes:
>> 
>> > 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, or they might be stuck somewhere
>> > causing trouble.
>> >
>> > The opal scheduled flash update code already ran into watchdog problems
>> > due to flashing taking a long time, but it's possible for regular
>> > reboots to trigger problems too (this is with watchdog_thresh set to 1,
>> > but I have seen it with watchdog_thresh at the default value once too):
>> >
>> >   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]
>> >
>> > So remove the special case for flash update, and unconditionally do
>> > smp_send_stop before rebooting.
>> >
>> > Return the CPUs to Linux stop loops rather than OPAL. The reason for
>> > this is that the path to firmware is longer, and the CPUs may have
>> > been interrupted from firmware, which may cause problems to re-enter
>> > it. It's better to put them into a simple spin loop to maximize the
>> > chance of a successful reboot.  
>> 
>> I always assumed we had to send the CPUs back to OPAL for the flashing
>> procedure. Is it OK to leave them in Linux?
>
> According to the comment and changelog
>
> 2196c6f1ed66eef23df3b478cfe71661ae83726e
>
> It was added just to keep secondaries from going silly. Vasant, can
> you remember details?

OK. My worry is that we've established an implicit contract with skiboot
on how we do this, and now we're looking to change it.

So I guess we just want to confirm that the skiboot code has not grown a
dependency on us returning CPUs, and then we should probably document
what the expectations are in eg. the OPAL_FLASH_UPDATE docs.

cheers

[PATCH v2 2/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.

Also use spin loop primitives in the stop callback, mainly to help
processing speed of the active thread speed in the simulator.

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

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index e0a4c1f82e25..ce891030d925 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -547,19 +547,20 @@ void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *))
 }
 #endif
 
-static void stop_this_cpu(void *dummy)
+static void __noreturn stop_this_cpu(struct pt_regs *regs)
 {
 	/* 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)
 {
-	smp_call_function(stop_this_cpu, NULL, 0);
+	smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
 }
 
 struct thread_info *current_set[NR_CPUS];
-- 
2.13.3

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

Hi Nicholas,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.14-rc6 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/avoid-secondary-hold-spinloop-when-possible/20171023-173012
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-ge_imp3a_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/smp.o: In function `smp_send_stop':
>> smp.c:(.text+0x774): undefined reference to `smp_send_nmi_ipi'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 18199 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]

Hi Nicholas,

[auto build test ERROR on powerpc/next]
[also build test ERROR on v4.14-rc6 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/avoid-secondary-hold-spinloop-when-possible/20171023-173012
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-iss476-smp_defconfig (attached as .config)
compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All errors (new ones prefixed by >>):

   arch/powerpc/kernel/smp.o: In function `smp_send_stop':
>> arch/powerpc/kernel/smp.c:563: undefined reference to `smp_send_nmi_ipi'

vim +563 arch/powerpc/kernel/smp.c

   560	
   561	void smp_send_stop(void)
   562	{
 > 563		smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);
   564	}
   565	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10340 bytes --]

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

[Nicholas Piggin]

On Tue, 24 Oct 2017 07:15:30 +0800
kbuild test robot <lkp@intel.com> wrote:

> Hi Nicholas,
> 
> [auto build test ERROR on powerpc/next]
> [also build test ERROR on v4.14-rc6 next-20171018]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> 
> url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/avoid-secondary-hold-spinloop-when-possible/20171023-173012
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
> config: powerpc-iss476-smp_defconfig (attached as .config)
> compiler: powerpc-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # save the attached .config to linux build tree
>         make.cross ARCH=powerpc 
> 
> All errors (new ones prefixed by >>):
> 
>    arch/powerpc/kernel/smp.o: In function `smp_send_stop':
> >> arch/powerpc/kernel/smp.c:563: undefined reference to `smp_send_nmi_ipi'  
> 
> vim +563 arch/powerpc/kernel/smp.c
> 
>    560	
>    561	void smp_send_stop(void)
>    562	{
>  > 563		smp_send_nmi_ipi(NMI_IPI_ALL_OTHERS, stop_this_cpu, 1000000);  
>    564	}
>    565	

Good catch, 0day. Thanks.

Rather than have all architectures select NMI_IPI just for this, I
think I'll make it depend on whether NMI_IPI was selected at all.
Otherwise just keep using the normal smp_call_function().

Thanks,
Nick

[PATCH v2 3/3] powerpc/powernv: Avoid waiting for secondary hold spinloop with OPAL

[Nicholas Piggin]

OPAL boot does not insert secondaries at 0x60 to wait at the secondary
hold spinloop. Instead they are started later, and inserted at
generic_secondary_smp_init(), which is after the secondary hold
spinloop.

Avoid waiting on this spinloop when booting with OPAL firmware. This
wait always times out that case.

This saves 100ms boot time on powernv, and 10s of seconds of real time
when booting on the simulator in SMP.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/head_64.S  | 16 +++++++++++-----
 arch/powerpc/kernel/setup_64.c | 10 +++++++++-
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index c9e760ec7530..0deef350004f 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -55,12 +55,18 @@
  *
  *  For pSeries or server processors:
  *   1. The MMU is off & open firmware is running in real mode.
- *   2. The kernel is entered at __start
+ *   2. The primary CPU enters at __start.
+ *   3. If the RTAS supports "query-cpu-stopped-state", then secondary
+ *      CPUs will enter as directed by "start-cpu" RTAS call, which is
+ *      generic_secondary_smp_init, with PIR in r3.
+ *   4. Else the secondary CPUs will enter at secondary_hold (0x60) as
+ *      directed by the "start-cpu" RTS call, with PIR in r3.
  * -or- For OPAL entry:
- *   1. The MMU is off, processor in HV mode, primary CPU enters at 0
- *      with device-tree in gpr3. We also get OPAL base in r8 and
- *	entry in r9 for debugging purposes
- *   2. Secondary processors enter at 0x60 with PIR in gpr3
+ *   1. The MMU is off, processor in HV mode.
+ *   2. The primary CPU enters at 0 with device-tree in r3, OPAL base
+ *      in r8, and entry in r9 for debugging purposes.
+ *   3. Secondary CPUs enter as directed by OPAL_START_CPU call, which
+ *      is at generic_secondary_smp_init, with PIR in r3.
  *
  *  For Book3E processors:
  *   1. The MMU is on running in AS0 in a state defined in ePAPR
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 3f2453858f60..ecaab70e4b78 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -363,8 +363,16 @@ void early_setup_secondary(void)
 #if defined(CONFIG_SMP) || defined(CONFIG_KEXEC_CORE)
 static bool use_spinloop(void)
 {
-	if (!IS_ENABLED(CONFIG_PPC_BOOK3E))
+	if (IS_ENABLED(CONFIG_PPC_BOOK3S)) {
+		/*
+		 * See comments in head_64.S -- not all platforms insert
+		 * secondaries at __secondary_hold and wait at the spin
+		 * loop.
+		 */
+		if (firmware_has_feature(FW_FEATURE_OPAL))
+			return false;
 		return true;
+	}
 
 	/*
 	 * When book3e boots from kexec, the ePAPR spin table does
-- 
2.13.3

Re: [v2, 3/3] powerpc/powernv: Avoid waiting for secondary hold spinloop with OPAL

[Michael Ellerman]

On Mon, 2017-10-23 at 08:05:07 UTC, Nicholas Piggin wrote:
> OPAL boot does not insert secondaries at 0x60 to wait at the secondary
> hold spinloop. Instead they are started later, and inserted at
> generic_secondary_smp_init(), which is after the secondary hold
> spinloop.
> 
> Avoid waiting on this spinloop when booting with OPAL firmware. This
> wait always times out that case.
> 
> This saves 100ms boot time on powernv, and 10s of seconds of real time
> when booting on the simulator in SMP.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/339a3293f4e493a6c40f71e4faab0c

cheers