Skip to contentSkip to conversation

2.6.5 BUG if smp_call_function is called with interrupts disabled

PATCH In review Keith Owens 2 participants 5 messages Apr 27, 2004

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 2.6.5 BUG if smp_call_function is called with interrupts disabled

There is a nasty race if smp_call_function() is called with interrupts disabled.

CPU A CPU B
Disable interrupts
smp_call_function()
Take call_lock
Send IPIs
Wait for all cpus to acknowledge IPI CPU A has not responded, spin waiting for cpu A to respond, holding call_lock
smp_call_function()
Spin waiting for call_lock
Deadlock Deadlock

This bug is hard to reproduce and even harder to diagnose. Since the comments at the start of smp_call_function() say it should never be entered with interrupts disabled, make it so.

Index: linux/arch/ia64/kernel/smp.c
=================================--- linux.orig/arch/ia64/kernel/smp.c Mon Apr 26 15:20:17 2004
+++ linux/arch/ia64/kernel/smp.c Tue Apr 27 09:56:55 2004
@@ -319,10 +319,14 @@
{
struct call_data_struct data;
int cpus = num_online_cpus()-1;
+ unsigned long psr;

if (!cpus)
return 0;

+ __asm__ __volatile__ ("mov %0=psr;; " : "=r" (psr));
+ BUG_ON(!(psr & IA64_PSR_I));
+
data.func = func;
data.info = info;
atomic_set(&data.started, 0);

#1 lore raw
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Keith Owens

Keith> + __asm__ __volatile__ ("mov %0=psr;; " : "=r" (psr))

Please no inline-asm. Especially when there is a perfectly suitable macro already (local_save_flags).

	--david
#2 lore raw
In reply to Keith Owens
Quoted 6 lines · David Mosberger
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Keith Owens

Keith> + __asm__ __volatile__ ("mov %0=psr;; " : "=r" (psr))

Please no inline-asm. Especially when there is a perfectly suitable macro already (local_save_flags).

Sorry, ported from an older 2.4 kernel. Redo.

There is a nasty race if smp_call_function() is called with interrupts disabled.

CPU A CPU B
Disable interrupts
smp_call_function()
Take call_lock
Send IPIs
Wait for all cpus to acknowledge IPI CPU A has not responded, spin waiting for cpu A to respond, holding call_lock
smp_call_function()
Spin waiting for call_lock
Deadlock Deadlock

This bug is hard to reproduce and even harder to diagnose. Since the comments at the start of smp_call_function() say it should never be entered with interrupts disabled, make it so.

Index: linux/arch/ia64/kernel/smp.c
=================================--- linux.orig/arch/ia64/kernel/smp.c Mon Apr 26 15:20:17 2004
+++ linux/arch/ia64/kernel/smp.c Tue Apr 27 10:36:58 2004
@@ -319,10 +319,14 @@
{
struct call_data_struct data;
int cpus = num_online_cpus()-1;
+ unsigned long psr;

if (!cpus)
return 0;

+ local_save_flags(psr);
+ BUG_ON(!(psr & IA64_PSR_I));
+
data.func = func;
data.info = info;
atomic_set(&data.started, 0);

#3 lore raw
In reply to Keith Owens
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line · Keith Owens

Keith> Sorry, ported from an older 2.4 kernel. Redo.

Sorry for doing this in an incremental fashion, but upon re-reading your patch, I vaguely remembered that we had a macro to check whether irqs are disabled. Sure enough, there is:

irqs_disabled()

--david

#4 lore raw
In reply to Keith Owens

Take 3.

There is a nasty race if smp_call_function() is called with interrupts disabled.

CPU A CPU B
Disable interrupts
smp_call_function()
Take call_lock
Send IPIs
Wait for all cpus to acknowledge IPI CPU A has not responded, spin waiting for cpu A to respond, holding call_lock
smp_call_function()
Spin waiting for call_lock
Deadlock Deadlock

This bug is hard to reproduce and even harder to diagnose. Since the comments at the start of smp_call_function() say it should never be entered with interrupts disabled, make it so.

Index: linux/arch/ia64/kernel/smp.c
=================================--- linux.orig/arch/ia64/kernel/smp.c Mon Apr 26 15:20:17 2004
+++ linux/arch/ia64/kernel/smp.c Tue Apr 27 11:30:48 2004
@@ -323,6 +323,8 @@
if (!cpus)
return 0;

+ BUG_ON(irqs_disabled());
+
data.func = func;
data.info = info;
atomic_set(&data.started, 0);