[V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
河合英宏 / KAWAI，HIDEHIRO
hidehiro.kawai.ez at hitachi.com
Mon Jul 11 19:49:12 PDT 2016
Hi Dave,
Thanks for the comments.
> From: Dave Young [mailto:dyoung at redhat.com]
> Sent: Monday, July 11, 2016 5:35 PM
>
> On 07/05/16 at 08:33pm, Hidehiro Kawai wrote:
> > This patch fixes one of the problems reported by Daniel Walker
> > (https://lkml.org/lkml/2015/6/24/44).
> >
> > If crash_kexec_post_notifiers boot option is specified, other CPUs
> > are stopped by smp_send_stop() instead of machine_crash_shutdown()
> > in crash_kexec() path. This behavior change leads two problems.
> >
> > Problem 1:
> > octeon_generic_shutdown() for MIPS OCTEON assumes that other CPUs are
> > still online and try to stop their watchdog timer. If
> > smp_send_stop() is called before octeon_generic_shutdown(), stopping
> > watchdog timer will fail because other CPUs have been offlined by
> > smp_send_stop().
> >
> > panic()
> > if crash_kexec_post_notifiers == 1
> > smp_send_stop()
> > atomic_notifier_call_chain()
> > kmsg_dump()
> > crash_kexec()
> > machine_crash_shutdown()
> > octeon_generic_shutdown() // shutdown watchdog for ONLINE CPUs
> >
> > Problem 2:
> > Most of architectures stop other CPUs in machine_crash_shutdown()
> > path, and they also do something needed for kdump. For example,
> > they save registers, disable virtualization extensions, and so on.
> > However, if smp_send_stop() stops other CPUs before
> > machine_crash_shutdown(), we miss those operations.
> >
> > How do we fix these problems? In the first place, we should stop
> > other CPUs as soon as possible when panic() was called, otherwise
> > other CPUs may wipe out a clue to the cause of the failure. So, we
> > replace smp_send_stop() with more suitable one for kdump.
>
> We have been avoiding extra things in panic path, but unfortunately
> crash_kexec_post_notifiers were added. I tend to agree the best place
> for this stuff is in 2nd kernel or purgatory instead of in 1st kernel.
Several months ago, I posted a patch set which writes regs to SEL, generate
an event to send SNMP message, and start/stop BMC's watchdog timer in
purgatory. This feature requires BMC with KCS (Keyboard Controller Style)
I/F, but the most of enterprise grade server would have it.
(http://thread.gmane.org/gmane.linux.kernel.kexec/15382)
Doing kmsg_dump things in purgatory wouldn't be suitable (should be done
in the 2nd kernel before enabling devices and IRQs?)
> As for this patch I'm not sure it is safe to replace the smp_send_stop
> with the kdump friendly function. I'm also not sure if the kdump friendly
> function is safe for kdump. Will glad to hear opinions from other
> arch experts.
This stuff depends on architectures, so I speak only about
x86 (the logic doesn't change on other architectures at this time).
kdump path with crash_kexec_post_notifiers disabled:
panic()
__crash_kexec()
crash_setup_regs()
crash_save_vmcoreinfo()
machine_crash_shutdown()
native_machine_crash_shutdown()
panic_smp_send_stop() /* mostly same as original
* kdump_nmi_shootdown_cpus()
*/
kdump path with crash_kexec_post_notifiers enabled:
panic()
panic_smp_send_stop()
__crash_kexec()
crash_setup_regs()
crash_save_vmcoreinfo()
machine_crash_shutdown()
native_machine_crash_shutdown()
panic_smp_send_stop() // do nothing
The difference is that stopping other CPUs before crash_setup_regs()
and crash_save_vmcoreinfo() or not. Since crash_setup_regs() and
crash_save_vmcoreinfo() just save information to some memory area,
they wouldn't be affected by panic_smp_send_stop(). This means
placing panic_smp_send_stop before __crash_kexec is safe.
BTW, I noticed my patch breaks Xen kernel. I'll fix it in the
next version.
> BTW, if one want to use crash_kexec_post_notifiers he should take the
> risk of unreliable kdump. How about only call smp_send_stop in case no
> crash_kexec_post_notifiers being used.
Unlike panic_smp_send_stop()/kdump_nmi_shootdown_cpus(), smp_send_stop()
for x86 tries to stop other CPUs with normal IPI before issuing NMI IPI.
This would be because NMI IPI has a risk of deadlock. We checked if
the kdump path has a risk of deadlock in the case of NMI panic and fixed
it. But I'm not sure about normal panic path. I agree with that use
smp_send_stop if crash_kexec_post_notifiers or kdump is disabled.
> > This patch solves Problem 2 by replacing smp_send_stop() in panic()
> > with panic_smp_send_stop(). This is a weak function which calls
> > smp_send_stop(), and architecture dependent code may override this
> > with appropriate one. This patch only provides x86-specific version.
>
> It does not fix the Problem 1, it seem not possible to fix it?
Problem 1 depends on architectures, and at least it doesn't happen
on x86. I can try to fix the Problem 1 for MIPS, but I can't test it.
Possible solution will be to use an smp_send_stop variant which stop
the CPU without offlining.
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
> > Changes in V3:
> > - Revise comments, description, and symbol names
> >
> > Changes in V2:
> > - Replace smp_send_stop() call with crash_kexec version which
> > saves cpu states and cleans up VMX/SVM
> > - Drop a fix for Problem 1 at this moment
> >
> > Reported-by: Daniel Walker <dwalker at fifo99.com>
> > Fixes: f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)
> > Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez at hitachi.com>
> > Cc: Dave Young <dyoung at redhat.com>
> > Cc: Baoquan He <bhe at redhat.com>
> > Cc: Vivek Goyal <vgoyal at redhat.com>
> > Cc: Eric Biederman <ebiederm at xmission.com>
> > Cc: Masami Hiramatsu <mhiramat at kernel.org>
> > Cc: Thomas Gleixner <tglx at linutronix.de>
> > Cc: Ingo Molnar <mingo at redhat.com>
> > Cc: "H. Peter Anvin" <hpa at zytor.com>
> > Cc: Borislav Petkov <bp at suse.de>
> > Cc: Toshi Kani <toshi.kani at hpe.com>
> > Cc: "Peter Zijlstra (Intel)" <peterz at infradead.org>
> > Cc: Takao Indoh <indou.takao at jp.fujitsu.com>
> > Cc: "Lee, Chun-Yi" <joeyli.kernel at gmail.com>
> > Cc: Minfei Huang <mnfhuang at gmail.com>
> > Cc: Andrew Morton <akpm at linux-foundation.org>
> > Cc: Michal Hocko <mhocko at suse.com>
> > Cc: Vitaly Kuznetsov <vkuznets at redhat.com>
> > Cc: Petr Mladek <pmladek at suse.com>
> > Cc: Tejun Heo <tj at kernel.org>
> > Cc: Josh Poimboeuf <jpoimboe at redhat.com>
> > ---
> > arch/x86/kernel/crash.c | 14 ++++++++++----
> > kernel/panic.c | 43 ++++++++++++++++++++++++++++++++-----------
> > 2 files changed, 42 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> > index 9ef978d..3305433 100644
> > --- a/arch/x86/kernel/crash.c
> > +++ b/arch/x86/kernel/crash.c
> > @@ -133,15 +133,21 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)
> > disable_local_APIC();
> > }
> >
> > -static void kdump_nmi_shootdown_cpus(void)
> > +/* Override the weak function in kernel/panic.c */
> > +void panic_smp_send_stop(void)
> > {
> > - nmi_shootdown_cpus(kdump_nmi_callback);
> > + static int cpus_stopped;
> > +
> > + if (cpus_stopped)
> > + return;
> >
> > + nmi_shootdown_cpus(kdump_nmi_callback);
> > disable_local_APIC();
> > + cpus_stopped = 1;
> > }
> >
> > #else
> > -static void kdump_nmi_shootdown_cpus(void)
> > +void panic_smp_send_stop(void)
> > {
> > /* There are no cpus to shootdown */
> > }
> > @@ -160,7 +166,7 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
> > /* The kernel is broken so disable interrupts */
> > local_irq_disable();
> >
> > - kdump_nmi_shootdown_cpus();
> > + panic_smp_send_stop();
> >
> > /*
> > * VMCLEAR VMCSs loaded on this cpu if needed.
> > diff --git a/kernel/panic.c b/kernel/panic.c
> > index 8aa7449..da8062d2 100644
> > --- a/kernel/panic.c
> > +++ b/kernel/panic.c
> > @@ -71,6 +71,32 @@ void __weak nmi_panic_self_stop(struct pt_regs *regs)
> > panic_smp_self_stop();
> > }
> >
> > +/*
> > + * Stop other CPUs in panic. Architecture dependent code may override this
> > + * with more suitable version. For example, if the architecture supports
> > + * crash dump, it should save registers of each stopped CPU and disable
> > + * per-CPU features such as virtualization extensions.
> > + */
> > +void __weak panic_smp_send_stop(void)
> > +{
> > + static int cpus_stopped;
> > +
> > + /*
> > + * This function can be called twice in panic path, but obviously
> > + * we execute this only once.
> > + */
> > + if (cpus_stopped)
> > + return;
> > +
> > + /*
> > + * Note smp_send_stop is the usual smp shutdown function, which
> > + * unfortunately means it may not be hardened to work in a panic
> > + * situation.
> > + */
> > + smp_send_stop();
> > + cpus_stopped = 1;
> > +}
> > +
> > atomic_t panic_cpu = ATOMIC_INIT(PANIC_CPU_INVALID);
> >
> > /*
> > @@ -125,7 +151,7 @@ void panic(const char *fmt, ...)
> > * Only one CPU is allowed to execute the panic code from here. For
> > * multiple parallel invocations of panic, all other CPUs either
> > * stop themself or will wait until they are stopped by the 1st CPU
> > - * with smp_send_stop().
> > + * with panic_smp_send_stop().
> > *
> > * `old_cpu == PANIC_CPU_INVALID' means this is the 1st CPU which
> > * comes here, so go ahead.
> > @@ -165,12 +191,7 @@ void panic(const char *fmt, ...)
> > __crash_kexec(NULL);
> > }
> >
> > - /*
> > - * Note smp_send_stop is the usual smp shutdown function, which
> > - * unfortunately means it may not be hardened to work in a panic
> > - * situation.
> > - */
> > - smp_send_stop();
> > + panic_smp_send_stop();
> >
> > /*
> > * Run any panic handlers, including those that might need to
> > @@ -198,10 +219,10 @@ void panic(const char *fmt, ...)
> >
> > /*
> > * We may have ended up stopping the CPU holding the lock (in
> > - * smp_send_stop()) while still having some valuable data in the console
> > - * buffer. Try to acquire the lock then release it regardless of the
> > - * result. The release will also print the buffers out. Locks debug
> > - * should be disabled to avoid reporting bad unlock balance when
> > + * panic_smp_send_stop()) while still having some valuable data in the
> > + * console buffer. Try to acquire the lock then release it regardless
> > + * of the result. The release will also print the buffers out. Locks
> > + * debug should be disabled to avoid reporting bad unlock balance when
> > * panic() is not being callled from OOPS.
> > */
> > debug_locks_off();
> >
> >
> >
> > _______________________________________________
> > kexec mailing list
> > kexec at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
>
> Thanks
> Dave
More information about the kexec
mailing list