Subject: [PATCH] linux-acpi: smp_alternatives sleeping in spinlock

Subject: [PATCH] linux-acpi: smp_alternatives sleeping in spinlock

[Raz]

From: Raz Ben Yehuda <raziebe@gmail.com>

When booting a kernel with PREEMPT_ENABLE and SLAB_DEBUG, unplugging a processor
results in BUG in slab.
Signed-off-by: Raz Ben Yehuda <raziebe@gmail.com>
---
arch/x86/kernel/alternative.c |   17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
this bug fix refers to bug numner 11562 in bugzilla.kernel.org

diff -urpN -X dontdiff
linux-2.6.26-5-vanilla/arch/x86/kernel/alternative.c
linux-2.6.26-5-dbg/arch/x86/kernel/alternative.c
--- linux-2.6.26-5-vanilla/arch/x86/kernel/alternative.c	2008-07-13
23:51:29.000000000 +0200
+++ linux-2.6.26-5-dbg/arch/x86/kernel/alternative.c	2008-09-13
15:31:52.000000000 +0200
@@ -13,6 +13,7 @@
 #include <asm/vsyscall.h>
 #include <asm/cacheflush.h>
 #include <asm/io.h>
+#include <linux/semaphore.h>

 #define MAX_PATCH_LEN (255-1)

@@ -279,7 +280,7 @@ struct smp_alt_module {
 	struct list_head next;
 };
 static LIST_HEAD(smp_alt_modules);
-static DEFINE_SPINLOCK(smp_alt);
+static __DECLARE_SEMAPHORE_GENERIC(smp_alt_lock, 1);
 static int smp_mode = 1;	/* protected by smp_alt */

 void alternatives_smp_module_add(struct module *mod, char *name,
@@ -312,12 +313,12 @@ void alternatives_smp_module_add(struct
 		__func__, smp->locks, smp->locks_end,
 		smp->text, smp->text_end, smp->name);

-	spin_lock(&smp_alt);
+	down(&smp_alt_lock);
 	list_add_tail(&smp->next, &smp_alt_modules);
 	if (boot_cpu_has(X86_FEATURE_UP))
 		alternatives_smp_unlock(smp->locks, smp->locks_end,
 					smp->text, smp->text_end);
-	spin_unlock(&smp_alt);
+	up(&smp_alt_lock);
 }

 void alternatives_smp_module_del(struct module *mod)
@@ -327,17 +328,17 @@ void alternatives_smp_module_del(struct
 	if (smp_alt_once || noreplace_smp)
 		return;

-	spin_lock(&smp_alt);
+	down(&smp_alt_lock);
 	list_for_each_entry(item, &smp_alt_modules, next) {
 		if (mod != item->mod)
 			continue;
 		list_del(&item->next);
-		spin_unlock(&smp_alt);
+		up(&smp_alt_lock);
 		DPRINTK("%s: %s\n", __func__, item->name);
 		kfree(item);
 		return;
 	}
-	spin_unlock(&smp_alt);
+	up(&smp_alt_lock);
 }

 void alternatives_smp_switch(int smp)
@@ -359,7 +360,7 @@ void alternatives_smp_switch(int smp)
 		return;
 	BUG_ON(!smp && (num_online_cpus() > 1));

