Date:	Tue, 12 Jul 2016 11:11:56 +0800
From:	Xunlei Pang <xpang@...hat.com>
To:	Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dave Young <dyoung@...hat.com>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Baoquan He <bhe@...hat.com>
Cc:	Michal Hocko <mhocko@...e.com>, Toshi Kani <toshi.kani@....com>,
	"Peter Zijlstra (Intel)" <peterz@...radead.org>,
	Minfei Huang <mnfhuang@...il.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	Daniel Walker <dwalker@...o99.com>,
	Ingo Molnar <mingo@...nel.org>,
	Takao Indoh <indou.takao@...fujitsu.com>, x86@...nel.org,
	"Lee, Chun-Yi" <joeyli.kernel@...il.com>,
	Borislav Petkov <bp@...e.de>, Vivek Goyal <vgoyal@...hat.com>,
	Petr Mladek <pmladek@...e.com>,
	Josh Poimboeuf <jpoimboe@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...hat.com>, kexec@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	HATAYAMA Daisuke <d.hatayama@...fujitsu.com>,
	Masami Hiramatsu <mhiramat@...nel.org>,
	Tejun Heo <tj@...nel.org>,
	Vitaly Kuznetsov <vkuznets@...hat.com>
Subject: Re: [V3 PATCH 1/2] x86/panic: Replace smp_send_stop() with kdump
 friendly version

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@...o99.com>
> Fixes: f06e5153f4ae (kernel/panic.c: add "crash_kexec_post_notifiers" option)
> Signed-off-by: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
> Cc: Dave Young <dyoung@...hat.com>
> Cc: Baoquan He <bhe@...hat.com>
> Cc: Vivek Goyal <vgoyal@...hat.com>
> Cc: Eric Biederman <ebiederm@...ssion.com>
> Cc: Masami Hiramatsu <mhiramat@...nel.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Toshi Kani <toshi.kani@....com>
> Cc: "Peter Zijlstra (Intel)" <peterz@...radead.org>
> Cc: Takao Indoh <indou.takao@...fujitsu.com>
> Cc: "Lee, Chun-Yi" <joeyli.kernel@...il.com>
> Cc: Minfei Huang <mnfhuang@...il.com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Michal Hocko <mhocko@...e.com>
> Cc: Vitaly Kuznetsov <vkuznets@...hat.com>
> Cc: Petr Mladek <pmladek@...e.com>
> Cc: Tejun Heo <tj@...nel.org>
> Cc: Josh Poimboeuf <jpoimboe@...hat.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?

> +
> +	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();

Regards,
Xunlei

>  
>  	/*
>  	 * 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@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec

Powered by blists - more mailing lists