Hello,
[ 100.666169] lockdep: fixing up alternatives.
[ 100.666183] SMP alternatives: switching to UP code
[ 106.942505] BUG: using smp_processor_id() in preemptible [00000000] code: bash/5165
[ 106.942541] caller is thermal_throttle_add_dev+0x20/0xa4
[ 106.942548] Pid: 5165, comm: bash Not tainted 2.6.36-rc1-dbg-git1-00187-g358afc9-dirty #132
[ 106.942553] Call Trace:
[ 106.942565] [<ffffffff811f6c01>] debug_smp_processor_id+0xc9/0xe4
[ 106.942571] [<ffffffff8136d377>] thermal_throttle_add_dev+0x20/0xa4
[ 106.942578] [<ffffffff8136d44d>] thermal_throttle_cpu_callback+0x52/0xb5
[ 106.942587] [<ffffffff810573f8>] notifier_call_chain+0x32/0x5e
[ 106.942595] [<ffffffff8103c840>] ? cpu_maps_update_begin+0x12/0x14
[ 106.942602] [<ffffffff81057443>] __raw_notifier_call_chain+0x9/0xb
[ 106.942609] [<ffffffff8103c703>] __cpu_notify+0x1b/0x2d
[ 106.942616] [<ffffffff8136f57d>] _cpu_up+0x6b/0xe9
[ 106.942622] [<ffffffff8136f645>] cpu_up+0x4a/0x57
[ 106.942629] [<ffffffff81363f77>] store_online+0x41/0x6e
[ 106.942637] [<ffffffff81291b4f>] sysdev_store+0x1b/0x1d
[ 106.942644] [<ffffffff81132186>] sysfs_write_file+0x103/0x13f
[ 106.942653] [<ffffffff810db026>] vfs_write+0xb0/0x14f
[ 106.942659] [<ffffffff810db2c2>] sys_write+0x45/0x6c
[ 106.942668] [<ffffffff81002002>] system_call_fastpath+0x16/0x1b
I have several solutions. Please kindly review.
The first one is:
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..1269661 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -205,7 +205,9 @@ static int therm_throt_process(bool new_event, int event, int level)
static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
{
int err;
- struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+ int cpu = get_cpu();
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
+ put_cpu();
err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
if (err)
Should we call smp_processor_id/get_cpu in thermal_throttle_add_dev?
We know the cpu when calling thermal_throttle_add_dev (in thermal_throttle_cpu_callback and
thermal_throttle_init_device) so we can pass the cpu number when calling it.
My second solution is:
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..bcd8499 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -202,11 +202,12 @@ static int therm_throt_process(bool new_event, int event, int level)
#ifdef CONFIG_SYSFS
/* Add/Remove thermal_throttle interface for CPU device: */
-static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
+static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
+ unsigned int cpu)
{
int err;
- struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
-
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
+
err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
if (err)
return err;
@@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
case CPU_UP_PREPARE:
case CPU_UP_PREPARE_FROZEN:
mutex_lock(&therm_cpu_lock);
- err = thermal_throttle_add_dev(sys_dev);
+ err = thermal_throttle_add_dev(sys_dev, cpu);
mutex_unlock(&therm_cpu_lock);
WARN_ON(err);
break;
@@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
#endif
/* connect live CPUs to sysfs */
for_each_online_cpu(cpu) {
- err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
+ err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
WARN_ON(err);
}
#ifdef CONFIG_HOTPLUG_CPU
On środa, 18 sierpnia 2010 o 11:11:58 Sergey Senozhatsky wrote:
> fix BUG: using smp_processor_id() in preemptible code (resend)
I got similar bug (during suspend to disk) on 2.6.36-rc1+ patch from:
https://bugzilla.kernel.org/show_bug.cgi?id=16615
[ 89.141370] Enabling non-boot CPUs ...
[ 89.142033] BUG: using smp_processor_id() in preemptible [00000000] code:
pm-hibernate/2584
[ 89.142147] caller is thermal_throttle_add_dev+0x10/0x7c
[ 89.142200] Pid: 2584, comm: pm-hibernate Not tainted 2.6.36-rc1 #1
[ 89.142248] Call Trace:
[ 89.142274] [<c03706ad>] ? printk+0xf/0x12
[ 89.142312] [<c02181d1>] debug_smp_processor_id+0xa5/0xbc
[ 89.142357] [<c036d13d>] thermal_throttle_add_dev+0x10/0x7c
[ 89.142403] [<c036d1f2>] thermal_throttle_cpu_callback+0x49/0x9c
[ 89.142454] [<c0141842>] notifier_call_chain+0x26/0x50
[ 89.142496] [<c01418c3>] __raw_notifier_call_chain+0xe/0x10
[ 89.142544] [<c012be4a>] __cpu_notify+0x15/0x29
[ 89.142581] [<c036ead6>] _cpu_up+0x59/0xdc
[ 89.142617] [<c0364953>] enable_nonboot_cpus+0x3d/0x94
[ 89.142660] [<c0151f94>] hibernation_snapshot+0x11d/0x1b9
[ 89.142704] [<c01520d5>] hibernate+0xa5/0x13f
[ 89.142740] [<c0151035>] ? state_store+0x0/0xa7
[ 89.142778] [<c0151089>] state_store+0x54/0xa7
[ 89.142815] [<c0151035>] ? state_store+0x0/0xa7
[ 89.142854] [<c020ee89>] kobj_attr_store+0x16/0x22
[ 89.142894] [<c01c9a9a>] sysfs_write_file+0xbc/0xe7
[ 89.142936] [<c018e5d3>] vfs_write+0x8a/0xfd
[ 89.142972] [<c01c99de>] ? sysfs_write_file+0x0/0xe7
[ 89.143018] [<c018e6dd>] sys_write+0x3b/0x60
[ 89.143055] [<c01026ec>] sysenter_do_call+0x12/0x22
[ 89.143112] SMP alternatives: switching to SMP code
PS. Sorry for overlook previosus mail, already I created bug entry (because
it's regression):
https://bugzilla.kernel.org/show_bug.cgi?id=16629
Regards
--
Maciej Rutecki
http://www.maciek.unixy.pl
On Wed, Aug 18, 2010 at 05:11:58AM -0400, Sergey Senozhatsky wrote:
> Should we call smp_processor_id/get_cpu in thermal_throttle_add_dev?
> We know the cpu when calling thermal_throttle_add_dev (in thermal_throttle_cpu_callback and
> thermal_throttle_init_device) so we can pass the cpu number when calling it.
>
> My second solution is:
>
> diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> index c2a8b26..bcd8499 100644
> --- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
> +++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
> @@ -202,11 +202,12 @@ static int therm_throt_process(bool new_event, int event, int level)
>
> #ifdef CONFIG_SYSFS
> /* Add/Remove thermal_throttle interface for CPU device: */
> -static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
> +static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
> + unsigned int cpu)
> {
> int err;
> - struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
> -
> + struct cpuinfo_x86 *c = &cpu_data(cpu);
> +
> err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
> if (err)
> return err;
> @@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
> case CPU_UP_PREPARE:
> case CPU_UP_PREPARE_FROZEN:
> mutex_lock(&therm_cpu_lock);
> - err = thermal_throttle_add_dev(sys_dev);
> + err = thermal_throttle_add_dev(sys_dev, cpu);
> mutex_unlock(&therm_cpu_lock);
> WARN_ON(err);
> break;
> @@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
> #endif
> /* connect live CPUs to sysfs */
> for_each_online_cpu(cpu) {
> - err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
> + err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
> WARN_ON(err);
> }
> #ifdef CONFIG_HOTPLUG_CPU
The notifier is preemtible so it can run on another CPU. So using the
supplied cpu arguemnt in the path looks like the right solution to me.
Joerg
--
AMD Operating System Research Center
Advanced Micro Devices GmbH Einsteinring 24 85609 Dornach
General Managers: Alberto Bozzo, Andrew Bowd
Registration: Dornach, Landkr. Muenchen; Registerger. Muenchen, HRB Nr. 43632
On 08/19/2010 10:03 AM, Roedel, Joerg wrote:
>
> The notifier is preemtible so it can run on another CPU. So using the
> supplied cpu arguemnt in the path looks like the right solution to me.
>
Sounds good -- Sergey, can you send a patch under cleaned up cover
(meaning just the one patch, with a description of why the change is the
right one)?
-hpa
Fix: using smp_processor_id() in preemptible thermal_throttle_add_dev.
We know the cpu number when calling thermal_throttle_add_dev, so we can
remove smp_processor_id call in thermal_throttle_add_dev by supplying
the cpu number as argument.
Signed-off-by: Sergey Senozhatsky <[email protected]>
---
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..bcd8499 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -202,11 +202,12 @@ static int therm_throt_process(bool new_event, int event, int level)
#ifdef CONFIG_SYSFS
/* Add/Remove thermal_throttle interface for CPU device: */
-static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
+static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
+ unsigned int cpu)
{
int err;
- struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
-
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
+
err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
if (err)
return err;
@@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
case CPU_UP_PREPARE:
case CPU_UP_PREPARE_FROZEN:
mutex_lock(&therm_cpu_lock);
- err = thermal_throttle_add_dev(sys_dev);
+ err = thermal_throttle_add_dev(sys_dev, cpu);
mutex_unlock(&therm_cpu_lock);
WARN_ON(err);
break;
@@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
#endif
/* connect live CPUs to sysfs */
for_each_online_cpu(cpu) {
- err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
+ err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
WARN_ON(err);
}
#ifdef CONFIG_HOTPLUG_CPU
Commit-ID: 51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
Gitweb: http://git.kernel.org/tip/51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
Author: Sergey Senozhatsky <[email protected]>
AuthorDate: Fri, 20 Aug 2010 10:36:34 +0300
Committer: H. Peter Anvin <[email protected]>
CommitDate: Fri, 20 Aug 2010 19:56:00 -0700
x86, hwmon: Fix unsafe smp_processor_id() in thermal_throttle_add_dev
Fix BUG: using smp_processor_id() in preemptible thermal_throttle_add_dev.
We know the cpu number when calling thermal_throttle_add_dev, so we can
remove smp_processor_id call in thermal_throttle_add_dev by supplying
the cpu number as argument.
This should resolve kernel bugzilla 16615/16629.
Signed-off-by: Sergey Senozhatsky <[email protected]>
LKML-Reference: <[email protected]>
Cc: Fenghua Yu <[email protected]>
Cc: Joerg Roedel <[email protected]>
Cc: Maciej Rutecki <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
---
arch/x86/kernel/cpu/mcheck/therm_throt.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/mcheck/therm_throt.c b/arch/x86/kernel/cpu/mcheck/therm_throt.c
index c2a8b26..d9368ee 100644
--- a/arch/x86/kernel/cpu/mcheck/therm_throt.c
+++ b/arch/x86/kernel/cpu/mcheck/therm_throt.c
@@ -202,10 +202,11 @@ static int therm_throt_process(bool new_event, int event, int level)
#ifdef CONFIG_SYSFS
/* Add/Remove thermal_throttle interface for CPU device: */
-static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev)
+static __cpuinit int thermal_throttle_add_dev(struct sys_device *sys_dev,
+ unsigned int cpu)
{
int err;
- struct cpuinfo_x86 *c = &cpu_data(smp_processor_id());
+ struct cpuinfo_x86 *c = &cpu_data(cpu);
err = sysfs_create_group(&sys_dev->kobj, &thermal_attr_group);
if (err)
@@ -251,7 +252,7 @@ thermal_throttle_cpu_callback(struct notifier_block *nfb,
case CPU_UP_PREPARE:
case CPU_UP_PREPARE_FROZEN:
mutex_lock(&therm_cpu_lock);
- err = thermal_throttle_add_dev(sys_dev);
+ err = thermal_throttle_add_dev(sys_dev, cpu);
mutex_unlock(&therm_cpu_lock);
WARN_ON(err);
break;
@@ -287,7 +288,7 @@ static __init int thermal_throttle_init_device(void)
#endif
/* connect live CPUs to sysfs */
for_each_online_cpu(cpu) {
- err = thermal_throttle_add_dev(get_cpu_sysdev(cpu));
+ err = thermal_throttle_add_dev(get_cpu_sysdev(cpu), cpu);
WARN_ON(err);
}
#ifdef CONFIG_HOTPLUG_CPU
On Fri, Aug 20, 2010 at 09:43:03PM -0700, tip-bot for Sergey Senozhatsky wrote:
> Commit-ID: 51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
> Gitweb: http://git.kernel.org/tip/51e3c1b558b31b11bf5fc66d3c6f5adacf3573f7
> Author: Sergey Senozhatsky <[email protected]>
> AuthorDate: Fri, 20 Aug 2010 10:36:34 +0300
> Committer: H. Peter Anvin <[email protected]>
> CommitDate: Fri, 20 Aug 2010 19:56:00 -0700
>
> x86, hwmon: Fix unsafe smp_processor_id() in thermal_throttle_add_dev
>
> Fix BUG: using smp_processor_id() in preemptible thermal_throttle_add_dev.
> We know the cpu number when calling thermal_throttle_add_dev, so we can
> remove smp_processor_id call in thermal_throttle_add_dev by supplying
> the cpu number as argument.
>
> This should resolve kernel bugzilla 16615/16629.
The fix is right. Current thermal_throttle_add_dev checks local processor to
determine PTS and PLN features on another processor. Logically that is wrong.
Thanks.
-Fenghua