| Message ID | 20190611181050.9647-2-aaro.koskinen@iki.fi (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] arm64: Improve parking of stopped CPUs | expand |
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
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.
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
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