From: Xunlei Pang <xpang@redhat.com>
To: "河合英宏 / KAWAI,HIDEHIRO" <hidehiro.kawai.ez@hitachi.com>,
"'xlpang@redhat.com'" <xlpang@redhat.com>,
"Andrew Morton" <akpm@linux-foundation.org>,
"Dave Young" <dyoung@redhat.com>,
"Eric W. Biederman" <ebiederm@xmission.com>,
"Baoquan He" <bhe@redhat.com>
Cc: Michal Hocko <mhocko@suse.com>, Toshi Kani <toshi.kani@hpe.com>,
"Peter Zijlstra (Intel)" <peterz@infradead.org>,
Minfei Huang <mnfhuang@gmail.com>,
"H. Peter Anvin" <hpa@zytor.com>,
Daniel Walker <dwalker@fifo99.com>,
Ingo Molnar <mingo@kernel.org>,
Takao Indoh <indou.takao@jp.fujitsu.com>,
"x86@kernel.org" <x86@kernel.org>,
"Lee, Chun-Yi" <joeyli.kernel@gmail.com>,
Borislav Petkov <bp@suse.de>, Vivek Goyal <vgoyal@redhat.com>,
Petr Mladek <pmladek@suse.com>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Thomas Gleixner <tglx@linutronix.de>,
Ingo Molnar <mingo@redhat.com>,
"kexec@lists.infradead.org" <kexec@lists.infradead.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>,
Masami Hiramatsu <mhiramat@kernel.org>, Tejun Heo <tj@kernel.org>,
Vitaly Kuznetsov <vkuznets@redhat.com>
Subject: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version
Date: Tue, 12 Jul 2016 14:57:24 +0800 [thread overview]
Message-ID: <578494D4.2020505@redhat.com> (raw)
In-Reply-To: <04EAB7311EE43145B2D3536183D1A84454C6F9CB@GSjpTKYDCembx31.service.hitachi.net>
On 2016/07/12 at 11:56, 河合英宏 / KAWAI,HIDEHIRO wrote:
> Hi Xunlei,
>
> Thanks for the review.
>
>> From: Xunlei Pang [mailto:xpang@redhat.com]
>> Sent: Tuesday, July 12, 2016 12:12 PM
>> On 2016/07/05 at 19:33, 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.
>>>
>>> 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
>>>
>>> Reported-by: Daniel Walker <dwalker@fifo99.com>
>>> Fixes: f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)
>>> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@hitachi.com>
>>> Cc: Dave Young <dyoung@redhat.com>
>>> Cc: Baoquan He <bhe@redhat.com>
>>> Cc: Vivek Goyal <vgoyal@redhat.com>
>>> Cc: Eric Biederman <ebiederm@xmission.com>
>>> Cc: Masami Hiramatsu <mhiramat@kernel.org>
>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>> Cc: Ingo Molnar <mingo@redhat.com>
>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>> Cc: Borislav Petkov <bp@suse.de>
>>> Cc: Toshi Kani <toshi.kani@hpe.com>
>>> Cc: "Peter Zijlstra (Intel)" <peterz@infradead.org>
>>> Cc: Takao Indoh <indou.takao@jp.fujitsu.com>
>>> Cc: "Lee, Chun-Yi" <joeyli.kernel@gmail.com>
>>> Cc: Minfei Huang <mnfhuang@gmail.com>
>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>> Cc: Michal Hocko <mhocko@suse.com>
>>> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
>>> Cc: Petr Mladek <pmladek@suse.com>
>>> Cc: Tejun Heo <tj@kernel.org>
>>> Cc: Josh Poimboeuf <jpoimboe@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;
>> Should be atomic_t type?
> 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
>>> +
>>> + 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();
>> 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();
> Dave also pointed about this, and I'll fix it.
>
> Best regards,
>
> Hidehiro Kawai
> Hitachi, Ltd. Research & Development Group
>
>>> /*
>>> * 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@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2016-07-12 6:57 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-05 11:33 [V3 PATCH 0/2] kexec: crash_kexec_post_notifiers boot option related fixes Hidehiro Kawai
2016-07-05 11:33 ` [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump friendly version Hidehiro Kawai
2016-07-11 8:34 ` Dave Young
2016-07-12 2:49 ` 河合英宏 / KAWAI,HIDEHIRO
2016-07-13 2:03 ` 'Dave Young'
2016-07-15 11:50 ` 河合英宏 / KAWAI,HIDEHIRO
2016-07-18 9:02 ` 'Dave Young'
2016-07-19 5:51 ` 河合英宏 / KAWAI,HIDEHIRO
2016-07-19 6:52 ` 'Dave Young'
2016-07-19 11:23 ` 河合英宏 / KAWAI,HIDEHIRO
2016-07-12 3:11 ` Xunlei Pang
2016-07-12 3:56 ` 河合英宏 / KAWAI,HIDEHIRO
2016-07-12 6:57 ` Xunlei Pang [this message]
2016-07-12 7:12 ` 河合英宏 / KAWAI,HIDEHIRO
2016-07-12 7:26 ` Xunlei Pang
2016-07-05 11:33 ` [V3 PATCH 2/2] kexec: Use core_param for crash_kexec_post_notifiers boot option Hidehiro Kawai
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=578494D4.2020505@redhat.com \
--to=xpang@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=bhe@redhat.com \
--cc=bp@suse.de \
--cc=d.hatayama@jp.fujitsu.com \
--cc=dwalker@fifo99.com \
--cc=dyoung@redhat.com \
--cc=ebiederm@xmission.com \
--cc=hidehiro.kawai.ez@hitachi.com \
--cc=hpa@zytor.com \
--cc=indou.takao@jp.fujitsu.com \
--cc=joeyli.kernel@gmail.com \
--cc=jpoimboe@redhat.com \
--cc=kexec@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mhiramat@kernel.org \
--cc=mhocko@suse.com \
--cc=mingo@kernel.org \
--cc=mingo@redhat.com \
--cc=mnfhuang@gmail.com \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=tglx@linutronix.de \
--cc=tj@kernel.org \
--cc=toshi.kani@hpe.com \
--cc=vgoyal@redhat.com \
--cc=vkuznets@redhat.com \
--cc=x86@kernel.org \
--cc=xlpang@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).