stop secondaries for reboot/shutdown
Nicholas Piggin
reviewercover stop secondaries for reboot/shutdown
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(-)
Signature · 1 line
2.16.3
Nicholas Piggin
reviewerPATCH v2 1/3 powerpc: use NMI IPI for smp_send_stop
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.
--- arch/powerpc/kernel/smp.c | 8 ++++++++ 1 file changed, 8 insertions(+)
arch/powerpc/kernel/smp.c +8 -0
@@ -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 ];
Signature · 1 line
2.16.3
Re: [PATCH v2 1/3] powerpc: use NMI IPI for smp_send_stop
Quoted 33 lines · On Sun, 1 Apr 2018 20:36:13 +1000 Nicholas Piggin <[email protected]> wrote:
Use the NMI IPI rather than
smp_call_functionforsmp_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_functioninfrastructure and (if supported) using true NMI IPIs makes this more robust.Signed-off-by Nicholas Piggin <[email protected]>--- arch/powerpc/kernel/smp.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/powerpc/kernel/smp.c +4
-0@@ -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);
arch/powerpc/kernel/smp.c static void stop_this_cpu(void *dummy) · line 585
Quoted 5 lines
arch/powerpc/kernel/smp.c +2
-0@@ -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.
Nicholas Piggin
reviewerRe: [PATCH v2 1/3] powerpc: use NMI IPI for smp_send_stop
Quoted 37 lines · On Tue, 3 Apr 2018 10:13:26 +1000 Balbir Singh <[email protected]> wrote:
Quoted 34 lines · On Sun, 1 Apr 2018 20:36:13 +1000 Nicholas Piggin <[email protected]> wrote:
Use the NMI IPI rather than
smp_call_functionforsmp_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_functioninfrastructure and (if supported) using true NMI IPIs makes this more robust.Signed-off-by Nicholas Piggin <[email protected]>--- arch/powerpc/kernel/smp.c | 8 ++++++++ 1 file changed, 8 insertions(+)arch/powerpc/kernel/smp.c +4
-0@@ -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)
Quoted 8 lines
arch/powerpc/kernel/smp.c static void stop_this_cpu(void *dummy) · line 585
Quoted 4 lines
arch/powerpc/kernel/smp.c +2
-0void 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_usshould 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
Quoted 3 lines
Balbir Singh.
Re: [v2,1/3] powerpc: use NMI IPI for smp_send_stop
In reply to Nicholas PigginQuoted 4 lines · Nicholas Piggin
Use the NMI IPI rather than
smp_call_functionforsmp_send_stop. Have stopped CPUs hard disable interrupts rather than just soft disable.
Quoted 8 lines
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_functioninfrastructure and (if supported) using true NMI IPIs makes this more robust.Signed-off-by Nicholas Piggin <[email protected]>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/6bed3237624e3faad1592543952907
cheers
Nicholas Piggin
reviewerPATCH v2 2/3 powerpc: hard disable irqs in smp_send_stop loop
In reply to Nicholas PigginThe hard lockup watchdog can fire under local_irq_disable on platforms with irq soft masking.
--- arch/powerpc/kernel/smp.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
arch/powerpc/kernel/smp.c +3 -2
@@ -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 )
Signature · 1 line
2.16.3
Nicholas Piggin
reviewerPATCH v2 3/3 powerpc/powernv: Always stop secondaries before reboot/shutdown
In reply to Nicholas PigginCurrently 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.
--- 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
@@ -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 );
arch/powerpc/platforms/powernv/opal-flash.c +1 -27
@@ -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
@@ -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)
Signature · 1 line
2.16.3
Re: [PATCH v2 3/3] powerpc/powernv: Always stop secondaries before reboot/shutdown
Quoted 23 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 -- 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 LOCKUPwatchdog: BUG: soft lockup - CPU#16 stuck for 3s! [swapper/16:0]This patch removes the special case for flash update, and calls
smp_send_stopin all cases before calling reboot/shutdown.
Quoted 7 lines
smp_send_stopcould 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 <[email protected]>
Nick,
Patch looks good to me. We have tested this patchset on FSP based system and everything looks fine.
-Vasant
Quoted 96 lines
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@@ -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 );arch/powerpc/platforms/powernv/opal-flash.c +1
-27@@ -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@@ -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)
Quoted 8 lines · 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.
I have tested this series on a FSP platform with firmware update and multiple reboot/shutdown tests. Not seen any issues.
Thanks
Pridhiviraj
Quoted 14 lines
Thanks,
NickNicholas Piggin (3):
powerpc: use NMI IPI forsmp_send_stop
powerpc: hard disable irqs insmp_send_stoploop powerpc/powernv: Always stop secondaries before reboot/shutdownarch/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(-)