Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
On Fri, Nov 20, 2015 at 06:36:46PM +0900, Hidehiro Kawai wrote:
> nmi_shootdown_cpus(), a subroutine of crash_kexec(), sends NMI IPI
> to non-panic cpus to stop them while saving their register
...to stop them and save their register...
> information and doing some cleanups for crash dumping. So if a
> non-panic cpus is infinitely looping in NMI context, we fail to
That should be CPU. Please use "CPU" instead of "cpu" in all your text
in your next submission.
> save its register information and lose the information from the
> crash dump.
>
> `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.
> This can
> happen on some servers which broadcasts NMIs to all CPUs when a dump
> button is pushed.
>
> 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().
> 2. call a callback of nmi_shootdown_cpus() directly to save
>registers and do some cleanups after setting waiting_for_crash_ipi
>which is used for counting down the number of cpus which handled
>the callback
>
> V5:
> - Use WRITE_ONCE() when setting crash_ipi_done to 1 so that the
> compiler doesn't change the instruction order
> - Support the case of b in the above description
> - Add poll_crash_ipi_and_callback()
>
> V4:
> - Rewrite the patch description
>
> V3:
> - Newly introduced
>
> Signed-off-by: Hidehiro Kawai
> Cc: Andrew Morton
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: "H. Peter Anvin"
> Cc: Peter Zijlstra
> Cc: Eric Biederman
> Cc: Vivek Goyal
> Cc: Michal Hocko
> ---
> arch/x86/include/asm/reboot.h |1 +
> arch/x86/kernel/nmi.c | 17 +
> arch/x86/kernel/reboot.c | 28
> include/linux/kernel.h| 12 ++--
> kernel/panic.c| 10 ++
> kernel/watchdog.c |2 +-
> 6 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.h
> index a82c4f1..964e82f 100644
> --- a/arch/x86/include/asm/reboot.h
> +++ b/arch/x86/include/asm/reboot.h
> @@ -25,5 +25,6 @@ void __noreturn machine_real_restart(unsigned int type);
>
> typedef void (*nmi_shootdown_cb)(int, struct pt_regs*);
> void nmi_shootdown_cpus(nmi_shootdown_cb callback);
> +void poll_crash_ipi_and_callback(struct pt_regs *regs);
>
> #endif /* _ASM_X86_REBOOT_H */
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 5131714..74a1434 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #define CREATE_TRACE_POINTS
> #include
> @@ -231,7 +232,7 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
> #endif
>
> if (panic_on_unrecovered_nmi)
> - nmi_panic("NMI: Not continuing");
> + nmi_panic(regs, "NMI: Not continuing");
>
> pr_emerg("Dazed and confused, but trying to continue\n");
>
> @@ -256,7 +257,7 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
> show_regs(regs);
>
> if (panic_on_io_nmi) {
> - nmi_panic("NMI IOCK error: Not continuing");
> + nmi_panic(regs, "NMI IOCK error: Not continuing");
>
> /*
>* If we return from nmi_panic(), it means we have received
> @@ -305,7 +306,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs
> *regs)
>
> pr_emerg("Do you have a strange power saving mode enabled?\n");
> if (unknown_nmi_panic || panic_on_unrecovered_nmi)
> - nmi_panic("NMI: Not continuing");
> + nmi_panic(regs, "NMI: Not continuing");
>
> pr_emerg("Dazed and confused, but trying to continue\n");
> }
> @@ -357,7 +358,15 @@ static void default_do_nmi(struct pt_regs *regs)
> }
>
> /* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> - raw_spin_lock(&nmi_reason_lock);
> +
> + /*
> + * Another CPU may be processing panic routines with holding
while
> + * nmi_reason_lock. Check IPI issuance from the panicking CPU
> + * and call the callback directly.
This is one strange sentence. Does it mean something like:
"Check if the panicking CPU issued the IPI and, if so, call the cra
Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
On Fri 20-11-15 18:36:46, Hidehiro Kawai wrote:
> nmi_shootdown_cpus(), a subroutine of crash_kexec(), sends NMI IPI
> to non-panic cpus to stop them while saving their register
> information and doing some cleanups for crash dumping. So if a
> non-panic cpus is infinitely looping in NMI context, we fail to
> save its register information and lose the information from the
> crash dump.
>
> `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. This can
> happen on some servers which broadcasts NMIs to all CPUs when a dump
> button is pushed.
>
> 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
> 2. call a callback of nmi_shootdown_cpus() directly to save
>registers and do some cleanups after setting waiting_for_crash_ipi
>which is used for counting down the number of cpus which handled
>the callback
>
> V5:
> - Use WRITE_ONCE() when setting crash_ipi_done to 1 so that the
> compiler doesn't change the instruction order
> - Support the case of b in the above description
> - Add poll_crash_ipi_and_callback()
>
> V4:
> - Rewrite the patch description
>
> V3:
> - Newly introduced
>
> Signed-off-by: Hidehiro Kawai
> Cc: Andrew Morton
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: "H. Peter Anvin"
> Cc: Peter Zijlstra
> Cc: Eric Biederman
> Cc: Vivek Goyal
> Cc: Michal Hocko
Yes this seems correct
Acked-by: Michal Hocko
> ---
> arch/x86/include/asm/reboot.h |1 +
> arch/x86/kernel/nmi.c | 17 +
> arch/x86/kernel/reboot.c | 28
> include/linux/kernel.h| 12 ++--
> kernel/panic.c| 10 ++
> kernel/watchdog.c |2 +-
> 6 files changed, 63 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.h
> index a82c4f1..964e82f 100644
> --- a/arch/x86/include/asm/reboot.h
> +++ b/arch/x86/include/asm/reboot.h
> @@ -25,5 +25,6 @@ void __noreturn machine_real_restart(unsigned int type);
>
> typedef void (*nmi_shootdown_cb)(int, struct pt_regs*);
> void nmi_shootdown_cpus(nmi_shootdown_cb callback);
> +void poll_crash_ipi_and_callback(struct pt_regs *regs);
>
> #endif /* _ASM_X86_REBOOT_H */
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index 5131714..74a1434 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -29,6 +29,7 @@
> #include
> #include
> #include
> +#include
>
> #define CREATE_TRACE_POINTS
> #include
> @@ -231,7 +232,7 @@ pci_serr_error(unsigned char reason, struct pt_regs *regs)
> #endif
>
> if (panic_on_unrecovered_nmi)
> - nmi_panic("NMI: Not continuing");
> + nmi_panic(regs, "NMI: Not continuing");
>
> pr_emerg("Dazed and confused, but trying to continue\n");
>
> @@ -256,7 +257,7 @@ io_check_error(unsigned char reason, struct pt_regs *regs)
> show_regs(regs);
>
> if (panic_on_io_nmi) {
> - nmi_panic("NMI IOCK error: Not continuing");
> + nmi_panic(regs, "NMI IOCK error: Not continuing");
>
> /*
>* If we return from nmi_panic(), it means we have received
> @@ -305,7 +306,7 @@ unknown_nmi_error(unsigned char reason, struct pt_regs
> *regs)
>
> pr_emerg("Do you have a strange power saving mode enabled?\n");
> if (unknown_nmi_panic || panic_on_unrecovered_nmi)
> - nmi_panic("NMI: Not continuing");
> + nmi_panic(regs, "NMI: Not continuing");
>
> pr_emerg("Dazed and confused, but trying to continue\n");
> }
> @@ -357,7 +358,15 @@ static void default_do_nmi(struct pt_regs *regs)
> }
>
> /* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
> - raw_spin_lock(&nmi_reason_lock);
> +
> + /*
> + * Another CPU may be processing panic routines with holding
> + * nmi_reason_lock. Check IPI issuance from the panicking CPU
> + * and call the callback directly.
> + */
> + while (!raw_spin_trylock(&nmi_reason_lock))
> + poll_crash_ipi_and_callback(regs);
> +
> reason = x86_platform.get_nmi_reason();
>
> if (reason & NMI_REASON_MASK) {
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 02693dd..44c5f5b 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -718,6 +718,7 @@ static int crashing_cpu;
> static nmi_shootdown_cb shootdown_callback;
>
> static atomic_t waiting_for_crash_ipi;
> +static int crash_ipi_don
Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
On Tue, Nov 24, 2015 at 11:48:53AM +0100, Borislav Petkov wrote:
>
> > +*/
> > + 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. :-\
This only does something if crash_ipi_done is set, which means you are killing
the box. But perhaps a comment that states that here would be useful, or maybe
just put in the check here. There's no need to make it depend on SMP, as
raw_spin_trylock() will turn to just ({1}) for UP, and that code wont even be
hit.
-- Steve
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
On Tue, Nov 24, 2015 at 02:37:00PM -0500, Steven Rostedt wrote:
> On Tue, Nov 24, 2015 at 11:48:53AM +0100, Borislav Petkov wrote:
> >
> > > + */
> > > + 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. :-\
>
> This only does something if crash_ipi_done is set, which means you are killing
> the box.
Yeah, Michal and I discussed that on IRC today. And yeah, it is really
tricky stuff. So I appreciate it a lot you looking at it too. Thanks!
> But perhaps a comment that states that here would be useful, or maybe
> just put in the check here. There's no need to make it depend on SMP, as
> raw_spin_trylock() will turn to just ({1}) for UP, and that code wont even be
> hit.
Right, this code needs much more thorough documentation to counter the
trickiness.
Thanks.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
RE: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
> On Fri, Nov 20, 2015 at 06:36:46PM +0900, Hidehiro Kawai wrote: > > nmi_shootdown_cpus(), a subroutine of crash_kexec(), sends NMI IPI > > to non-panic cpus to stop them while saving their register > >...to stop them and save their register... Thanks for the correction. > > information and doing some cleanups for crash dumping. So if a > > non-panic cpus is infinitely looping in NMI context, we fail to > > That should be CPU. Please use "CPU" instead of "cpu" in all your text > in your next submission. OK, I'll fix that. > > save its register information and lose the information from the > > crash dump. > > > > `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 > > This can > > happen on some servers which broadcasts NMIs to all CPUs when a dump > > button is pushed. > > > > 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. > > 2. call a callback of nmi_shootdown_cpus() directly to save > >registers and do some cleanups after setting waiting_for_crash_ipi > >which is used for counting down the number of cpus which handled > >the callback > > > > V5: > > - Use WRITE_ONCE() when setting crash_ipi_done to 1 so that the > > compiler doesn't change the instruction order > > - Support the case of b in the above description > > - Add poll_crash_ipi_and_callback() > > > > V4: > > - Rewrite the patch description > > > > V3: > > - Newly introduced > > > > Signed-off-by: Hidehiro Kawai > > Cc: Andrew Morton > > Cc: Thomas Gleixner > > Cc: Ingo Molnar > > Cc: "H. Peter Anvin" > > Cc: Peter Zijlstra > > Cc: Eric Biederman > > Cc: Vivek Goyal > > Cc: Michal Hocko > > --- > > arch/x86/include/asm/reboot.h |1 + > > arch/x86/kernel/nmi.c | 17 + > > arch/x86/kernel/reboot.c | 28 > > include/linux/kernel.h| 12 ++-- > > kernel/panic.c| 10 ++ > > kernel/watchdog.c |2 +- > > 6 files changed, 63 insertions(+), 7 deletions(-) > > > > diff --git a/arch/x86/include/asm/reboot.h b/arch/x86/include/asm/reboot.h > > index a82c4f1..964e82f 100644 > > --- a/arch/x86/include/asm/reboot.h > > +++ b/arch/x86/include/asm/reboot.h > > @@ -25,5 +25,6 @@ void __noreturn machine_real_restart(unsigned int type); > > > > typedef void (*nmi_shootdown_cb)(int, struct pt_regs*); > > void nmi_shootdown_cpus(nmi_shootdown_cb callback); > > +void poll_crash_ipi_and_callback(struct pt_regs *regs); > > > > #endif /* _ASM_X86_REBOOT_H */ > > diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c > > index 5131714..74a1434 100644 > > --- a/arch/x86/kernel/nmi.c > > +++ b/arch/x86/kernel/nmi.c > > @@ -29,6 +29,7 @@ > > #include > > #include > > #include > > +#include > > > > #defin
RE: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
> On Tue, Nov 24, 2015 at 11:48:53AM +0100, Borislav Petkov wrote:
> >
> > > + */
> > > + 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. :-\
>
> This only does something if crash_ipi_done is set, which means you are killing
> the box. But perhaps a comment that states that here would be useful, or maybe
> just put in the check here.
OK, I'll add more comments around this.
> There's no need to make it depend on SMP, as
> raw_spin_trylock() will turn to just ({1}) for UP, and that code wont even be
> hit.
I'll integrate these SMP and UP versions with a comment about
that.
Regards,
--
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
On Wed, Nov 25, 2015 at 05:51:59AM +, 河合英宏 / 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. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
RE: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
> On Wed, Nov 25, 2015 at 05:51:59AM +, 河合英宏 / 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. Does *old* code mean the code without this patch *series* ? panic_cpu and nmi_panic() are introduced by PATCH 1/4, not this patch. > 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: ... Good suggestion! I'll revise a bit with following your comment. > > > > + * 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"? I prefer this, but we might want to add some more prefix or suffix. For example, "conditionally_run_crash_nmi_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 for your deep review! -- Hidehiro Kawai Hitachi, Ltd. Research & Development Group ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
On Wed, Nov 25, 2015 at 09:46:37AM +, 河合英宏 / KAWAI，HIDEHIRO wrote: > Does *old* code mean the code without this patch *series* ? Yes. > I prefer this, but we might want to add some more prefix or suffix. > For example, "conditionally_run_crash_nmi_callback". That's unnecessary IMO. If you need to express that, you could write that in a comment above the function definition. Anyone who looks at the code then will know that it is conditional, like so many other kernel functions. :) > Thanks for your deep review! Thanks for the patience! :-) -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
RE: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
> On Wed, Nov 25, 2015 at 09:46:37AM +, 河合英宏 / KAWAI，HIDEHIRO wrote: ... > > I prefer this, but we might want to add some more prefix or suffix. > > For example, "conditionally_run_crash_nmi_callback". > > That's unnecessary IMO. If you need to express that, you could write > that in a comment above the function definition. Anyone who looks at > the code then will know that it is conditional, like so many other > kernel functions. :) OK, I'll use a simple one with a comment. Regards, -- Hidehiro Kawai Hitachi, Ltd. Research & Development Group ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
RE: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context
> @@ -357,7 +358,15 @@ static void default_do_nmi(struct pt_regs *regs) > } > > /* Non-CPU-specific NMI: NMI sources can be processed on any CPU */ > - raw_spin_lock(&nmi_reason_lock); > + > + /* > + * Another CPU may be processing panic routines with holding > + * nmi_reason_lock. Check IPI issuance from the panicking CPU > + * and call the callback directly. > + */ > + while (!raw_spin_trylock(&nmi_reason_lock)) > + poll_crash_ipi_and_callback(regs); > + > reason = x86_platform.get_nmi_reason(); I noticed this logic is unneeded until applying PATCH 4/4. Currently, unknown NMI can be broadcast to all CPUs, but in that case, panic()/nmi_panic() are called after releasing nmi_reason_lock. So CPUs can't loop infinitely here. PATCH 4/4 allows us to broadcast external NMIs to all CPUs, and it causes infinite loop in raw_spin_lock(&nmi_reason_lock). So the above changes are needed. I'll move these chagnes to a later patch in the next version. Thanks, -- Hidehiro Kawai Hitachi, Ltd. Research & Development Group ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
