avoid secondary hold spinloop when possible

PATCH Applied Nicholas Piggin 4 participants 11 messages Oct 23, 2017 – Nov 14, 2017 PowerPC

Share & tools

Nicholas Piggin

author reviewer

2017-10-23 08:05 UTC #0 lore raw

v2 cover avoid secondary hold spinloop when possible

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

Signature · 1 line

2.13.3

Nicholas Piggin

author reviewer

2017-10-23 08:05 UTC #1 lore raw

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

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 <[email protected]>

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

arch/powerpc/include/asm/opal.h +1 -1

index 04c32b08ffa1..ce58f4139ff5 100644
@@ -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);

arch/powerpc/platforms/powernv/opal-flash.c +1 ~~-27~~

index 2fa3ac80cb4e..632871d78576 100644
@@ -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();
 }
 
 /*

arch/powerpc/platforms/powernv/setup.c +5 ~~-10~~

index b23d8beb0f1e..ff9422776a81 100644
@@ -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)

Signature · 1 line

2.13.3

Michael Ellerman

reviewer

2017-11-10 11:08 UTC #2 lore raw

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

Quoted 18 lines · 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]

Quoted 8 lines

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

Nicholas Piggin

author reviewer

2017-11-11 06:00 UTC #3 lore raw

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

Quoted 23 lines · On Fri, 10 Nov 2017 22:08:32 +1100 Michael Ellerman <[email protected]> wrote:

Quoted 21 lines · 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.

Quoted 8 lines

Quoted 5 lines
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

Michael Ellerman

reviewer

2017-11-12 11:57 UTC #4 lore raw

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

Quoted 35 lines · Nicholas Piggin

Quoted 23 lines · On Fri, 10 Nov 2017 22:08:32 +1100 Michael Ellerman <[email protected]> wrote:
Quoted 21 lines · 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.
Quoted 8 lines
Quoted 5 lines
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?

Quoted 6 lines

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

Nicholas Piggin

author reviewer

2017-10-23 08:05 UTC #5 lore raw

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

In reply to 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 <[email protected]>

---
 arch/powerpc/kernel/smp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

arch/powerpc/kernel/smp.c +5 -4

index e0a4c1f82e25..ce891030d925 100644
@@ -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];

Signature · 1 line

2.13.3

kbuild test robot

build bot

2017-10-23 21:06 UTC #6 lore raw

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

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…

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

kbuild test robot

build bot

2017-10-23 23:16 UTC #7 lore raw

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

In reply to Nicholas Piggin

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…

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

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

Nicholas Piggin

author reviewer

2017-10-31 07:43 UTC #8 lore raw

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

Quoted 21 lines · On Tue, 24 Oct 2017 07:15:30 +0800 kbuild test robot <[email protected]> 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':
Quoted 1 line
Quoted 1 line
arch/powerpc/kernel/smp.c:563: undefined reference to `smp_send_nmi_ipi'

Quoted 8 lines

vim +563 arch/powerpc/kernel/smp.c
   560	
   561	void smp_send_stop(void)
   562	{
Quoted 1 line
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

Nicholas Piggin

author reviewer

2017-10-23 08:05 UTC #9 lore raw

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

In reply to 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 <[email protected]>

---
 arch/powerpc/kernel/head_64.S  | 16 +++++++++++-----
 arch/powerpc/kernel/setup_64.c | 10 +++++++++-
 2 files changed, 20 insertions(+), 6 deletions(-)

arch/powerpc/kernel/head_64.S +11 -5

index c9e760ec7530..0deef350004f 100644
@@ -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

arch/powerpc/kernel/setup_64.c +9 -1

index 3f2453858f60..ecaab70e4b78 100644
@@ -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

Signature · 1 line

2.13.3

Michael Ellerman

Applied

2017-11-14 11:12 UTC #10 lore raw

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

Quoted 5 lines · 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.

Quoted 7 lines

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 <[email protected]>

Applied to powerpc next, thanks.

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

cheers