kexec: crash_kexec_post_notifiers boot option related fixes
Subsystems (3)
v3 cover kexec: crash_kexec_post_notifiers boot option related fixes
This is the update version of the patch which I posted one year ago
(https://lkml.org/lkml/2015/7/23/864). Because I couldn't received positive opinions against the previous version, I had changed the approach to another one which utilizes kexec purgatory. Then I'm back here.
PATCH 1/2 is a bugfix for crash_kexec_post_notifiers boot option which allows users to call panic notifiers and kmsg dumpers before kdump. When this option is enabled, smp_send_stop() is used to stop other CPUs instead of a function provided by kdump. smp_send_stop() just stops other CPUs and doesn't other things needed for kdump. So, fix it by replacing smp_send_stop() with new appropriate one, panic_smp_send_stop(). Since panic_smp_send_stop() is architecture specific and I can't simply copy from kdump routines, only x86-specific version is provided.
PATCH 2/2 makes crash_kexec_post_notifiers boot option modifiable after boot. We don't need to limit it to being modifiable at boot time.
Changes in V3:
- Revise comments, description, and symbol names (the logic doesn't change)
- Make crash_kexec_post_notifiers boot option modifiable after boot
Changes in V2: https://lkml.org/lkml/2015/7/23/864
- Replace smp_send_stop() call with crash_kexec version which saves cpu states and does cleanups instead of changing execution flow
- Drop a fix for Problem 1
- Drop other patches because they aren't needed anymore
V1: https://lkml.org/lkml/2015/7/10/316
---
Hidehiro Kawai (2):
x86/panic: Replace smp_send_stop() with kdump friendly version
kexec: Use core_param for crash_kexec_post_notifiers boot option
arch/x86/kernel/crash.c | 14 ++++++++---- kernel/panic.c | 56 ++++++++++++++++++++++++++++++----------------- 2 files changed, 46 insertions(+), 24 deletions(-)
Signature · 9 lines
Hidehiro Kawai Hitachi, Ltd. Research & Development Group _______________________________________________ kexec mailing list [email protected] http://lists.infradead.org/mailman/listinfo/kexec
PATCH v3 1/2 x86/panic: Replace smp_send_stop() with kdump friendly version
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.
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.
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
--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)
arch/x86/kernel/crash.c +10 -4
@@ -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.
kernel/panic.c +32 -11
67 lines not on this page · raw message
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Quoted 30 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_crash_shutdown(), we miss those operations.
Quoted 4 lines
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 replacesmp_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.
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.
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.
Quoted 5 lines
This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_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?
Quoted 153 lines
Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +10
-4@@ -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. kernel/panic.c +32
-11@@ -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 ();
Quoted 4 lines
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Thanks
Dave
RE: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Hi Dave,
Thanks for the comments.
Quoted 34 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMQuoted 30 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_crash_shutdown(), we miss those operations.
Quoted 8 lines
Quoted 4 lines
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 replacesmp_send_stop()with more suitable one for kdump.We have been avoiding extra things in panic path, but unfortunately
crash_kexec_post_notifierswere 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?)
Quoted 4 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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.
Quoted 3 lines
BTW, if one want to use
crash_kexec_post_notifiershe should take the risk of unreliable kdump. How about only callsmp_send_stopin case nocrash_kexec_post_notifiersbeing 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.
Quoted 6 lines
Quoted 4 lines
This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_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
Quoted 159 lines
Quoted 152 lines
Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +10
-4@@ -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. kernel/panic.c +32
-11@@ -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 ();Quoted 4 lines
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexecThanks
Dave
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Quoted 47 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for the comments.
Quoted 34 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMQuoted 30 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_crash_shutdown(), we miss those operations.Quoted 8 lines
Quoted 4 lines
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 replacesmp_send_stop()with more suitable one for kdump.We have been avoiding extra things in panic path, but unfortunately
crash_kexec_post_notifierswere added. I tend to agree the best place for this stuff is in 2nd kernel or purgatory instead of in 1st kernel.
Quoted 8 lines
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_dumpthings in purgatory wouldn't be suitable (should be done in the 2nd kernel before enabling devices and IRQs?)
In theory it is doable maybe do something like oldmem_kmsg_dump while /proc/vmcore initializing?
Quoted 30 lines
Quoted 5 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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_notifiersdisabled:
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_notifiersenabled:
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
Quoted 8 lines
The difference is that stopping other CPUs before
crash_setup_regs()andcrash_save_vmcoreinfo()or not. Sincecrash_setup_regs()andcrash_save_vmcoreinfo()just save information to some memory area, they wouldn't be affected bypanic_smp_send_stop(). This means placingpanic_smp_send_stopbefore__crash_kexecis safe.BTW, I noticed my patch breaks Xen kernel. I'll fix it in the next version.
But it does breaks stuff which depends on cpu not being disabled like problem 1 you mentioned in patch log.
Quoted 11 lines
Quoted 4 lines
BTW, if one want to use
crash_kexec_post_notifiershe should take the risk of unreliable kdump. How about only callsmp_send_stopin case nocrash_kexec_post_notifiersbeing 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 usesmp_send_stopifcrash_kexec_post_notifiersor kdump is disabled.
What I mean is like below, problem 1 will not exist in this way, but kdump will be unreliable:
if (!crash_kexec_post_notifiers)
smp_send_stop()
Quoted 13 lines
Quoted 7 lines
Quoted 5 lines
This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_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_stopvariant which stop the CPU without offlining.
Quoted 5 lines
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
[snip]
Thanks
Dave
RE: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Hi Dave,
Thanks for your reply.
Quoted 58 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Wednesday, July 13, 2016 11:04 AMQuoted 54 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for the comments.
Quoted 35 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMQuoted 31 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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 beforeocteon_generic_shutdown(),
stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 ONLINECPUs
Problem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()
path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on.
However, ifsmp_send_stop()stops other CPUs beforemachine_crash_shutdown(), we miss those operations.Quoted 8 lines
Quoted 4 lines
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 replacesmp_send_stop()with more suitable one for kdump.We have been avoiding extra things in panic path, but unfortunately
crash_kexec_post_notifierswere 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)
Quoted 4 lines
Quoted 2 lines
Doing
kmsg_dumpthings in purgatory wouldn't be suitable (should be done in the 2nd kernel before enabling devices and IRQs?)In theory it is doable maybe do something like
oldmem_kmsg_dumpwhile/proc/vmcoreinitializing?
Hmm, I checked the case of using ACPI ERST as a persistent storage. The feature is initialized by device_initcall():
device_initcall
erst_init
pstore_register
And vmcore is initialized by fs_initcall() which is called before device_initcall(). We may be able to change the sequence, but anyway, these are done in later phase of the kernel initialization. So, it may get less reliable although it would be doable.
Quoted 35 lines
Quoted 35 lines
Quoted 4 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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_notifiersdisabled:
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_notifiersenabled:
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 nothingThe difference is that stopping other CPUs before
crash_setup_regs()andcrash_save_vmcoreinfo()or not. Sincecrash_setup_regs()andcrash_save_vmcoreinfo()just save information to some memory area, they wouldn't be affected bypanic_smp_send_stop(). This means placingpanic_smp_send_stopbefore__crash_kexecis safe.
Quoted 4 lines
Quoted 2 lines
BTW, I noticed my patch breaks Xen kernel. I'll fix it in the next version.
But it does breaks stuff which depends on cpu not being disabled like problem 1 you mentioned in patch log.
As I mentioned in the description of this patch, we should stop other CPUs ASAP to preserve current state either crash_kexec_post_notifiers is enabled or not.
Then, all remaining procedures should work well
after stopping other CPUs (but keep the CPU map online).
Vivek also mentioned similar things:
https://lkml.org/lkml/2015/7/14/433
Quoted 11 lines
Quoted 10 lines
Quoted 3 lines
BTW, if one want to use
crash_kexec_post_notifiershe should take the risk of unreliable kdump. How about only callsmp_send_stopin case nocrash_kexec_post_notifiersbeing 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 usesmp_send_stopifcrash_kexec_post_notifiersor kdump is disabled.
Quoted 3 lines
What I mean is like below, problem 1 will not exist in this way, but kdump will be unreliable:
if (!crash_kexec_post_notifiers)smp_send_stop()
Remaining procedures including atomic_notifier_call_chain and kmsg_dump may assume that other CPUs have stopped. Actually, IPMI driver callback assumes so that. Also, other CPUs may change the current state while calling these callbacks and make the dump analysis difficult.
[snip]
Best regards,
Hidehiro Kawai
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Hi,
Quoted 70 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for your reply.
Quoted 58 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Wednesday, July 13, 2016 11:04 AMQuoted 54 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for the comments.
Quoted 35 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMQuoted 31 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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 beforeocteon_generic_shutdown(),
stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 ONLINECPUs
Problem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()
path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on.
However, ifsmp_send_stop()stops other CPUs beforemachine_crash_shutdown(), we miss those operations.Quoted 8 lines
Quoted 4 lines
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 replacesmp_send_stop()with more suitable one for kdump.We have been avoiding extra things in panic path, but unfortunately
crash_kexec_post_notifierswere 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)Quoted 4 lines
Quoted 2 lines
Doing
kmsg_dumpthings in purgatory wouldn't be suitable (should be done in the 2nd kernel before enabling devices and IRQs?)In theory it is doable maybe do something like
oldmem_kmsg_dumpwhile/proc/vmcoreinitializing?Hmm, I checked the case of using ACPI ERST as a persistent storage. The feature is initialized by
device_initcall():
Quoted 8 lines
device_initcall
erst_init
pstore_registerAnd vmcore is initialized by
fs_initcall()which is called beforedevice_initcall(). We may be able to change the sequence, but anyway, these are done in later phase of the kernel initialization. So, it may get less reliable although it would be doable.
Agreed, it is just an idea, it may need more experiments if you need.
Quoted 41 lines
Quoted 36 lines
Quoted 36 lines
Quoted 5 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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_notifiersdisabled:
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_notifiersenabled:
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 nothingThe difference is that stopping other CPUs before
crash_setup_regs()andcrash_save_vmcoreinfo()or not. Sincecrash_setup_regs()andcrash_save_vmcoreinfo()just save information to some memory area, they wouldn't be affected bypanic_smp_send_stop(). This means placingpanic_smp_send_stopbefore__crash_kexecis safe.Quoted 4 lines
Quoted 2 lines
BTW, I noticed my patch breaks Xen kernel. I'll fix it in the next version.
But it does breaks stuff which depends on cpu not being disabled like problem 1 you mentioned in patch log.
Quoted 8 lines
As I mentioned in the description of this patch, we should stop other CPUs ASAP to preserve current state either
crash_kexec_post_notifiersis enabled or not.
Then, all remaining procedures should work well
after stopping other CPUs (but keep the CPU map online).Vivek also mentioned similar things:
https://lkml.org/lkml/2015/7/14/433
The implementation in this patchset is different from suggestion in above link?
I think Vivek's suggestion is a good idea, to drop smp_send_stop and do below:
stop_cpus_save_register_state;
if (!crash_kexec_post_notifiers)
crash_kexec()atomic_notifier_call_chain()kmsg_dump()
I'm just commenting from code flow point of view, the detail implementation definitely need more comments from Arch experts.
Any reason did not move the kdump friendly function to earlier point like before previous __crash_kexec() below?
if (!crash_kexec_post_notifiers) {
printk_nmi_flush_on_panic();
__crash_kexec(NULL);
}Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 4 lines
BTW, if one want to use
crash_kexec_post_notifiershe should take the risk of unreliable kdump. How about only callsmp_send_stopin case nocrash_kexec_post_notifiersbeing 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 usesmp_send_stopifcrash_kexec_post_notifiersor kdump is disabled.
Quoted 7 lines
Quoted 3 lines
What I mean is like below, problem 1 will not exist in this way, but kdump will be unreliable:
if (!crash_kexec_post_notifiers)smp_send_stop()Remaining procedures including
atomic_notifier_call_chainandkmsg_dumpmay assume that other CPUs have stopped. Actually, IPMI driver callback assumes so that. Also, other CPUs may
Ok, if so please ignore my previous suggestion.
Quoted 9 lines
change the current state while calling these callbacks and make the dump analysis difficult.
[snip]
Best regards,
Hidehiro Kawai
Thanks
Dave
RE: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Hi,
Quoted 19 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Monday, July 18, 2016 6:02 PMQuoted 16 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for your reply.
Quoted 12 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Wednesday, July 13, 2016 11:04 AMQuoted 8 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for the comments.
Quoted 4 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMOn 07/05/16 at 08:33pm, Hidehiro Kawai wrote:
[snip]
Quoted 63 lines
Quoted 40 lines
Quoted 35 lines
Quoted 35 lines
Quoted 4 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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_notifiersdisabled:
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_notifiersenabled:
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 nothingThe difference is that stopping other CPUs before
crash_setup_regs()andcrash_save_vmcoreinfo()or not. Sincecrash_setup_regs()andcrash_save_vmcoreinfo()just save information to some memory area, they wouldn't be affected bypanic_smp_send_stop(). This means placingpanic_smp_send_stopbefore__crash_kexecis safe.Quoted 4 lines
Quoted 2 lines
BTW, I noticed my patch breaks Xen kernel. I'll fix it in the next version.
But it does breaks stuff which depends on cpu not being disabled like problem 1 you mentioned in patch log.
Quoted 8 lines
As I mentioned in the description of this patch, we should stop other CPUs ASAP to preserve current state either
crash_kexec_post_notifiersis enabled or not.
Then, all remaining procedures should work well
after stopping other CPUs (but keep the CPU map online).Vivek also mentioned similar things:
https://lkml.org/lkml/2015/7/14/433The implementation in this patchset is different from suggestion in above link?
I think Vivek's suggestion is a good idea, to drop
smp_send_stopand do below:
stop_cpus_save_register_state;if (!crash_kexec_post_notifiers)
crash_kexec()atomic_notifier_call_chain()kmsg_dump()I'm just commenting from code flow point of view, the detail implementation definitely need more comments from Arch experts.
Quoted 6 lines
Any reason did not move the kdump friendly function to earlier point like before previous
__crash_kexec()below?if (!crash_kexec_post_notifiers) { printk_nmi_flush_on_panic(); __crash_kexec(NULL); }
The reason why the implementation differs from Vivek's is to keep the current code flow if crash_kexec_post_notifiers is not specified.
If we apply Vivek's or your suggestion, it may always cause kdump to fail on MIPS OCTEON due to Problem 1. I don't want to make things any worse. I may post a patch for MIPS OCTEON, but I can't test it. For other architectures, I'm not sure what problems there are. So at first, I want to fix the case where crash_kexec_post_notifiers is specified on x86. Then, if all other architectures support `stop other CPUs before crash_kexec', switch to your or Vivek's suggesting code.
Is this acceptable?
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Hi,
Quoted 104 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi,
Quoted 19 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Monday, July 18, 2016 6:02 PMQuoted 16 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for your reply.
Quoted 12 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Wednesday, July 13, 2016 11:04 AMQuoted 8 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for the comments.
Quoted 4 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMOn 07/05/16 at 08:33pm, Hidehiro Kawai wrote:
[snip]
Quoted 63 lines
Quoted 40 lines
Quoted 35 lines
Quoted 35 lines
Quoted 4 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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_notifiersdisabled:
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_notifiersenabled:
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 nothingThe difference is that stopping other CPUs before
crash_setup_regs()andcrash_save_vmcoreinfo()or not. Sincecrash_setup_regs()andcrash_save_vmcoreinfo()just save information to some memory area, they wouldn't be affected bypanic_smp_send_stop(). This means placingpanic_smp_send_stopbefore__crash_kexecis safe.Quoted 4 lines
Quoted 2 lines
BTW, I noticed my patch breaks Xen kernel. I'll fix it in the next version.
But it does breaks stuff which depends on cpu not being disabled like problem 1 you mentioned in patch log.
Quoted 8 lines
As I mentioned in the description of this patch, we should stop other CPUs ASAP to preserve current state either
crash_kexec_post_notifiersis enabled or not.
Then, all remaining procedures should work well
after stopping other CPUs (but keep the CPU map online).Vivek also mentioned similar things:
https://lkml.org/lkml/2015/7/14/433The implementation in this patchset is different from suggestion in above link?
I think Vivek's suggestion is a good idea, to drop
smp_send_stopand do below:
stop_cpus_save_register_state;if (!crash_kexec_post_notifiers)
crash_kexec()atomic_notifier_call_chain()kmsg_dump()I'm just commenting from code flow point of view, the detail implementation definitely need more comments from Arch experts.
Quoted 6 lines
Any reason did not move the kdump friendly function to earlier point like before previous
__crash_kexec()below?if (!crash_kexec_post_notifiers) { printk_nmi_flush_on_panic(); __crash_kexec(NULL); }The reason why the implementation differs from Vivek's is to keep the current code flow if
crash_kexec_post_notifiersis not specified.If we apply Vivek's or your suggestion, it may always cause kdump to fail on MIPS OCTEON due to Problem 1. I don't want to make things any worse. I may post a patch for MIPS OCTEON, but I can't test it. For other architectures, I'm not sure what problems there are. So at first, I want to fix the case where
crash_kexec_post_notifiersis specified on x86. Then, if all other architectures support `stop other CPUs beforecrash_kexec', switch to your or Vivek's suggesting code.
Quoted 1 line
Is this acceptable?
Maybe you can find someone who can test MIPS OCTEON so that they can give some thoughts first and maybe test a fix?
[dyoung@localhost linux]$ ./scripts/get_maintainer.pl -f arch/mips/cavium-octeon Ralf Baechle <[email protected]> (supporter:MIPS,commit_signer:32/35=91%) David Daney <[email protected]> (commit_signer:21/35=60%,authored:8/35=23%) Aaro Koskinen <[email protected]> (commit_signer:15/35=43%,authored:8/35=23%) Janne Huttunen <[email protected]> (commit_signer:7/35=20%,authored:7/35=20%)
Thomas Gleixner <[email protected]> (commit_signer:4/35=11%,authored:2/35=6%) [email protected] (open list:MIPS) [email protected] (open list)
Thanks
Dave
RE: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Quoted 113 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Tuesday, July 19, 2016 3:52 PM
Hi,Quoted 104 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi,
Quoted 19 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Monday, July 18, 2016 6:02 PMQuoted 16 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for your reply.
Quoted 12 lines
From: 'Dave Young' [mailto:[email protected]]
Sent: Wednesday, July 13, 2016 11:04 AMQuoted 8 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Dave,
Thanks for the comments.
Quoted 4 lines
From: Dave Young [mailto:[email protected]]
Sent: Monday, July 11, 2016 5:35 PMOn 07/05/16 at 08:33pm, Hidehiro Kawai wrote:
[snip]
Quoted 63 lines
Quoted 40 lines
Quoted 35 lines
Quoted 35 lines
Quoted 4 lines
As for this patch I'm not sure it is safe to replace the
smp_send_stopwith 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_notifiersdisabled:
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_notifiersenabled:
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 nothingThe difference is that stopping other CPUs before
crash_setup_regs()andcrash_save_vmcoreinfo()or not. Sincecrash_setup_regs()andcrash_save_vmcoreinfo()just save information to some memory area, they wouldn't be affected bypanic_smp_send_stop(). This means placingpanic_smp_send_stopbefore__crash_kexecis safe.Quoted 4 lines
Quoted 2 lines
BTW, I noticed my patch breaks Xen kernel. I'll fix it in the next version.
But it does breaks stuff which depends on cpu not being disabled like problem 1 you mentioned in patch log.
Quoted 8 lines
As I mentioned in the description of this patch, we should stop other CPUs ASAP to preserve current state either
crash_kexec_post_notifiersis enabled or not.
Then, all remaining procedures should work well
after stopping other CPUs (but keep the CPU map online).Vivek also mentioned similar things:
https://lkml.org/lkml/2015/7/14/433The implementation in this patchset is different from suggestion in above link?
I think Vivek's suggestion is a good idea, to drop
smp_send_stopand do below:
stop_cpus_save_register_state;if (!crash_kexec_post_notifiers)
crash_kexec()atomic_notifier_call_chain()kmsg_dump()I'm just commenting from code flow point of view, the detail implementation definitely need more comments from Arch experts.
Quoted 6 lines
Any reason did not move the kdump friendly function to earlier point like before previous
__crash_kexec()below?if (!crash_kexec_post_notifiers) { printk_nmi_flush_on_panic(); __crash_kexec(NULL); }The reason why the implementation differs from Vivek's is to keep the current code flow if
crash_kexec_post_notifiersis not specified.If we apply Vivek's or your suggestion, it may always cause kdump to fail on MIPS OCTEON due to Problem 1. I don't want to make things any worse. I may post a patch for MIPS OCTEON, but I can't test it. For other architectures, I'm not sure what problems there are. So at first, I want to fix the case where
crash_kexec_post_notifiersis specified on x86. Then, if all other architectures support `stop other CPUs beforecrash_kexec', switch to your or Vivek's suggesting code.Quoted 1 line
Is this acceptable?
Maybe you can find someone who can test MIPS OCTEON so that they can give some thoughts first and maybe test a fix?
Quoted 9 lines
[dyoung@localhost linux]$
./scripts/get_maintainer.pl-farch/mips/cavium-octeonRalf Baechle <[email protected]> (supporter:MIPS,commit_signer:32/35=91%) David Daney <[email protected]> (commit_signer:21/35=60%,authored:8/35=23%) Aaro Koskinen <[email protected]> (commit_signer:15/35=43%,authored:8/35=23%) Janne Huttunen <[email protected]> (commit_signer:7/35=20%,authored:7/35=20%)
Thomas Gleixner <[email protected]> (commit_signer:4/35=11%,authored:2/35=6%) [email protected] (open list:MIPS) [email protected] (open list)
So I'll try to fix for MIPS OCTEON, but I'm going to keep the current behavior when crash_kexec_post_notifiers is not specified because I'm not sure what will happen on other architectures.
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
In reply to Hidehiro KawaiQuoted 81 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_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 replacesmp_send_stop()with more suitable one for kdump.This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_send_stop(), and architecture dependent code may override this with appropriate one. This patch only provides x86-specific version.Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +0
-0
arch/x86/kernel/crash.c static void kdump_nmi_callback(int cpu, struct pt_regs *regs) · line 139
Quoted 9 lines
arch/x86/kernel/crash.c +3
-2@@ -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;
Should be atomic_t type?
Quoted 71 lines
arch/x86/kernel/crash.c +7
-2+ +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. kernel/panic.c +27
-1@@ -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.
kernel/panic.c void panic(const char *fmt, ...) · line 194
Quoted 10 lines
kernel/panic.c +1
-6@@ -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 ();
If crash_kexec_post_notifiers and kdump both are not used, you actually changed the original behaviour, it would be better to make it conditionally like:
if (crash_kexec_post_notifiers)
panic_smp_send_stop(); // renaming it to crash_smp_send_stop() is better?
else
smp_send_stop();Regards,
Xunlei
Quoted 25 lines
kernel/panic.c +4
-4/* * 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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
RE: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Hi Xunlei,
Thanks for the review.
Quoted 87 lines
From: Xunlei Pang [mailto:[email protected]]
Sent: Tuesday, July 12, 2016 12:12 PMQuoted 84 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_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 replacesmp_send_stop()with more suitable one for kdump.This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_send_stop(), and architecture dependent code may override this with appropriate one. This patch only provides x86-specific version.Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +0
-0@@ -133,15 +133,21 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)disable_local_APIC (); }
Quoted 8 lines
arch/x86/kernel/crash.c static void kdump_nmi_callback(int cpu, struct pt_regs *regs) · line 139
Quoted 6 lines
arch/x86/kernel/crash.c +3
-2-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;Should be
atomic_ttype?
panic_smp_send_stop() can be called by only one panicking CPU (but can be called twice). It is sufficient to be normal variable.
Quoted 84 lines
Quoted 71 lines
arch/x86/kernel/crash.c +7
-2+ +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. kernel/panic.c +27
-1@@ -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. kernel/panic.c void panic(const char *fmt, ...) · line 194
Quoted 10 lines
kernel/panic.c +1
-6@@ -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 ();If
crash_kexec_post_notifiersand kdump both are not used, you actually changed the original behaviour, it would be better to make it conditionally like:
Quoted 4 lines
if (crash_kexec_post_notifiers)
panic_smp_send_stop(); // renaming it to crash_smp_send_stop() is better?
else
smp_send_stop();Dave also pointed about this, and I'll fix it.
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
Quoted 24 lines
Quoted 24 lines
kernel/panic.c +4
-4/* * 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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Quoted 98 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Xunlei,
Thanks for the review.
Quoted 87 lines
From: Xunlei Pang [mailto:[email protected]]
Sent: Tuesday, July 12, 2016 12:12 PMQuoted 84 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_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 replacesmp_send_stop()with more suitable one for kdump.This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_send_stop(), and architecture dependent code may override this with appropriate one. This patch only provides x86-specific version.Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +0
-0@@ -133,15 +133,21 @@ static void kdump_nmi_callback(int cpu, struct pt_regs *regs)disable_local_APIC (); }Quoted 7 lines
arch/x86/kernel/crash.c static void kdump_nmi_callback(int cpu, struct pt_regs *regs) · line 139
Quoted 6 lines
arch/x86/kernel/crash.c +3
-2-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;Should be
atomic_ttype?
Quoted 2 lines
panic_smp_send_stop()can be called by only one panicking CPU (but can be called twice). It is sufficient to be normal variable.
There are other call sites of __crash_kexec() for oops cases, which can call panic_smp_send_stop() concurrently on a different cpu.
Regards,
Xunlei
Quoted 118 lines
Quoted 83 lines
Quoted 71 lines
arch/x86/kernel/crash.c +7
-2+ +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. kernel/panic.c +27
-1@@ -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. kernel/panic.c void panic(const char *fmt, ...) · line 194
Quoted 10 lines
kernel/panic.c +1
-6@@ -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 ();If
crash_kexec_post_notifiersand kdump both are not used, you actually changed the original behaviour, it would be better to make it conditionally like:Quoted 4 lines
if (crash_kexec_post_notifiers) panic_smp_send_stop(); // renaming it to crash_smp_send_stop() is better? else smp_send_stop();Dave also pointed about this, and I'll fix it.
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development GroupQuoted 24 lines
Quoted 24 lines
kernel/panic.c +4
-4/* * 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
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
RE: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Quoted 101 lines
From: [email protected] [mailto:[email protected]] On Behalf Of Xunlei Pang
Sent: Tuesday, July 12, 2016 3:57 PMQuoted 98 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Xunlei,
Thanks for the review.
Quoted 94 lines
From: Xunlei Pang [mailto:[email protected]]
Sent: Tuesday, July 12, 2016 12:12 PMQuoted 81 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_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 replacesmp_send_stop()with more suitable one for kdump.This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_send_stop(), and architecture dependent code may override this with appropriate one. This patch only provides x86-specific version.Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +0
-0arch/x86/kernel/crash.c static void kdump_nmi_callback(int cpu, struct pt_regs *regs) · line 139
Quoted 9 lines
arch/x86/kernel/crash.c +3
-2@@ -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;Should be
atomic_ttype?
Quoted 5 lines
Quoted 2 lines
panic_smp_send_stop()can be called by only one panicking CPU (but can be called twice). It is sufficient to be normal variable.There are other call sites of
__crash_kexec()for oops cases, which can callpanic_smp_send_stop()concurrently on a different cpu.
In oops cases, crash_kexec() is called first, then __crash_kexec() is called. crash_kexec() excludes concurrent execution of panic() and crash_kexec() via panic_cpu, so panic_smp_send_stop() shouldn't be called concurrently.
Best regards,
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec
Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Quoted 101 lines · 河合英宏 / KAWAI,HIDEHIRO
Quoted 101 lines
From: [email protected] [mailto:[email protected]] On Behalf Of Xunlei Pang
Sent: Tuesday, July 12, 2016 3:57 PMQuoted 98 lines · 河合英宏 / KAWAI,HIDEHIRO
Hi Xunlei,
Thanks for the review.
Quoted 94 lines
From: Xunlei Pang [mailto:[email protected]]
Sent: Tuesday, July 12, 2016 12:12 PMQuoted 81 lines · Hidehiro Kawai
This patch fixes one of the problems reported by Daniel Walker
(https://lkml.org/lkml/2015/6/24/44).If
crash_kexec_post_notifiersboot option is specified, other CPUs are stopped bysmp_send_stop()instead ofmachine_crash_shutdown()incrash_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. Ifsmp_send_stop()is called beforeocteon_generic_shutdown(), stopping watchdog timer will fail because other CPUs have been offlined bysmp_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 CPUsProblem 2:
Most of architectures stop other CPUs inmachine_crash_shutdown()path, and they also do something needed for kdump. For example, they save registers, disable virtualization extensions, and so on. However, ifsmp_send_stop()stops other CPUs beforemachine_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 replacesmp_send_stop()with more suitable one for kdump.This patch solves Problem 2 by replacing
smp_send_stop()inpanic()withpanic_smp_send_stop(). This is a weak function which callssmp_send_stop(), and architecture dependent code may override this with appropriate one. This patch only provides x86-specific version.Changes in V3:
- Revise comments, description, and symbol namesChanges in V2:
- Replacesmp_send_stop()call withcrash_kexecversion which saves cpu states and cleans up VMX/SVM
- Drop a fix for Problem 1 at this momentReported-by Daniel Walker <[email protected]>Fixes f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)Signed-off-by Hidehiro Kawai <[email protected]>Cc Dave Young <[email protected]>Cc Baoquan He <[email protected]>Cc Vivek Goyal <[email protected]>Cc Eric Biederman <[email protected]>Cc Masami Hiramatsu <[email protected]>Cc Thomas Gleixner <[email protected]>Cc Ingo Molnar <[email protected]>Cc "H. Peter Anvin" <[email protected]>Cc Borislav Petkov <[email protected]>Cc Toshi Kani <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Takao Indoh <[email protected]>Cc "Lee, Chun-Yi" <[email protected]>Cc Minfei Huang <[email protected]>Cc Andrew Morton <[email protected]>Cc Michal Hocko <[email protected]>Cc Vitaly Kuznetsov <[email protected]>Cc Petr Mladek <[email protected]>Cc Tejun Heo <[email protected]>Cc Josh Poimboeuf <[email protected]>--- arch/x86/kernel/crash.c | 14 ++++++++++---- kernel/panic.c | 43 ++++++++++++++++++++++++++++++++----------- 2 files changed, 42 insertions(+), 15 deletions(-)arch/x86/kernel/crash.c +0
-0arch/x86/kernel/crash.c static void kdump_nmi_callback(int cpu, struct pt_regs *regs) · line 139
Quoted 9 lines
arch/x86/kernel/crash.c +3
-2@@ -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;Should be
atomic_ttype?
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
panic_smp_send_stop()can be called by only one panicking CPU (but can be called twice). It is sufficient to be normal variable.There are other call sites of
__crash_kexec()for oops cases, which can callpanic_smp_send_stop()concurrently on a different cpu.In oops cases,
crash_kexec()is called first, then__crash_kexec()is called.crash_kexec()excludes concurrent execution ofpanic()andcrash_kexec()viapanic_cpu, sopanic_smp_send_stop()shouldn't be called concurrently.
Right, that's why oops calls crash_kexec() not __crash_kexec(). I have no problem on this. Thanks!
Regards,
Xunlei
PATCH v3 2/2 kexec: Use core_param for crash_kexec_post_notifiers boot option
In reply to Hidehiro Kawaicrash_kexec_post_notifiers ia a boot option which controls whether the 1st kernel calls panic notifiers or not before booting the 2nd kernel. However, there is no need to limit it to being modifiable only at boot time. So, use core_param instead of early_param.
--- kernel/panic.c | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-)
kernel/panic.c +4 -9
@@ -134,6 +134,7 @@ void panic(const char *fmt, ...)long i, i_next =0 ;int state =0 ;int old_cpu, this_cpu; +bool _crash_kexec_post_notifiers = crash_kexec_post_notifiers;/* * Disable local interrupts. This will prevent panic_smp_self_stop
@@ -186,7 +187,7 @@ void panic(const char *fmt, ...)* * Bypass the panic_cpu check and call __crash_kexec directly. */ -if (!crash_kexec_post_notifiers) { +if (!_crash_kexec_post_notifiers) {printk_nmi_flush_on_panic ();__crash_kexec (NULL ); }
@@ -212,7 +213,7 @@ void panic(const char *fmt, ...)* * Bypass the panic_cpu check and call __crash_kexec directly. */ -if (crash_kexec_post_notifiers) +if (_crash_kexec_post_notifiers)__crash_kexec (NULL );bust_spinlocks (0 );
@@ -592,13 +593,7 @@ EXPORT_SYMBOL(__stack_chk_fail);core_param (panic, panic_timeout, int,0644 );core_param (pause_on_oops, pause_on_oops, int,0644 );core_param (panic_on_warn, panic_on_warn, int,0644 ); - -static int __init setup_crash_kexec_post_notifiers (char *s) -{ - crash_kexec_post_notifiers = true; -return 0 ; -} -early_param ("crash_kexec_post_notifiers" , setup_crash_kexec_post_notifiers); +core_param (crash_kexec_post_notifiers, crash_kexec_post_notifiers, bool,0644 );static int __init oops_setup (char *s) {
_______________________________________________
kexec mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kexec