Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context

[Borislav Petkov <bp@alien8.de>]

On Wed, Nov 25, 2015 at 05:51:59AM +0000, 河合英宏 / KAWAI，HIDEHIRO wrote:
> > > `Infinite loop in NMI context' can happen:
> > >
> > >   a. when a cpu panics on NMI while another cpu is processing panic
> > >   b. when a cpu received an external or unknown NMI while another
> > >      cpu is processing panic on NMI
> > >
> > > In the case of a, it loops in panic_smp_self_stop().  In the case
> > > of b, it loops in raw_spin_lock() of nmi_reason_lock.
> > 
> > Please describe those two cases more verbosely - it takes slow people
> > like me a while to figure out what exactly can happen.
> 
>   a. when a cpu panics on NMI while another cpu is processing panic
>      Ex.
>      CPU 0                     CPU 1
>      =================         =================
>      panic()
>        panic_cpu <-- 0
>        check panic_cpu
>        crash_kexec()
>                                receive an unknown NMI
>                                unknown_nmi_error()
>                                  nmi_panic()
>                                    panic()
>                                      check panic_cpu
>                                      panic_smp_self_stop()
>                                        infinite loop in NMI context
> 
>   b. when a cpu received an external or unknown NMI while another
>      cpu is processing panic on NMI
>      Ex.
>      CPU 0                     CPU 1
>      ======================    ==================
>      receive an unknown NMI
>      unknown_nmi_error()
>        nmi_panic()             receive an unknown NMI
>          panic_cpu <-- 0       unknown_nmi_error()
>          panic()                 nmi_panic()
>            check panic_cpu         panic
>            crash_kexec()             check panic_cpu
>                                      panic_smp_self_stop()
>                                        infinite loop in NMI context

Ok, that's what I saw too, thanks for confirming.

But please write those examples with the *old* code in the commit
message, i.e. without panic_cpu and nmi_panic() which you're adding.

Basically, you want to structure your commit message this way:

This is the problem the current code has: ...

But we need to do this: ...

We fix it by doing that: ...

This will be of great help now when reading the commit message and of
invaluable help later, when we all have forgotten about the issue and
are scratching heads over why stuff was added.

> > > To save registers in these case too, this patch does following things:
> > >
> > > 1. Move the timing of `infinite loop in NMI context' (actually
> > >    done by panic_smp_self_stop()) outside of panic() to enable us to
> > >    refer pt_regs
> > 
> > I can't parse that sentence. And I really tried :-\
> > panic_smp_self_stop() is still in panic().
> 
> panic_smp_self_stop() is still used when a CPU in normal context
> should go into infinite loop.  Only when a CPU is in NMI context,
> nmi_panic_self_stop() is called and the CPU loops infinitely
> without entering panic().
> 
> I'll try to revise this sentense.

FWIW, it sounds better already! :)

> > > +	 */
> > > +	while (!raw_spin_trylock(&nmi_reason_lock))
> > > +		poll_crash_ipi_and_callback(regs);
> > 
> > Waaait a minute: so if we're getting NMIs broadcasted on every core but
> > we're *not* crash dumping, we will run into here too. This can't be
> > right. :-\
> 
> As Steven commented, poll_crash_ipi_and_callback() does nothing
> if we're not crash dumping.  But yes, this is confusing.  I'll add
> more detailed comment, or change the logic a bit if I come up with
> better one.

Thanks, much appreciated!

> > > +/*
> > > + * Wait for the timing of IPI for crash dumping, and then call its callback
> > 
> > "Wait for the crash dumping IPI to complete... "
> 
> So, I think "Wait for the crash dumping IPI to be issued..." is better.
> "complete" would be ambiguous in this context.

Ok.

> 
> > > + * directly.  This function is used when we have already been in NMI handler.
> > > + */
> > > +void poll_crash_ipi_and_callback(struct pt_regs *regs)
> > 
> > Why "poll"? We won't return from crash_nmi_callback() if we're not the
> > crashing CPU.
> 
> This function polls that crash IPI has been issued by checking
> crash_ipi_done, then invokes the callback.  This is different
> from so-called "poll" functions.  Do you have some good name?

Maybe something as simple as "run_crash_callback"?

Or since we're calling it from other places, maybe add the "crash"
prefix:

	while (!raw_spin_trylock(&nmi_reason_lock))
		crash_run_callback(regs);

Looks even better to me in code context. :)

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.