Hi,
I noticed a scenario where an i386 kernel would print warning messages
about smp_send_stop() being called with interrupts off. ?This can happen
both from panic() and from machine_restart in arch/i386/kernel/reboot.c
if called from an interrupt handler. ?To reproduce I added a sysrq handler
that simply called panic().
I noticed that the same behavior did not occur on the x86_64 architecture.
In investigating the differences between the architectures, I found the
patch with commit ID e6e7c2a9222016f41613d2389d230b03a36c9f20 during the
2.6.12-rc2 timeframe which addressed this issue for the x86_64 architecture.
I modified the i386/kernel/smp.c file to be functionally equivalent to the
x86_64/kernel/smp.c file to address this issue.
Comments?
Drew
Signed-off-by: Drew Moseley <[email protected]>
Description:
? ? Make arch/i386/kernel/smp.c functionally equivalent to
arch/x86_64/kernel/smp.c
? ? This allows for the case of calling smp_send_stop when interrupts are
disabled
? ? in a few isolated cases.
Index: linux-2.6.10/arch/i386/kernel/smp.c
===================================================================
--- linux-2.6.10.orig/arch/i386/kernel/smp.c
+++ linux-2.6.10/arch/i386/kernel/smp.c
@@ -560,30 +560,14 @@ void dump_send_ipi(void)
? * this function sends a 'generic call function' IPI to all other CPUs
? * in the system.
? */
-
-int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
-???????????????????????int wait)
-/*
- * [SUMMARY] Run a function on all other CPUs.
- * <func> The function to run. This must be fast and non-blocking.
- * <info> An arbitrary pointer to pass to the function.
- * <nonatomic> currently unused.
- * <wait> If true, wait (atomically) until function has completed on other
CPUs.
- * [RETURNS] 0 on success, else a negative status code. Does not return until
- * remote CPUs are nearly ready to execute <<func>> or are or have executed.
- *
- * You must not call this function with disabled interrupts or from a
- * hardware interrupt handler or from a bottom half handler.
- */
+static void __smp_call_function (void (*func) (void *info), void *info,
+???????????????????????????????int nonatomic, int wait)
?{
????????struct call_data_struct data;
????????int cpus = num_online_cpus()-1;
?
????????if (!cpus)
-???????????????return 0;
-
-???????/* Can deadlock when called with interrupts disabled */
-???????WARN_ON(irqs_disabled());
+???????????????return;
?
????????data.func = func;
????????data.info = info;
@@ -592,7 +576,6 @@ int smp_call_function (void (*func) (voi
????????if (wait)
????????????????atomic_set(&data.finished, 0);
?
-???????spin_lock(&call_lock);
????????call_data = &data;
????????mb();
????????
@@ -603,11 +586,32 @@ int smp_call_function (void (*func) (voi
????????while (atomic_read(&data.started) != cpus)
????????????????cpu_relax();
?
-???????if (wait)
-???????????????while (atomic_read(&data.finished) != cpus)
-???????????????????????cpu_relax();
-???????spin_unlock(&call_lock);
+???????if (!wait)
+???????????????return;
+
+???????while (atomic_read(&data.finished) != cpus)
+???????????????cpu_relax();
+}
?
+/*
+ * [SUMMARY] Run a function on all other CPUs.
+ * <func> The function to run. This must be fast and non-blocking.
+ * <info> An arbitrary pointer to pass to the function.
+ * <nonatomic> currently unused.
+ * <wait> If true, wait (atomically) until function has completed on other
CPUs.
+ * [RETURNS] 0 on success, else a negative status code. Does not return until
+ * remote CPUs are nearly ready to execute <<func>> or are or have executed.
+ *
+ * You must not call this function with disabled interrupts or from a
+ * hardware interrupt handler or from a bottom half handler.
+ * Actually there are a few legal cases, like panic.
+ */
+int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
+???????????????????????int wait)
+{
+???????spin_lock(&call_lock);
+???????__smp_call_function(func,info,nonatomic,wait);
+???????spin_unlock(&call_lock);
????????return 0;
?}
?
@@ -630,7 +634,15 @@ void stop_this_cpu (void * dummy)
?
?void smp_send_stop(void)
?{
-???????smp_call_function(stop_this_cpu, NULL, 1, 0);
+???????int nolock = 0;
+???????/* Don't deadlock on the call lock in panic */
+???????if (!spin_trylock(&call_lock)) {
+???????????????/* ignore locking because we have paniced anyways */
+???????????????nolock = 1;
+???????}
+???????__smp_call_function(stop_this_cpu, NULL, 0, 0);
+???????if (!nolock)
+???????????????spin_unlock(&call_lock);
?
????????local_irq_disable();
????????disable_local_APIC();
Drew Moseley (on Tue, 16 May 2006 19:43:05 -0700) wrote:
>I noticed a scenario where an i386 kernel would print warning messages
>about smp_send_stop() being called with interrupts off. This can happen
>both from panic() and from machine_restart in arch/i386/kernel/reboot.c
>if called from an interrupt handler. To reproduce I added a sysrq handler
>that simply called panic().
>
>I noticed that the same behavior did not occur on the x86_64 architecture.
>In investigating the differences between the architectures, I found the
>patch with commit ID e6e7c2a9222016f41613d2389d230b03a36c9f20 during the
>2.6.12-rc2 timeframe which addressed this issue for the x86_64 architecture.
>
>I modified the i386/kernel/smp.c file to be functionally equivalent to the
>x86_64/kernel/smp.c file to address this issue.
>
>Comments?
>
>Drew
>
>
>
>
>
>
>Signed-off-by: Drew Moseley <[email protected]>
>Description:
> Make arch/i386/kernel/smp.c functionally equivalent to
>arch/x86_64/kernel/smp.c
> This allows for the case of calling smp_send_stop when interrupts are
>disabled
> in a few isolated cases.
>
>Index: linux-2.6.10/arch/i386/kernel/smp.c
>===================================================================
>--- linux-2.6.10.orig/arch/i386/kernel/smp.c
>+++ linux-2.6.10/arch/i386/kernel/smp.c
>@@ -560,30 +560,14 @@ void dump_send_ipi(void)
> * this function sends a 'generic call function' IPI to all other CPUs
> * in the system.
> */
>-
>-int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
>- int wait)
>-/*
>- * [SUMMARY] Run a function on all other CPUs.
>- * <func> The function to run. This must be fast and non-blocking.
>- * <info> An arbitrary pointer to pass to the function.
>- * <nonatomic> currently unused.
>- * <wait> If true, wait (atomically) until function has completed on other
>CPUs.
>- * [RETURNS] 0 on success, else a negative status code. Does not return until
>- * remote CPUs are nearly ready to execute <<func>> or are or have executed.
>- *
>- * You must not call this function with disabled interrupts or from a
>- * hardware interrupt handler or from a bottom half handler.
>- */
>+static void __smp_call_function (void (*func) (void *info), void *info,
>+ int nonatomic, int wait)
> {
> struct call_data_struct data;
> int cpus = num_online_cpus()-1;
>
> if (!cpus)
>- return 0;
>-
>- /* Can deadlock when called with interrupts disabled */
>- WARN_ON(irqs_disabled());
>+ return;
>
> data.func = func;
> data.info = info;
>@@ -592,7 +576,6 @@ int smp_call_function (void (*func) (voi
> if (wait)
> atomic_set(&data.finished, 0);
>
>- spin_lock(&call_lock);
> call_data = &data;
> mb();
>
>@@ -603,11 +586,32 @@ int smp_call_function (void (*func) (voi
> while (atomic_read(&data.started) != cpus)
> cpu_relax();
>
>- if (wait)
>- while (atomic_read(&data.finished) != cpus)
>- cpu_relax();
>- spin_unlock(&call_lock);
>+ if (!wait)
>+ return;
>+
>+ while (atomic_read(&data.finished) != cpus)
>+ cpu_relax();
>+}
>
>+/*
>+ * [SUMMARY] Run a function on all other CPUs.
>+ * <func> The function to run. This must be fast and non-blocking.
>+ * <info> An arbitrary pointer to pass to the function.
>+ * <nonatomic> currently unused.
>+ * <wait> If true, wait (atomically) until function has completed on other
>CPUs.
>+ * [RETURNS] 0 on success, else a negative status code. Does not return until
>+ * remote CPUs are nearly ready to execute <<func>> or are or have executed.
>+ *
>+ * You must not call this function with disabled interrupts or from a
>+ * hardware interrupt handler or from a bottom half handler.
>+ * Actually there are a few legal cases, like panic.
>+ */
>+int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
>+ int wait)
>+{
>+ spin_lock(&call_lock);
>+ __smp_call_function(func,info,nonatomic,wait);
>+ spin_unlock(&call_lock);
> return 0;
> }
>
>@@ -630,7 +634,15 @@ void stop_this_cpu (void * dummy)
>
> void smp_send_stop(void)
> {
>- smp_call_function(stop_this_cpu, NULL, 1, 0);
>+ int nolock = 0;
>+ /* Don't deadlock on the call lock in panic */
>+ if (!spin_trylock(&call_lock)) {
>+ /* ignore locking because we have paniced anyways */
>+ nolock = 1;
>+ }
>+ __smp_call_function(stop_this_cpu, NULL, 0, 0);
>+ if (!nolock)
>+ spin_unlock(&call_lock);
>
> local_irq_disable();
> disable_local_APIC();
NAK this patch. There is a real potential deadlock if you call
smp_call_function() with interrupts disabled, it has caused kernel
hangs in the past. This patch removes the check for the potential
deadlock and will allow bad code to creep back into the kernel. See
http://lkml.org/lkml/2004/5/2/116. I find it quite disturbing that
x86_64 has removed that check :(
On Tuesday 16 May 2006 9:56 pm, Keith Owens wrote:
> Drew Moseley (on Tue, 16 May 2006 19:43:05 -0700) wrote:
...
> >
> >I modified the i386/kernel/smp.c file to be functionally equivalent to the
> >x86_64/kernel/smp.c file to address this issue.
> >
...
>
> NAK this patch. There is a real potential deadlock if you call
> smp_call_function() with interrupts disabled, it has caused kernel
> hangs in the past. This patch removes the check for the potential
> deadlock and will allow bad code to creep back into the kernel. See
> http://lkml.org/lkml/2004/5/2/116. I find it quite disturbing that
> x86_64 has removed that check :(
So presumably the warn_on() call should be added back to the x86_64 arch?
Does that mean that the comment in the current code
* You must not call this function with disabled interrupts or from a
* hardware interrupt handler or from a bottom half handler.
* Actually there are a few legal cases, like panic.
is incorrect?
I did manage to find this thread; http://lkml.org/lkml/2004/12/2/209; which
seems related..
Does is sound reasonable then to call local_irq_enable() for the scenarios
where this will occur? Specifically, any call to smp_send_stop() where IRQ's
might be disabled may need that call to eliminate the deadlock opportunity.
Or am I just plain confused?
Drew
Drew Moseley (on Wed, 17 May 2006 11:34:57 -0700) wrote:
>On Tuesday 16 May 2006 9:56 pm, Keith Owens wrote:
>> Drew Moseley (on Tue, 16 May 2006 19:43:05 -0700) wrote:
>...
>> >
>> >I modified the i386/kernel/smp.c file to be functionally equivalent to the
>> >x86_64/kernel/smp.c file to address this issue.
>> >
>...
>>
>> NAK this patch. There is a real potential deadlock if you call
>> smp_call_function() with interrupts disabled, it has caused kernel
>> hangs in the past. This patch removes the check for the potential
>> deadlock and will allow bad code to creep back into the kernel. See
>> http://lkml.org/lkml/2004/5/2/116. I find it quite disturbing that
>> x86_64 has removed that check :(
>
>So presumably the warn_on() call should be added back to the x86_64 arch?
Yes. smp_call_function() in i386 has to stay the way it is, and x86_64
has to revert to be the same as i386 and all the other architectures,
including the WARN_ON(irqs_disabled()).
That still leaves the panic and sysrq cases, and anywhere else that
needs to do an emergency stop or reboot of the system from a context
where interrupts are disabled. I do not like the idea of reenabling
interrupts just to suppress a warning from smp_call_function(). By
definition, this is an unusual state and reenabling interrupts runs
significant risks of not doing what the caller wants.
The cleanest solution is the way that alpha, arm and ia64 do it.
smp_send_stop() sends IPI_CPU_STOP to all cpus and does not go through
smp_call_function(). That avoids all the problems of calling
smp_send_stop() in an emergency. If it works for ia64 with 1,024 cpus
in a single system image then it must be good :)
Alternatively smp_send_stop() could pass a special wait value into
smp_call_function() to say that interrupts being disabled is expected.
smp_call_function() would then skip its lock and it would skip waiting
for all the other cpus to respond, or at least timeout after a short
period and continue. That would avoid the deadlock. Like this (i386,
not even compiled, let alone tested).
/* wait == 0, do not wait for the function to complete on other cpus. wait ==
* 1, wait. wait < 0, do not take locks and do not wait forever, not irqsafe.
*/
int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
int wait)
{
struct call_data_struct data;
int cpus, timeout, irqsafe = wait >= 0;
cpus = num_online_cpus() - 1;
timeout = cpus + 1;
if (irqsafe) {
/* Holding any lock stops cpus from going down. */
spin_lock(&call_lock);
if (!cpus) {
spin_unlock(&call_lock);
return 0;
}
/* Can deadlock when called with interrupts disabled */
WARN_ON(irqs_disabled());
} else {
wait = 0;
}
data.func = func;
data.info = info;
atomic_set(&data.started, 0);
data.wait = wait;
if (wait)
atomic_set(&data.finished, 0);
call_data = &data;
mb();
/* Send a message to all other CPUs and wait for them to respond */
send_IPI_allbutself(CALL_FUNCTION_VECTOR);
/* Wait for response */
while (atomic_read(&data.started) != cpus) {
cpu_relax();
if (!irqsafe) {
mdelay(1);
if (timeout-- == 0)
break;
}
}
if (wait)
while (atomic_read(&data.finished) != cpus)
cpu_relax();
if (irqsafe)
spin_unlock(&call_lock);
return 0;
}
void smp_send_stop(void)
{
smp_call_function(stop_this_cpu, NULL, 1, irqs_disabled() ? -1 : 0);
local_irq_disable();
disable_local_APIC();
local_irq_enable();
}