-	spin_lock(&smp_alt);
+	down(&smp_alt_lock);

 	/*
 	 * Avoid unnecessary switches because it forces JIT based VMs to
@@ -383,7 +384,7 @@ void alternatives_smp_switch(int smp)
 						mod->text, mod->text_end);
 	}
 	smp_mode = smp;
-	spin_unlock(&smp_alt);
+	up(&smp_alt_lock);
 }

 #endif

Re: Subject: [PATCH] linux-acpi: smp_alternatives sleeping in spinlock

[Ingo Molnar]

* Raz <raziebe@gmail.com> wrote:

> From: Raz Ben Yehuda <raziebe@gmail.com>
> 
> When booting a kernel with PREEMPT_ENABLE and SLAB_DEBUG, unplugging a 
> processor results in BUG in slab.

could you please post that BUG? (and which version of the kernel you 
have tried, and exactly what you did to trigger this bug)

note that the conversion to a sleeping lock:

> -static DEFINE_SPINLOCK(smp_alt);
> +static __DECLARE_SEMAPHORE_GENERIC(smp_alt_lock, 1);

is already done correctly in the latest upstream kernel, see this 
commit:

 # 2f1dafe: x86: fix SMP alternatives: use mutex instead of spinlock

the better solution is to use a mutex, not a semaphore. This fix is part 
of the v2.6.26 kernel.

	Ingo

Re: Subject: [PATCH] linux-acpi: smp_alternatives sleeping in spinlock

[Raz]

On Sun, Sep 14, 2008 at 3:18 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
> * Raz <raziebe@gmail.com> wrote:
>
>> From: Raz Ben Yehuda <raziebe@gmail.com>
>>
>> When booting a kernel with PREEMPT_ENABLE and SLAB_DEBUG, unplugging a
>> processor results in BUG in slab.
>
> could you please post that BUG? (and which version of the kernel you
> have tried, and exactly what you did to trigger this bug)

Latest working kernel version: unknown
Earliest failing kernel version: 2.6.26.5  <-- this is the kernel I tested.
Distribution:centos 5.0 64bit
Hardware Environment: Lenovo T61 Dual core.
Software Environment:
Problem Description:
1. configure kernel as describe in Documentation/submitCheckList section 12.
2. boot this kernel
3. run . echo 0 >  /sys/devices/system/node/node0/cpu1/online

result:

SMP alternatives: switching to UP code
BUG: sleeping function called from invalid context at mm/slab.c:3052
in_atomic():1, irqs_disabled():0
INFO: lockdep is turned off.
Pid: 2463, comm: bash Not tainted 2.6.26.5 #28

Call Trace:
 [<ffffffff80251c61>] ? __debug_show_held_locks+0x1b/0x24
 [<ffffffff8022aae6>] __might_sleep+0x108/0x10a
 [<ffffffff802962fd>] kmem_cache_alloc_node+0x39/0x1a3
 [<ffffffff80289432>] ? __get_vm_area_node+0xa4/0x1d7
 [<ffffffff80289432>] __get_vm_area_node+0xa4/0x1d7
 [<ffffffff8020a3d7>] ? disable_TSC+0x17/0x53
 [<ffffffff802895c5>] get_vm_area_caller+0x2f/0x31
 [<ffffffff804b2653>] ? text_poke+0x11d/0x19a
 [<ffffffff80289d40>] vmap+0x31/0x63
 [<ffffffff804b7b62>] ? _etext+0x0/0xe
 [<ffffffff804b2653>] text_poke+0x11d/0x19a
 [<ffffffff804b7b62>] ? _etext+0x0/0xe
 [<ffffffff802118d4>] alternatives_smp_unlock+0x4f/0x63
 [<ffffffff80211b77>] alternatives_smp_switch+0x161/0x19e
 [<ffffffff8021b80c>] __cpu_die+0x5c/0x86
 [<ffffffff8049d047>] _cpu_down+0x1b5/0x28d
 [<ffffffff8049d145>] cpu_down+0x26/0x36
 [<ffffffff8049e306>] store_online+0x32/0x75
 [<ffffffff8037402e>] sysdev_store+0x24/0x26
 [<ffffffff802e3134>] sysfs_write_file+0xe5/0x121
 [<ffffffff8029dddc>] vfs_write+0xae/0x124
 [<ffffffff8029e320>] sys_write+0x47/0x70
 [<ffffffff8020bffb>] system_call_after_swapgs+0x7b/0x80

Steps to reproduce:

> note that the conversion to a sleeping lock:
>
>> -static DEFINE_SPINLOCK(smp_alt);
>> +static __DECLARE_SEMAPHORE_GENERIC(smp_alt_lock, 1);
>
> is already done correctly in the latest upstream kernel, see this
> commit:
>
>  # 2f1dafe: x86: fix SMP alternatives: use mutex instead of spinlock
>
> the better solution is to use a mutex, not a semaphore. This fix is part
> of the v2.6.26 kernel.
>
>        Ingo
>
yes. I understand.

Re: Subject: [PATCH] linux-acpi: smp_alternatives sleeping in spinlock

[Ingo Molnar]

* Raz <raziebe@gmail.com> wrote:

> > is already done correctly in the latest upstream kernel, see this 
> > commit:
> >
> >  # 2f1dafe: x86: fix SMP alternatives: use mutex instead of spinlock
> >
> > the better solution is to use a mutex, not a semaphore. This fix is 
> > part of the v2.6.26 kernel.
>
> yes. I understand.

ah, sorry - it's actually part of .27-rc, so not yet part of the stable 
kernel.

I've Cc:-ed stable@kernel.org. Stable folks, please apply the commit 
below to -stable. I've checked that it cherry-picks cleanly on v2.6.26.

	Ingo

-------------->
>From 2f1dafe50cc4e58a239fd81bd47f87f32042a1ee Mon Sep 17 00:00:00 2001
From: Pekka Paalanen <pq@iki.fi>
Date: Mon, 12 May 2008 21:21:01 +0200
Subject: [PATCH] x86: fix SMP alternatives: use mutex instead of spinlock, text_poke is sleepable

text_poke is sleepable.
The original fix by Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>.

Signed-off-by: Pekka Paalanen <pq@iki.fi>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 arch/x86/kernel/alternative.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index de240ba..2763cb3 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1,6 +1,6 @@
 #include <linux/module.h>
 #include <linux/sched.h>
-#include <linux/spinlock.h>
+#include <linux/mutex.h>
 #include <linux/list.h>
 #include <linux/kprobes.h>
 #include <linux/mm.h>
@@ -279,7 +279,7 @@ struct smp_alt_module {
 	struct list_head next;
 };
 static LIST_HEAD(smp_alt_modules);
-static DEFINE_SPINLOCK(smp_alt);
+static DEFINE_MUTEX(smp_alt);
 static int smp_mode = 1;	/* protected by smp_alt */
 
 void alternatives_smp_module_add(struct module *mod, char *name,
@@ -312,12 +312,12 @@ void alternatives_smp_module_add(struct module *mod, char *name,
 		__func__, smp->locks, smp->locks_end,
 		smp->text, smp->text_end, smp->name);
 
-	spin_lock(&smp_alt);
+	mutex_lock(&smp_alt);
 	list_add_tail(&smp->next, &smp_alt_modules);
 	if (boot_cpu_has(X86_FEATURE_UP))
 		alternatives_smp_unlock(smp->locks, smp->locks_end,
 					smp->text, smp->text_end);
-	spin_unlock(&smp_alt);
+	mutex_unlock(&smp_alt);
 }
 
 void alternatives_smp_module_del(struct module *mod)
