From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756021AbbIYMOC (ORCPT ); Fri, 25 Sep 2015 08:14:02 -0400 Received: from mail7.hitachi.co.jp ([133.145.228.42]:54670 "EHLO mail7.hitachi.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754567AbbIYMN7 (ORCPT ); Fri, 25 Sep 2015 08:13:59 -0400 From: =?utf-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= To: =?utf-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= , Jonathan Corbet , "Peter Zijlstra" , Ingo Molnar , "Eric W. Biederman" , "H. Peter Anvin" , "Andrew Morton" , Thomas Gleixner , Vivek Goyal CC: "linux-doc@vger.kernel.org" , "x86@kernel.org" , "kexec@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Michal Hocko , Ingo Molnar , =?utf-8?B?5bmz5p2+6ZuF5bezIC8gSElSQU1BVFXvvIxNQVNBTUk=?= Subject: RE: [V4 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI Thread-Topic: [V4 PATCH 1/4] panic/x86: Fix re-entrance problem due to panic on NMI Thread-Index: AQHQ94ogKkFdl2DyxkWUrca7dnyww55NJslg Date: Fri, 25 Sep 2015 12:13:55 +0000 Message-ID: <04EAB7311EE43145B2D3536183D1A8445499083F@GSjpTKYDCembx31.service.hitachi.net> References: <20150925112803.4258.94241.stgit@softrs> <20150925112805.4258.97380.stgit@softrs> In-Reply-To: <20150925112805.4258.97380.stgit@softrs> Accept-Language: ja-JP, en-US Content-Language: ja-JP X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.198.219.41] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t8PCE8u4029234 Peter saids -tip tree doesn't have panic_on_unrecovered_nmi in the previoius discussion, but it still exists. So, I didn't change anything about panic_on_unrecovered_nmi. Thanks, Hidehiro Kawai Hitachi, Ltd. Research & Development Group > From: Hidehiro Kawai [mailto:hidehiro.kawai.ez@hitachi.com] > > If panic on NMI happens just after panic() on the same CPU, panic() > is recursively called. As the result, it stalls after failing to > acquire panic_lock. > > To avoid this problem, don't call panic() in NMI context if > we've already entered panic(). > > V4: > - Improve comments in io_check_error() and panic() > > V3: > - Introduce nmi_panic() macro to reduce code duplication > - In the case of panic on NMI, don't return from NMI handlers > if another cpu already panicked > > V2: > - Use atomic_cmpxchg() instead of current spin_trylock() to > exclude concurrent accesses to the panic routines > - Don't introduce no-lock version of panic() > > Signed-off-by: Hidehiro Kawai > Cc: Andrew Morton > Cc: Thomas Gleixner > Cc: Ingo Molnar > Cc: "H. Peter Anvin" > Cc: Peter Zijlstra > Cc: Michal Hocko > --- > arch/x86/kernel/nmi.c | 16 ++++++++++++---- > include/linux/kernel.h | 13 +++++++++++++ > kernel/panic.c | 15 ++++++++++++--- > kernel/watchdog.c | 2 +- > 4 files changed, 38 insertions(+), 8 deletions(-) > > diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c > index 697f90d..5131714 100644 > --- a/arch/x86/kernel/nmi.c > +++ b/arch/x86/kernel/nmi.c > @@ -231,7 +231,7 @@ void unregister_nmi_handler(unsigned int type, const char *name) > #endif > > if (panic_on_unrecovered_nmi) > - panic("NMI: Not continuing"); > + nmi_panic("NMI: Not continuing"); > > pr_emerg("Dazed and confused, but trying to continue\n"); > > @@ -255,8 +255,16 @@ void unregister_nmi_handler(unsigned int type, const char *name) > reason, smp_processor_id()); > show_regs(regs); > > - if (panic_on_io_nmi) > - panic("NMI IOCK error: Not continuing"); > + if (panic_on_io_nmi) { > + nmi_panic("NMI IOCK error: Not continuing"); > + > + /* > + * If we return from nmi_panic(), it means we have received > + * NMI while processing panic(). So, simply return without > + * a delay and re-enabling NMI. > + */ > + return; > + } > > /* Re-enable the IOCK line, wait for a few seconds */ > reason = (reason & NMI_REASON_CLEAR_MASK) | NMI_REASON_CLEAR_IOCHK; > @@ -297,7 +305,7 @@ void unregister_nmi_handler(unsigned int type, const char *name) > > pr_emerg("Do you have a strange power saving mode enabled?\n"); > if (unknown_nmi_panic || panic_on_unrecovered_nmi) > - panic("NMI: Not continuing"); > + nmi_panic("NMI: Not continuing"); > > pr_emerg("Dazed and confused, but trying to continue\n"); > } > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 5582410..57c33da 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -443,6 +443,19 @@ extern __scanf(2, 0) > > extern bool crash_kexec_post_notifiers; > > +extern atomic_t panic_cpu; > + > +/* > + * A variant of panic() called from NMI context. > + * If we've already panicked on this cpu, return from here. > + */ > +#define nmi_panic(fmt, ...) \ > + do { \ > + int this_cpu = raw_smp_processor_id(); \ > + if (atomic_cmpxchg(&panic_cpu, -1, this_cpu) != this_cpu) \ > + panic(fmt, ##__VA_ARGS__); \ > + } while (0) > + > /* > * Only to be used by arch init code. If the user over-wrote the default > * CONFIG_PANIC_TIMEOUT, honor it. > diff --git a/kernel/panic.c b/kernel/panic.c > index 04e91ff..a105e67 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -60,6 +60,8 @@ void __weak panic_smp_self_stop(void) > cpu_relax(); > } > > +atomic_t panic_cpu = ATOMIC_INIT(-1); > + > /** > * panic - halt the system > * @fmt: The text string to print > @@ -70,17 +72,17 @@ void __weak panic_smp_self_stop(void) > */ > void panic(const char *fmt, ...) > { > - static DEFINE_SPINLOCK(panic_lock); > static char buf[1024]; > va_list args; > long i, i_next = 0; > int state = 0; > + int old_cpu, this_cpu; > > /* > * Disable local interrupts. This will prevent panic_smp_self_stop > * from deadlocking the first cpu that invokes the panic, since > * there is nothing to prevent an interrupt handler (that runs > - * after the panic_lock is acquired) from invoking panic again. > + * after setting panic_cpu) from invoking panic again. > */ > local_irq_disable(); > > @@ -93,8 +95,15 @@ void panic(const char *fmt, ...) > * 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(). > + * > + * `old_cpu == -1' means this is the 1st CPU which comes here, so > + * go ahead. > + * `old_cpu == this_cpu' means we came from nmi_panic() which sets > + * panic_cpu to this cpu. In this case, this is also the 1st CPU. > */ > - if (!spin_trylock(&panic_lock)) > + this_cpu = raw_smp_processor_id(); > + old_cpu = atomic_cmpxchg(&panic_cpu, -1, this_cpu); > + if (old_cpu != -1 && old_cpu != this_cpu) > panic_smp_self_stop(); > > console_verbose(); > diff --git a/kernel/watchdog.c b/kernel/watchdog.c > index 64ed1c3..00fbaa29 100644 > --- a/kernel/watchdog.c > +++ b/kernel/watchdog.c > @@ -324,7 +324,7 @@ static void watchdog_overflow_callback(struct perf_event *event, > return; > > if (hardlockup_panic) > - panic("Watchdog detected hard LOCKUP on cpu %d", > + nmi_panic("Watchdog detected hard LOCKUP on cpu %d", > this_cpu); > else > WARN(1, "Watchdog detected hard LOCKUP on cpu %d", > {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I