RE: [V4 PATCH 2/2] mips/panic: Replace smp_send_stop() with kdump friendly version in panic path

[æåèå / KAWAIïHIDEHIRO]

> From: Corey Minyard [mailto:cminyard@xxxxxxxxxx]
> Sent: Tuesday, August 16, 2016 3:02 AM
> On 08/15/2016 12:06 PM, Corey Minyard wrote:
> > On 08/15/2016 06:35 AM, æåèå / KAWAIïHIDEHIRO wrote:
> >> Hi Corey,
> >>
> >>> From: Corey Minyard [mailto:cminyard@xxxxxxxxxx]
> >>> Sent: Friday, August 12, 2016 10:56 PM
> >>> I'll try to test this, but I have one comment inline...
> >> Thank you very much!
> >>
> >>> On 08/11/2016 10:17 PM, Dave Young wrote:
> >>>> On 08/10/16 at 05:09pm, Hidehiro Kawai wrote:
> >> [snip]
> >>>>> diff --git a/arch/mips/kernel/crash.c b/arch/mips/kernel/crash.c
> >>>>> index 610f0f3..1723b17 100644
> >>>>> --- a/arch/mips/kernel/crash.c
> >>>>> +++ b/arch/mips/kernel/crash.c
> >>>>> @@ -47,9 +47,14 @@ static void crash_shutdown_secondary(void
> >>>>> *passed_regs)
> >>>>>
> >>>>>    static void crash_kexec_prepare_cpus(void)
> >>>>>    {
> >>>>> +    static int cpus_stopped;
> >>>>>        unsigned int msecs;
> >>>>> +    unsigned int ncpus;
> >>>>>
> >>>>> -    unsigned int ncpus = num_online_cpus() - 1;/* Excluding the
> >>>>> panic cpu */
> >>>>> +    if (cpus_stopped)
> >>>>> +        return;
> >>> Wouldn't you want an atomic operation and some special handling here to
> >>> ensure that only one CPU does this?  So if a CPU comes in here and
> >>> another CPU is already in the process stopping the CPUs it won't
> >>> result in a
> >>> deadlock.
> >> Because this function can be called only one panicking CPU,
> >> there is no problem.
> >>
> >> There are two paths which crash_kexec_prepare_cpus is called.
> >>
> >> Path 1 (panic path):
> >> panic()
> >>    crash_smp_send_stop()
> >>      crash_kexec_prepare_cpus()
> >>
> >> Path 2 (oops path):
> >> crash_kexec()
> >>    __crash_kexec()
> >>      machine_crash_shutdown()
> >>        default_machine_crash_shutdown() // for MIPS
> >>          crash_kexec_prepare_cpus()
> >>
> >> Here, panic() and crash_kexec() run exclusively via
> >> panic_cpu atomic variable.  So we can use cpus_stopped as
> >> normal variable.
> >
> > Ok, if the code can only be entered once, what's the purpose of
> > cpus_stopped?
> > I guess that's what confused me.  You are right, the panic_cpu atomic
> > should
> > keep this on a single CPU.
> 
> Never mind, I see the path through panic() where that is required. My
> question
> below still remains, though.
> 
> > Also, panic() will call panic_smp_self_stop() if it finds another CPU
> > has already
> > called panic, which will just spin with interrupts off by default. I
> > didn't see a
> > definition for it in MIPS, wouldn't it need to be overridden to avoid
> > a deadlock?

No deadlock should happen. Panicking CPU calls crash_kexec_prepare_cpus(),
and it issues an IPI and wait for other CPUs handle it.  If some of them
are looping in panic_smp_self_stop() with interrupt disabled, they can't
handle the IPI.  But it's not a severe problem.  crash_kexec_prepare_cpus()
has a timeout mechanism, and it will go out from the wait loop when it
happens.

In that case, of course, their registers are not saved.  This could be
improved, but I'd like to entrust MIPS experts with the improvement.
This is another issue.

Best regards,

Hidehiro Kawai