@@ -327,17 +327,17 @@ void alternatives_smp_module_del(struct module *mod)
 	if (smp_alt_once || noreplace_smp)
 		return;
 
-	spin_lock(&smp_alt);
+	mutex_lock(&smp_alt);
 	list_for_each_entry(item, &smp_alt_modules, next) {
 		if (mod != item->mod)
 			continue;
 		list_del(&item->next);
-		spin_unlock(&smp_alt);
+		mutex_unlock(&smp_alt);
 		DPRINTK("%s: %s\n", __func__, item->name);
 		kfree(item);
 		return;
 	}
-	spin_unlock(&smp_alt);
+	mutex_unlock(&smp_alt);
 }
 
 void alternatives_smp_switch(int smp)
@@ -359,7 +359,7 @@ void alternatives_smp_switch(int smp)
 		return;
 	BUG_ON(!smp && (num_online_cpus() > 1));
 
-	spin_lock(&smp_alt);
+	mutex_lock(&smp_alt);
 
 	/*
 	 * Avoid unnecessary switches because it forces JIT based VMs to
@@ -383,7 +383,7 @@ void alternatives_smp_switch(int smp)
 						mod->text, mod->text_end);
 	}
 	smp_mode = smp;
-	spin_unlock(&smp_alt);
+	mutex_unlock(&smp_alt);
 }
 
 #endif