Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755064AbbKYI43 (ORCPT ); Wed, 25 Nov 2015 03:56:29 -0500 Received: from mail.skyhub.de ([78.46.96.112]:51485 "EHLO mail.skyhub.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753676AbbKYI40 (ORCPT ); Wed, 25 Nov 2015 03:56:26 -0500 Date: Wed, 25 Nov 2015 09:56:21 +0100 From: Borislav Petkov To: =?utf-8?B?5rKz5ZCI6Iux5a6PIC8gS0FXQUnvvIxISURFSElSTw==?= Cc: Jonathan Corbet , Peter Zijlstra , Ingo Molnar , "Eric W. Biederman" , "H. Peter Anvin" , Andrew Morton , Thomas Gleixner , Vivek Goyal , Baoquan He , "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: [V5 PATCH 2/4] panic/x86: Allow cpus to save registers even if they are looping in NMI context Message-ID: <20151125085620.GA29499@pd.tnic> References: <20151120093641.4285.97253.stgit@softrs> <20151120093646.4285.62259.stgit@softrs> <20151124104853.GC3785@pd.tnic> <04EAB7311EE43145B2D3536183D1A84454A2A425@GSjpTKYDCembx31.service.hitachi.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <04EAB7311EE43145B2D3536183D1A84454A2A425@GSjpTKYDCembx31.service.hitachi.net> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4953 Lines: 138 On Wed, Nov 25, 2015 at 05:51:59AM +0000, 河合英宏 / KAWAI，HIDEHIRO wrote: > > > `Infinite loop in NMI context' can happen: > > > > > > a. when a cpu panics on NMI while another cpu is processing panic > > > b. when a cpu received an external or unknown NMI while another > > > cpu is processing panic on NMI > > > > > > In the case of a, it loops in panic_smp_self_stop(). In the case > > > of b, it loops in raw_spin_lock() of nmi_reason_lock. > > > > Please describe those two cases more verbosely - it takes slow people > > like me a while to figure out what exactly can happen. > > a. when a cpu panics on NMI while another cpu is processing panic > Ex. > CPU 0 CPU 1 > ================= ================= > panic() > panic_cpu <-- 0 > check panic_cpu > crash_kexec() > receive an unknown NMI > unknown_nmi_error() > nmi_panic() > panic() > check panic_cpu > panic_smp_self_stop() > infinite loop in NMI context > > b. when a cpu received an external or unknown NMI while another > cpu is processing panic on NMI > Ex. > CPU 0 CPU 1 > ====================== ================== > receive an unknown NMI > unknown_nmi_error() > nmi_panic() receive an unknown NMI > panic_cpu <-- 0 unknown_nmi_error() > panic() nmi_panic() > check panic_cpu panic > crash_kexec() check panic_cpu > panic_smp_self_stop() > infinite loop in NMI context Ok, that's what I saw too, thanks for confirming. But please write those examples with the *old* code in the commit message, i.e. without panic_cpu and nmi_panic() which you're adding. Basically, you want to structure your commit message this way: This is the problem the current code has: ... But we need to do this: ... We fix it by doing that: ... This will be of great help now when reading the commit message and of invaluable help later, when we all have forgotten about the issue and are scratching heads over why stuff was added. > > > To save registers in these case too, this patch does following things: > > > > > > 1. Move the timing of `infinite loop in NMI context' (actually > > > done by panic_smp_self_stop()) outside of panic() to enable us to > > > refer pt_regs > > > > I can't parse that sentence. And I really tried :-\ > > panic_smp_self_stop() is still in panic(). > > panic_smp_self_stop() is still used when a CPU in normal context > should go into infinite loop. Only when a CPU is in NMI context, > nmi_panic_self_stop() is called and the CPU loops infinitely > without entering panic(). > > I'll try to revise this sentense. FWIW, it sounds better already! :) > > > + */ > > > + while (!raw_spin_trylock(&nmi_reason_lock)) > > > + poll_crash_ipi_and_callback(regs); > > > > Waaait a minute: so if we're getting NMIs broadcasted on every core but > > we're *not* crash dumping, we will run into here too. This can't be > > right. :-\ > > As Steven commented, poll_crash_ipi_and_callback() does nothing > if we're not crash dumping. But yes, this is confusing. I'll add > more detailed comment, or change the logic a bit if I come up with > better one. Thanks, much appreciated! > > > +/* > > > + * Wait for the timing of IPI for crash dumping, and then call its callback > > > > "Wait for the crash dumping IPI to complete... " > > So, I think "Wait for the crash dumping IPI to be issued..." is better. > "complete" would be ambiguous in this context. Ok. > > > > + * directly. This function is used when we have already been in NMI handler. > > > + */ > > > +void poll_crash_ipi_and_callback(struct pt_regs *regs) > > > > Why "poll"? We won't return from crash_nmi_callback() if we're not the > > crashing CPU. > > This function polls that crash IPI has been issued by checking > crash_ipi_done, then invokes the callback. This is different > from so-called "poll" functions. Do you have some good name? Maybe something as simple as "run_crash_callback"? Or since we're calling it from other places, maybe add the "crash" prefix: while (!raw_spin_trylock(&nmi_reason_lock)) crash_run_callback(regs); Looks even better to me in code context. :) Thanks! -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/