[2/2] arm64: Implement panic_smp_self_stop()
diff mbox series

Message ID 20190611181050.9647-2-aaro.koskinen@iki.fi
State New, archived
Headers show
Series
  • [1/2] arm64: Improve parking of stopped CPUs
Related show

Commit Message

Aaro Koskinen June 11, 2019, 6:10 p.m. UTC
From: Aaro Koskinen <aaro.koskinen@nokia.com>

Currently arm64 uses the default implementation of panic_smp_self_stop()
that is simply a cpu_relax() loop. As a result, when two CPUs panic()
simultaneously we get "SMP: failed to stop secondary CPUs" warnings and
extra delays before a reset.

Provide an implementation of panic_smp_self_stop() that offlines the
CPU properly.

Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com>
---
 arch/arm64/kernel/smp.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

James Morse June 12, 2019, 3:18 p.m. UTC | #1
Hi Aaro,

On 11/06/2019 19:10, Aaro Koskinen wrote:
> From: Aaro Koskinen <aaro.koskinen@nokia.com>
> 
> Currently arm64 uses the default implementation of panic_smp_self_stop()
> that is simply a cpu_relax() loop. As a result, when two CPUs panic()
> simultaneously we get "SMP: failed to stop secondary CPUs" warnings and
> extra delays before a reset.

> Provide an implementation of panic_smp_self_stop() that offlines the
> CPU properly.

This had me looking to the PSCI call that would take the CPU offline, but its just
conflicting terminology. Its the:
| set_cpu_online(cpu, false);
you're referring to here.

Would 'marks the CPU offline' be clearer?


Regardless,
Reviewed-by: James Morse <james.morse@arm.com>


Thanks,

James
Aaro Koskinen June 12, 2019, 10:38 p.m. UTC | #2
Hi,

On Wed, Jun 12, 2019 at 04:18:32PM +0100, James Morse wrote:
> On 11/06/2019 19:10, Aaro Koskinen wrote:
> > Currently arm64 uses the default implementation of panic_smp_self_stop()
> > that is simply a cpu_relax() loop. As a result, when two CPUs panic()
> > simultaneously we get "SMP: failed to stop secondary CPUs" warnings and
> > extra delays before a reset.
> 
> > Provide an implementation of panic_smp_self_stop() that offlines the
> > CPU properly.
> 
> This had me looking to the PSCI call that would take the CPU offline, but its just
> conflicting terminology. Its the:
> | set_cpu_online(cpu, false);
> you're referring to here.
> 
> Would 'marks the CPU offline' be clearer?

Yes, I will update the change log. I'll wait and see if there are other
comments as well, and send a new version next week.

> Regardless,
> Reviewed-by: James Morse <james.morse@arm.com>

Thanks,

A.
Will Deacon June 13, 2019, 9:21 a.m. UTC | #3
On Tue, Jun 11, 2019 at 09:10:50PM +0300, Aaro Koskinen wrote:
> From: Aaro Koskinen <aaro.koskinen@nokia.com>
> 
> Currently arm64 uses the default implementation of panic_smp_self_stop()
> that is simply a cpu_relax() loop. As a result, when two CPUs panic()
> simultaneously we get "SMP: failed to stop secondary CPUs" warnings and
> extra delays before a reset.

Please can you elaborate a bit on this in the commit message (and preferably
a comment in panic_smp_self_stop() justifying the need for our own
implementation)? I initially thought it was something like two CPUs trying
to IPI each other, but it's much simpler than that.

> Provide an implementation of panic_smp_self_stop() that offlines the
> CPU properly.
> 
> Signed-off-by: Aaro Koskinen <aaro.koskinen@nokia.com>
> ---
>  arch/arm64/kernel/smp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index 1a1b96a50245..5e09e5032409 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -857,6 +857,11 @@ static void ipi_cpu_stop(unsigned int cpu)
>  	cpu_park_loop();
>  }
>  
> +void panic_smp_self_stop(void)
> +{
> +	ipi_cpu_stop(smp_processor_id());
> +}

ipi_cpu_stop should really be void, and just do smp_processor_id() itself.
It clearly only works for the local CPU. I'd be ok with you folding that
change in here, unless you fancy an extra patch. Maybe rename the function
too, now that it doesn't always run in interrupt context.

Cheers,

Will

Patch
diff mbox series

diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index 1a1b96a50245..5e09e5032409 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -857,6 +857,11 @@  static void ipi_cpu_stop(unsigned int cpu)
 	cpu_park_loop();
 }
 
+void panic_smp_self_stop(void)
+{
+	ipi_cpu_stop(smp_processor_id());
+}
+
 #ifdef CONFIG_KEXEC_CORE
 static atomic_t waiting_for_crash_ipi = ATOMIC_INIT(0);
 #endif