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 <cmetcalf at tilera.com>
>
> Michael
> ---
> From: Michael Holzheu<holzheu at linux.vnet.ibm.com>
> 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<holzheu at linux.vnet.ibm.com>
> ---
> 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
