Skip to contentSkip to conversation

bug_smp_call_function

PATCH In review Keith Owens 3 participants 7 messages Jun 19, 2004

Subsystems (6)
MIPS s390 SuperH User-mode Linux (UML) +2 more subsystems
lore
Share & tools
lore

Keith Owens

author
#0 lore raw

PATCH 1/1 bug_smp_call_function

sg.c has been fixed to no longer call vfree() with interrupts disabled. Change smp_call_function() from WARN_ON to BUG_ON when interrupts are disabled. It was only set to WARN_ON because of sg.c.

Signed-off-by Keith Owens <[email protected]>
arch/alpha/kernel/smp.c +1 -1
Index: 2.6.7/arch/alpha/kernel/smp.c
===================================================================
@@ -821,7 +821,7 @@ smp_call_function_on_cpu (void (*func) (
 	int num_cpus_to_call;
 	
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/i386/kernel/smp.c +1 -1
Index: 2.6.7/arch/i386/kernel/smp.c
===================================================================
@@ -520,7 +520,7 @@ int smp_call_function (void (*func) (voi
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/ia64/kernel/smp.c +1 -1
Index: 2.6.7/arch/ia64/kernel/smp.c
===================================================================
@@ -332,7 +332,7 @@ smp_call_function (void (*func) (void *i
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/mips/kernel/smp.c +1 -1
Index: 2.6.7/arch/mips/kernel/smp.c
===================================================================
@@ -152,7 +152,7 @@ int smp_call_function (void (*func) (voi
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/parisc/kernel/smp.c +1 -1
Index: 2.6.7/arch/parisc/kernel/smp.c
===================================================================
@@ -326,7 +326,7 @@ smp_call_function (void (*func) (void *i
 	static spinlock_t lock = SPIN_LOCK_UNLOCKED;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 	
 	data.func = func;
 	data.info = info;
arch/ppc/kernel/smp.c +1 -1
Index: 2.6.7/arch/ppc/kernel/smp.c
===================================================================
@@ -212,7 +212,7 @@ int smp_call_function(void (*func) (void
 	if (num_online_cpus() <= 1)
 		return 0;
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 	return __smp_call_function(func, info, wait, MSG_ALL_BUT_SELF);
 }
arch/ppc64/kernel/smp.c +1 -1
Index: 2.6.7/arch/ppc64/kernel/smp.c
===================================================================
@@ -692,7 +692,7 @@ int smp_call_function (void (*func) (voi
 	unsigned long timeout;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/s390/kernel/smp.c +1 -1
Index: 2.6.7/arch/s390/kernel/smp.c
===================================================================
@@ -129,7 +129,7 @@ int smp_call_function (void (*func) (voi
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/sh/kernel/smp.c +1 -1
Index: 2.6.7/arch/sh/kernel/smp.c
===================================================================
@@ -182,7 +182,7 @@ int smp_call_function(void (*func)(void 
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	spin_lock(&smp_fn_call.lock);
arch/sparc64/kernel/smp.c +1 -1
Index: 2.6.7/arch/sparc64/kernel/smp.c
===================================================================
@@ -602,7 +602,7 @@ int smp_call_function(void (*func)(void 
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
arch/um/kernel/smp.c +1 -1
Index: 2.6.7/arch/um/kernel/smp.c
===================================================================
@@ -267,7 +267,7 @@ int smp_call_function(void (*_func)(void
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	spin_lock_bh(&call_lock);
 	atomic_set(&scf_started, 0);
arch/x86_64/kernel/smp.c +1 -1
Index: 2.6.7/arch/x86_64/kernel/smp.c
===================================================================
@@ -405,7 +405,7 @@ int smp_call_function (void (*func) (voi
 		return 0;
 
 	/* Can deadlock when called with interrupts disabled */
-	WARN_ON(irqs_disabled());
+	BUG_ON(irqs_disabled());
 
 	data.func = func;
 	data.info = info;
#1 lore raw
Quoted 4 lines · Keith Owens

sg.c has been fixed to no longer call vfree() with interrupts disabled. Change smp_call_function() from WARN_ON to BUG_ON when interrupts are disabled. It was only set to WARN_ON because of sg.c.

I prefer the WARN_ON. It is exceedingly unlikely that the bug will cause lockups or memory/data corruption or anything else, so why nuke the user's box when we can trivially continue?

We'll be sent the bug report either way.

author
#2 lore raw
Quoted 11 lines · Andrew Morton
Quoted 4 lines · Keith Owens

sg.c has been fixed to no longer call vfree() with interrupts disabled. Change smp_call_function() from WARN_ON to BUG_ON when interrupts are disabled. It was only set to WARN_ON because of sg.c.

I prefer the WARN_ON. It is exceedingly unlikely that the bug will cause lockups or memory/data corruption or anything else, so why nuke the user's box when we can trivially continue?

We'll be sent the bug report either way.

I prefer to catch this bug every time instead of assuming that somebody will see the report in the syslog. Once the kernel code is clean (which it should be now), BUG_ON() will prevent any new code reintroducing this error.

#3 lore raw
In reply to Andrew Morton
Quoted 3 lines · Keith Owens
Quoted 3 lines

sg.c has been fixed to no longer call vfree() with interrupts disabled. Change smp_call_function() from WARN_ON to BUG_ON when interrupts are disabled. It was only set to WARN_ON because of sg.c.

Quoted 4 lines · Andrew Morton

I prefer the WARN_ON. It is exceedingly unlikely that the bug will cause lockups or memory/data corruption or anything else, so why nuke the user's box when we can trivially continue?
We'll be sent the bug report either way.

Calls to smp_call_function() with interrupts off or spinlocks held typically causes deadlocks on SMP systems. ISTR debugging such an issue in the scheduler a while back, i.e. mmdrop() under rq->lock doing vfree() of an LDT. Basically smp_call_function() will spin waiting for the other cpus to answer the interrupt on multiple cpus. It also doesn't need to be the same function doing smp_call_function(); generally TLB flushing deadlocks against anything doing this.

-- wli

author
#4 lore raw
Quoted 11 lines · William Lee Irwin III
Quoted 3 lines · Keith Owens
Quoted 3 lines

sg.c has been fixed to no longer call vfree() with interrupts disabled. Change smp_call_function() from WARN_ON to BUG_ON when interrupts are disabled. It was only set to WARN_ON because of sg.c.

Quoted 4 lines · Andrew Morton

I prefer the WARN_ON. It is exceedingly unlikely that the bug will cause lockups or memory/data corruption or anything else, so why nuke the user's box when we can trivially continue?
We'll be sent the bug report either way.

Quoted 7 lines

Calls to smp_call_function() with interrupts off or spinlocks held typically causes deadlocks on SMP systems. ISTR debugging such an issue in the scheduler a while back, i.e. mmdrop() under rq->lock doing vfree() of an LDT. Basically smp_call_function() will spin waiting for the other cpus to answer the interrupt on multiple cpus. It also doesn't need to be the same function doing smp_call_function(); generally TLB flushing deadlocks against anything doing this.

Agreed, that is exactly the class of problems that I spent days debugging. WARN_ON() lets developers add code that breaks the rules and assumes that we will have to fix the bad code later. BUG_ON() prevents any bad code being added because it catches the developer as soon as they add it.

#5 lore raw
In reply to William Lee Irwin III
Quoted 6 lines · William Lee Irwin III
Quoted 3 lines · Keith Owens
Quoted 3 lines

sg.c has been fixed to no longer call vfree() with interrupts disabled. Change smp_call_function() from WARN_ON to BUG_ON when interrupts are disabled. It was only set to WARN_ON because of sg.c.

Quoted 8 lines
Quoted 4 lines · Andrew Morton

I prefer the WARN_ON. It is exceedingly unlikely that the bug will cause lockups or memory/data corruption or anything else, so why nuke the user's box when we can trivially continue?
We'll be sent the bug report either way.

Calls to smp_call_function() with interrupts off or spinlocks held typically causes deadlocks on SMP systems.

No, this doesn't "typically" deadlock. It will deadlock on every ten millionth call. The preceding 9,999,999 warnings should have imparted sufficient clue?

#6 lore raw
Quoted 2 lines · William Lee Irwin III
Quoted 2 lines

Calls to smp_call_function() with interrupts off or spinlocks held typically causes deadlocks on SMP systems.

Quoted 3 lines · Andrew Morton

No, this doesn't "typically" deadlock. It will deadlock on every ten millionth call. The preceding 9,999,999 warnings should have imparted sufficient clue?

I wasn't counting function calls. I suppose it won't deadlock on every call.

-- wli