From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from usmamail.tilera.com ([206.83.70.75]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1ROuV8-0007Pp-Pg for kexec@lists.infradead.org; Fri, 11 Nov 2011 17:02:51 +0000 Message-ID: <4EBD5536.7010806@tilera.com> Date: Fri, 11 Nov 2011 12:02:46 -0500 From: Chris Metcalf MIME-Version: 1.0 Subject: Re: [PATCH] kdump: Fix crash_kexec - smp_send_stop race in panic References: <1319639649.3321.11.camel@br98xy6r> <20111028161143.e5ebf617.akpm@linux-foundation.org> <1320055036.2796.8.camel@br98xy6r> <20111031033948.a0edb7f3.akpm@linux-foundation.org> <1320314844.2989.6.camel@br98xy6r> <20111109160400.cc2d27d9.akpm@linux-foundation.org> <1320934932.16425.14.camel@br98xy6r> <4EBBE9B4.3040009@tilera.com> <1321014494.2745.7.camel@br98xy6r> In-Reply-To: <1321014494.2745.7.camel@br98xy6r> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: kexec-bounces@lists.infradead.org Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: holzheu@linux.vnet.ibm.com Cc: Benjamin Herrenschmidt , Heiko Carstens , David Howells , Chen Liqin , Paul Mackerras , "H. Peter Anvin" , Guan Xuetao , Lennox Wu , Hans-Christian Egtvedt , Jonas Bonn , Jesper Nilsson , Russell King , Yoshinori Sato , "David S. Miller" , Richard Weinberger , Helge Deller , "James E.J. Bottomley" , Ingo Molnar , Geert Uytterhoeven , linux-arch@vger.kernel.org, Matt Turner , Vivek Goyal , Haavard Skinnemoen , Don Zickus , Fenghua Yu , Mike Frysinger , Peter Zijlstra , Jeff Dike , Mikael Starvik , Ivan Kokshaysky , Thomas Gleixner , Richard Henderson , Chris Zankel , Michal Simek , Tony Luck , kexec@lists.infradead.org, linux-kernel@vger.kernel.org, Ralf Baechle , Richard Kuo , Kyle McMartin , Paul Mundt , "Eric W. Biederman" , Martin Schwidefsky , Andrew Morton , Koichi Yasutake , Hirokazu Takata On 11/11/2011 7:28 AM, Michael Holzheu wrote: > Hello Chris, > > On Thu, 2011-11-10 at 10:11 -0500, Chris Metcalf wrote: >> On 11/10/2011 9:22 AM, Michael Holzheu wrote: > [snip] > >> If a cleaner API seems useful (either for power reasons or restartability >> or whatever), I suppose a standard global function name could be specified >> that's the thing you execute when you get an smp_send_stop IPI (in tile's >> case it's "smp_stop_cpu_interrupt()") and the panic() code could instead >> just do an atomic_inc_return() of a global panic counter, and if it wasn't >> the first panicking cpu, call directly into the smp_stop handler routine to >> quiesce itself. Then the panicking cpu could finish whatever it needs to >> do and then halt, reboot, etc., all the cpus. > Thanks for the info. So introducing a "weak" function that can stop the > CPU it is running on could solve the problem. Every architecture can > override the function with something appropriate. E.g. "tile" can use > the lower-power "nap" instruction there. > > What about the following patch. Seems reasonable to me. Acked-by: Chris Metcalf > > Michael > --- > From: Michael Holzheu > Subject: kdump: fix crash_kexec()/smp_send_stop() race in panic > > When two CPUs call panic at the same time there is a possible race > condition that can stop kdump. The first CPU calls crash_kexec() and the > second CPU calls smp_send_stop() in panic() before crash_kexec() finished > on the first CPU. So the second CPU stops the first CPU and therefore > kdump fails: > > 1st CPU: > panic()->crash_kexec()->mutex_trylock(&kexec_mutex)-> do kdump > > 2nd CPU: > panic()->crash_kexec()->kexec_mutex already held by 1st CPU > ->smp_send_stop()-> stop 1st CPU (stop kdump) > > This patch fixes the problem by introducing a spinlock in panic that > allows only one CPU to process crash_kexec() and the subsequent panic > code. > > Signed-off-by: Michael Holzheu > --- > kernel/panic.c | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -49,6 +49,15 @@ static long no_blink(int state) > long (*panic_blink)(int state); > EXPORT_SYMBOL(panic_blink); > > +/* > + * Stop ourself in panic -- architecture code may override this > + */ > +void __attribute__ ((weak)) panic_smp_self_stop(void) > +{ > + while (1) > + cpu_relax(); > +} > + > /** > * panic - halt the system > * @fmt: The text string to print > @@ -59,6 +68,7 @@ EXPORT_SYMBOL(panic_blink); > */ > NORET_TYPE void panic(const char * fmt, ...) > { > + static DEFINE_SPINLOCK(panic_lock); > static char buf[1024]; > va_list args; > long i, i_next = 0; > @@ -68,8 +78,14 @@ NORET_TYPE void panic(const char * fmt, > * It's possible to come here directly from a panic-assertion and > * not have preempt disabled. Some functions called from here want > * preempt to be disabled. No point enabling it later though... > + * > + * 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(). > */ > - preempt_disable(); > + if (!spin_trylock(&panic_lock)) > + panic_smp_self_stop(); > > console_verbose(); > bust_spinlocks(1); > > -- Chris Metcalf, Tilera Corp. http://www.tilera.com _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec