* [PATCH] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch
@ 2012-07-20 1:57 Stephen Boyd
2012-07-20 10:05 ` Rafael J. Wysocki
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2012-07-20 1:57 UTC (permalink / raw)
To: Rafael J . Wysocki; +Cc: linux-kernel, cpufreq, linux-pm
Running one program that continuously hotplugs and replugs a cpu
concurrently with another program that continuously writes to the
scaling_setspeed node eventually deadlocks with:
=============================================
[ INFO: possible recursive locking detected ]
3.4.0 #37 Tainted: G W
---------------------------------------------
filemonkey/122 is trying to acquire lock:
(s_active#13){++++.+}, at: [<c01a3d28>] sysfs_remove_dir+0x9c/0xb4
but task is already holding lock:
(s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(s_active#13);
lock(s_active#13);
*** DEADLOCK ***
May be due to missing lock nesting notation
2 locks held by filemonkey/122:
#0: (&buffer->mutex){+.+.+.}, at: [<c01a2230>] sysfs_write_file+0x28/0x140
#1: (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
stack backtrace:
[<c0014fcc>] (unwind_backtrace+0x0/0x120) from [<c00ca600>] (validate_chain+0x6f8/0x1054)
[<c00ca600>] (validate_chain+0x6f8/0x1054) from [<c00cb778>] (__lock_acquire+0x81c/0x8d8)
[<c00cb778>] (__lock_acquire+0x81c/0x8d8) from [<c00cb9c0>] (lock_acquire+0x18c/0x1e8)
[<c00cb9c0>] (lock_acquire+0x18c/0x1e8) from [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180)
[<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180) from [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4)
[<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4) from [<c02d0e5c>] (kobject_del+0x10/0x38)
[<c02d0e5c>] (kobject_del+0x10/0x38) from [<c02d0f74>] (kobject_release+0xf0/0x194)
[<c02d0f74>] (kobject_release+0xf0/0x194) from [<c0565a98>] (cpufreq_cpu_put+0xc/0x24)
[<c0565a98>] (cpufreq_cpu_put+0xc/0x24) from [<c05683f0>] (store+0x6c/0x74)
[<c05683f0>] (store+0x6c/0x74) from [<c01a2314>] (sysfs_write_file+0x10c/0x140)
[<c01a2314>] (sysfs_write_file+0x10c/0x140) from [<c014af44>] (vfs_write+0xb0/0x128)
[<c014af44>] (vfs_write+0xb0/0x128) from [<c014b06c>] (sys_write+0x3c/0x68)
[<c014b06c>] (sys_write+0x3c/0x68) from [<c000e0e0>] (ret_fast_syscall+0x0/0x3c)
This is because store() in cpufreq.c indirectly calls
kobject_get() via cpufreq_cpu_get() and is the last one to call
kobject_put() via cpufreq_cpu_put(). Sysfs code should not call
kobject_get() or kobject_put() directly (see the comment around
sysfs_schedule_callback() for more information).
Fix this deadlock by introducing two new functions:
struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
which do the same thing as cpufreq_cpu_{get,put}() but don't call
kobject functions.
To easily trigger this deadlock you can apply a one line patch to
the store() function in cpufreq.c
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index a290771..62af12d 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -675,6 +675,7 @@ static ssize_t store(struct kobject *kobj
unlock_policy_rwsem_write(policy->cpu);
fail:
+ msleep(10000);
cpufreq_cpu_put_sysfs(policy);
no_policy:
return ret;
and then write scaling_setspeed in one task and offline the cpu
in another. The first task will hang and be detected by the hung
task detector.
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
Before you ask, I've seen the comment above cpufreq_add_dev() about
concurrent hotplug/cpufreq.
drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7f2f149..a290771 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -138,7 +138,7 @@ void disable_cpufreq(void)
static LIST_HEAD(cpufreq_governor_list);
static DEFINE_MUTEX(cpufreq_governor_mutex);
-struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
+static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, int sysfs)
{
struct cpufreq_policy *data;
unsigned long flags;
@@ -162,7 +162,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
if (!data)
goto err_out_put_module;
- if (!kobject_get(&data->kobj))
+ if (!sysfs && !kobject_get(&data->kobj))
goto err_out_put_module;
spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -175,16 +175,35 @@ err_out_unlock:
err_out:
return NULL;
}
+
+struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
+{
+ return __cpufreq_cpu_get(cpu, 0);
+}
EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
+static struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
+{
+ return __cpufreq_cpu_get(cpu, 1);
+}
-void cpufreq_cpu_put(struct cpufreq_policy *data)
+static void __cpufreq_cpu_put(struct cpufreq_policy *data, int sysfs)
{
- kobject_put(&data->kobj);
+ if (!sysfs)
+ kobject_put(&data->kobj);
module_put(cpufreq_driver->owner);
}
+
+void cpufreq_cpu_put(struct cpufreq_policy *data)
+{
+ __cpufreq_cpu_put(data, 0);
+}
EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
+static void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
+{
+ __cpufreq_cpu_put(data, 1);
+}
/*********************************************************************
* EXTERNALLY AFFECTING FREQUENCY CHANGES *
@@ -617,7 +636,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
struct cpufreq_policy *policy = to_policy(kobj);
struct freq_attr *fattr = to_attr(attr);
ssize_t ret = -EINVAL;
- policy = cpufreq_cpu_get(policy->cpu);
+ policy = cpufreq_cpu_get_sysfs(policy->cpu);
if (!policy)
goto no_policy;
@@ -631,7 +650,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
unlock_policy_rwsem_read(policy->cpu);
fail:
- cpufreq_cpu_put(policy);
+ cpufreq_cpu_put_sysfs(policy);
no_policy:
return ret;
}
@@ -642,7 +661,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
struct cpufreq_policy *policy = to_policy(kobj);
struct freq_attr *fattr = to_attr(attr);
ssize_t ret = -EINVAL;
- policy = cpufreq_cpu_get(policy->cpu);
+ policy = cpufreq_cpu_get_sysfs(policy->cpu);
if (!policy)
goto no_policy;
@@ -656,7 +675,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
unlock_policy_rwsem_write(policy->cpu);
fail:
- cpufreq_cpu_put(policy);
+ cpufreq_cpu_put_sysfs(policy);
no_policy:
return ret;
}
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch
2012-07-20 1:57 [PATCH] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch Stephen Boyd
@ 2012-07-20 10:05 ` Rafael J. Wysocki
2012-07-20 17:34 ` Stephen Boyd
2012-07-20 18:14 ` [PATCHv2] " Stephen Boyd
0 siblings, 2 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2012-07-20 10:05 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-kernel, cpufreq, linux-pm
On Friday, July 20, 2012, Stephen Boyd wrote:
> Running one program that continuously hotplugs and replugs a cpu
> concurrently with another program that continuously writes to the
> scaling_setspeed node eventually deadlocks with:
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.4.0 #37 Tainted: G W
> ---------------------------------------------
> filemonkey/122 is trying to acquire lock:
> (s_active#13){++++.+}, at: [<c01a3d28>] sysfs_remove_dir+0x9c/0xb4
>
> but task is already holding lock:
> (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(s_active#13);
> lock(s_active#13);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 2 locks held by filemonkey/122:
> #0: (&buffer->mutex){+.+.+.}, at: [<c01a2230>] sysfs_write_file+0x28/0x140
> #1: (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
>
> stack backtrace:
> [<c0014fcc>] (unwind_backtrace+0x0/0x120) from [<c00ca600>] (validate_chain+0x6f8/0x1054)
> [<c00ca600>] (validate_chain+0x6f8/0x1054) from [<c00cb778>] (__lock_acquire+0x81c/0x8d8)
> [<c00cb778>] (__lock_acquire+0x81c/0x8d8) from [<c00cb9c0>] (lock_acquire+0x18c/0x1e8)
> [<c00cb9c0>] (lock_acquire+0x18c/0x1e8) from [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180)
> [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180) from [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4)
> [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4) from [<c02d0e5c>] (kobject_del+0x10/0x38)
> [<c02d0e5c>] (kobject_del+0x10/0x38) from [<c02d0f74>] (kobject_release+0xf0/0x194)
> [<c02d0f74>] (kobject_release+0xf0/0x194) from [<c0565a98>] (cpufreq_cpu_put+0xc/0x24)
> [<c0565a98>] (cpufreq_cpu_put+0xc/0x24) from [<c05683f0>] (store+0x6c/0x74)
> [<c05683f0>] (store+0x6c/0x74) from [<c01a2314>] (sysfs_write_file+0x10c/0x140)
> [<c01a2314>] (sysfs_write_file+0x10c/0x140) from [<c014af44>] (vfs_write+0xb0/0x128)
> [<c014af44>] (vfs_write+0xb0/0x128) from [<c014b06c>] (sys_write+0x3c/0x68)
> [<c014b06c>] (sys_write+0x3c/0x68) from [<c000e0e0>] (ret_fast_syscall+0x0/0x3c)
>
> This is because store() in cpufreq.c indirectly calls
> kobject_get() via cpufreq_cpu_get() and is the last one to call
> kobject_put() via cpufreq_cpu_put(). Sysfs code should not call
> kobject_get() or kobject_put() directly (see the comment around
> sysfs_schedule_callback() for more information).
>
> Fix this deadlock by introducing two new functions:
>
> struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
> void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
>
> which do the same thing as cpufreq_cpu_{get,put}() but don't call
> kobject functions.
>
> To easily trigger this deadlock you can apply a one line patch to
> the store() function in cpufreq.c
The following part of your changelog has confused Patchwork. I guess it
will also confuse other tools, so care to describe what to do instead?
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index a290771..62af12d 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -675,6 +675,7 @@ static ssize_t store(struct kobject *kobj
>
> unlock_policy_rwsem_write(policy->cpu);
> fail:
> + msleep(10000);
> cpufreq_cpu_put_sysfs(policy);
> no_policy:
> return ret;
>
> and then write scaling_setspeed in one task and offline the cpu
> in another. The first task will hang and be detected by the hung
> task detector.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
>
> Before you ask, I've seen the comment above cpufreq_add_dev() about
> concurrent hotplug/cpufreq.
>
> drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7f2f149..a290771 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -138,7 +138,7 @@ void disable_cpufreq(void)
> static LIST_HEAD(cpufreq_governor_list);
> static DEFINE_MUTEX(cpufreq_governor_mutex);
>
> -struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> +static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, int sysfs)
I'd prefer the sysfs arg to be a bool.
> {
> struct cpufreq_policy *data;
> unsigned long flags;
> @@ -162,7 +162,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> if (!data)
> goto err_out_put_module;
>
> - if (!kobject_get(&data->kobj))
> + if (!sysfs && !kobject_get(&data->kobj))
> goto err_out_put_module;
>
> spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> @@ -175,16 +175,35 @@ err_out_unlock:
> err_out:
> return NULL;
> }
> +
> +struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> +{
> + return __cpufreq_cpu_get(cpu, 0);
> +}
> EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
>
> +static struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
> +{
> + return __cpufreq_cpu_get(cpu, 1);
> +}
>
> -void cpufreq_cpu_put(struct cpufreq_policy *data)
> +static void __cpufreq_cpu_put(struct cpufreq_policy *data, int sysfs)
> {
> - kobject_put(&data->kobj);
> + if (!sysfs)
> + kobject_put(&data->kobj);
> module_put(cpufreq_driver->owner);
> }
> +
> +void cpufreq_cpu_put(struct cpufreq_policy *data)
> +{
> + __cpufreq_cpu_put(data, 0);
> +}
> EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
>
> +static void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
> +{
> + __cpufreq_cpu_put(data, 1);
> +}
>
> /*********************************************************************
> * EXTERNALLY AFFECTING FREQUENCY CHANGES *
> @@ -617,7 +636,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
> struct cpufreq_policy *policy = to_policy(kobj);
> struct freq_attr *fattr = to_attr(attr);
> ssize_t ret = -EINVAL;
> - policy = cpufreq_cpu_get(policy->cpu);
> + policy = cpufreq_cpu_get_sysfs(policy->cpu);
> if (!policy)
> goto no_policy;
>
> @@ -631,7 +650,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>
> unlock_policy_rwsem_read(policy->cpu);
> fail:
> - cpufreq_cpu_put(policy);
> + cpufreq_cpu_put_sysfs(policy);
> no_policy:
> return ret;
> }
> @@ -642,7 +661,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
> struct cpufreq_policy *policy = to_policy(kobj);
> struct freq_attr *fattr = to_attr(attr);
> ssize_t ret = -EINVAL;
> - policy = cpufreq_cpu_get(policy->cpu);
> + policy = cpufreq_cpu_get_sysfs(policy->cpu);
> if (!policy)
> goto no_policy;
>
> @@ -656,7 +675,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>
> unlock_policy_rwsem_write(policy->cpu);
> fail:
> - cpufreq_cpu_put(policy);
> + cpufreq_cpu_put_sysfs(policy);
> no_policy:
> return ret;
> }
>
Thanks,
Rafael
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch
2012-07-20 10:05 ` Rafael J. Wysocki
@ 2012-07-20 17:34 ` Stephen Boyd
2012-07-20 18:14 ` [PATCHv2] " Stephen Boyd
1 sibling, 0 replies; 6+ messages in thread
From: Stephen Boyd @ 2012-07-20 17:34 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: linux-kernel, cpufreq, linux-pm
On 07/20/12 03:05, Rafael J. Wysocki wrote:
>
> The following part of your changelog has confused Patchwork. I guess it
> will also confuse other tools, so care to describe what to do instead?
Sure. I thought that might happen but I put a space in front in hopes it
wouldn't cause troubles.
>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index a290771..62af12d 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -675,6 +675,7 @@ static ssize_t store(struct kobject *kobj
>>
>> unlock_policy_rwsem_write(policy->cpu);
>> fail:
>> + msleep(10000);
>> cpufreq_cpu_put_sysfs(policy);
>> no_policy:
>> return ret;
>>
>> and then write scaling_setspeed in one task and offline the cpu
>> in another. The first task will hang and be detected by the hung
>> task detector.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>> ---
>>
>> Before you ask, I've seen the comment above cpufreq_add_dev() about
>> concurrent hotplug/cpufreq.
>>
>> drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++++--------
>> 1 file changed, 27 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 7f2f149..a290771 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -138,7 +138,7 @@ void disable_cpufreq(void)
>> static LIST_HEAD(cpufreq_governor_list);
>> static DEFINE_MUTEX(cpufreq_governor_mutex);
>>
>> -struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>> +static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, int sysfs)
> I'd prefer the sysfs arg to be a bool.
Sure. V2 coming right up.
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCHv2] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch
2012-07-20 10:05 ` Rafael J. Wysocki
2012-07-20 17:34 ` Stephen Boyd
@ 2012-07-20 18:14 ` Stephen Boyd
2012-07-20 19:51 ` Rafael J. Wysocki
1 sibling, 1 reply; 6+ messages in thread
From: Stephen Boyd @ 2012-07-20 18:14 UTC (permalink / raw)
To: Rafael J . Wysocki; +Cc: linux-kernel, cpufreq, linux-pm
Running one program that continuously hotplugs and replugs a cpu
concurrently with another program that continuously writes to the
scaling_setspeed node eventually deadlocks with:
=============================================
[ INFO: possible recursive locking detected ]
3.4.0 #37 Tainted: G W
---------------------------------------------
filemonkey/122 is trying to acquire lock:
(s_active#13){++++.+}, at: [<c01a3d28>] sysfs_remove_dir+0x9c/0xb4
but task is already holding lock:
(s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(s_active#13);
lock(s_active#13);
*** DEADLOCK ***
May be due to missing lock nesting notation
2 locks held by filemonkey/122:
#0: (&buffer->mutex){+.+.+.}, at: [<c01a2230>] sysfs_write_file+0x28/0x140
#1: (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
stack backtrace:
[<c0014fcc>] (unwind_backtrace+0x0/0x120) from [<c00ca600>] (validate_chain+0x6f8/0x1054)
[<c00ca600>] (validate_chain+0x6f8/0x1054) from [<c00cb778>] (__lock_acquire+0x81c/0x8d8)
[<c00cb778>] (__lock_acquire+0x81c/0x8d8) from [<c00cb9c0>] (lock_acquire+0x18c/0x1e8)
[<c00cb9c0>] (lock_acquire+0x18c/0x1e8) from [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180)
[<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180) from [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4)
[<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4) from [<c02d0e5c>] (kobject_del+0x10/0x38)
[<c02d0e5c>] (kobject_del+0x10/0x38) from [<c02d0f74>] (kobject_release+0xf0/0x194)
[<c02d0f74>] (kobject_release+0xf0/0x194) from [<c0565a98>] (cpufreq_cpu_put+0xc/0x24)
[<c0565a98>] (cpufreq_cpu_put+0xc/0x24) from [<c05683f0>] (store+0x6c/0x74)
[<c05683f0>] (store+0x6c/0x74) from [<c01a2314>] (sysfs_write_file+0x10c/0x140)
[<c01a2314>] (sysfs_write_file+0x10c/0x140) from [<c014af44>] (vfs_write+0xb0/0x128)
[<c014af44>] (vfs_write+0xb0/0x128) from [<c014b06c>] (sys_write+0x3c/0x68)
[<c014b06c>] (sys_write+0x3c/0x68) from [<c000e0e0>] (ret_fast_syscall+0x0/0x3c)
This is because store() in cpufreq.c indirectly calls
kobject_get() via cpufreq_cpu_get() and is the last one to call
kobject_put() via cpufreq_cpu_put(). Sysfs code should not call
kobject_get() or kobject_put() directly (see the comment around
sysfs_schedule_callback() for more information).
Fix this deadlock by introducing two new functions:
struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
which do the same thing as cpufreq_cpu_{get,put}() but don't call
kobject functions.
To easily trigger this deadlock you can insert an msleep() with a
reasonably large value right after the fail label at the bottom
of the store() function in cpufreq.c and then write
scaling_setspeed in one task and offline the cpu in another. The
first task will hang and be detected by the hung task detector.
Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
---
Changes from v1:
- switch sysfs arg to bool
- remove patch from commit text
drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 7f2f149..fb8a527 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -138,7 +138,7 @@ void disable_cpufreq(void)
static LIST_HEAD(cpufreq_governor_list);
static DEFINE_MUTEX(cpufreq_governor_mutex);
-struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
+static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs)
{
struct cpufreq_policy *data;
unsigned long flags;
@@ -162,7 +162,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
if (!data)
goto err_out_put_module;
- if (!kobject_get(&data->kobj))
+ if (!sysfs && !kobject_get(&data->kobj))
goto err_out_put_module;
spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -175,16 +175,35 @@ err_out_unlock:
err_out:
return NULL;
}
+
+struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
+{
+ return __cpufreq_cpu_get(cpu, false);
+}
EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
+static struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
+{
+ return __cpufreq_cpu_get(cpu, true);
+}
-void cpufreq_cpu_put(struct cpufreq_policy *data)
+static void __cpufreq_cpu_put(struct cpufreq_policy *data, bool sysfs)
{
- kobject_put(&data->kobj);
+ if (!sysfs)
+ kobject_put(&data->kobj);
module_put(cpufreq_driver->owner);
}
+
+void cpufreq_cpu_put(struct cpufreq_policy *data)
+{
+ __cpufreq_cpu_put(data, false);
+}
EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
+static void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
+{
+ __cpufreq_cpu_put(data, true);
+}
/*********************************************************************
* EXTERNALLY AFFECTING FREQUENCY CHANGES *
@@ -617,7 +636,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
struct cpufreq_policy *policy = to_policy(kobj);
struct freq_attr *fattr = to_attr(attr);
ssize_t ret = -EINVAL;
- policy = cpufreq_cpu_get(policy->cpu);
+ policy = cpufreq_cpu_get_sysfs(policy->cpu);
if (!policy)
goto no_policy;
@@ -631,7 +650,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
unlock_policy_rwsem_read(policy->cpu);
fail:
- cpufreq_cpu_put(policy);
+ cpufreq_cpu_put_sysfs(policy);
no_policy:
return ret;
}
@@ -642,7 +661,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
struct cpufreq_policy *policy = to_policy(kobj);
struct freq_attr *fattr = to_attr(attr);
ssize_t ret = -EINVAL;
- policy = cpufreq_cpu_get(policy->cpu);
+ policy = cpufreq_cpu_get_sysfs(policy->cpu);
if (!policy)
goto no_policy;
@@ -656,7 +675,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
unlock_policy_rwsem_write(policy->cpu);
fail:
- cpufreq_cpu_put(policy);
+ cpufreq_cpu_put_sysfs(policy);
no_policy:
return ret;
}
--
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCHv2] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch
2012-07-20 18:14 ` [PATCHv2] " Stephen Boyd
@ 2012-07-20 19:51 ` Rafael J. Wysocki
2012-07-21 16:57 ` Shilimkar, Santosh
0 siblings, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2012-07-20 19:51 UTC (permalink / raw)
To: Stephen Boyd; +Cc: linux-kernel, cpufreq, linux-pm
On Friday, July 20, 2012, Stephen Boyd wrote:
> Running one program that continuously hotplugs and replugs a cpu
> concurrently with another program that continuously writes to the
> scaling_setspeed node eventually deadlocks with:
>
> =============================================
> [ INFO: possible recursive locking detected ]
> 3.4.0 #37 Tainted: G W
> ---------------------------------------------
> filemonkey/122 is trying to acquire lock:
> (s_active#13){++++.+}, at: [<c01a3d28>] sysfs_remove_dir+0x9c/0xb4
>
> but task is already holding lock:
> (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(s_active#13);
> lock(s_active#13);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 2 locks held by filemonkey/122:
> #0: (&buffer->mutex){+.+.+.}, at: [<c01a2230>] sysfs_write_file+0x28/0x140
> #1: (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
>
> stack backtrace:
> [<c0014fcc>] (unwind_backtrace+0x0/0x120) from [<c00ca600>] (validate_chain+0x6f8/0x1054)
> [<c00ca600>] (validate_chain+0x6f8/0x1054) from [<c00cb778>] (__lock_acquire+0x81c/0x8d8)
> [<c00cb778>] (__lock_acquire+0x81c/0x8d8) from [<c00cb9c0>] (lock_acquire+0x18c/0x1e8)
> [<c00cb9c0>] (lock_acquire+0x18c/0x1e8) from [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180)
> [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180) from [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4)
> [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4) from [<c02d0e5c>] (kobject_del+0x10/0x38)
> [<c02d0e5c>] (kobject_del+0x10/0x38) from [<c02d0f74>] (kobject_release+0xf0/0x194)
> [<c02d0f74>] (kobject_release+0xf0/0x194) from [<c0565a98>] (cpufreq_cpu_put+0xc/0x24)
> [<c0565a98>] (cpufreq_cpu_put+0xc/0x24) from [<c05683f0>] (store+0x6c/0x74)
> [<c05683f0>] (store+0x6c/0x74) from [<c01a2314>] (sysfs_write_file+0x10c/0x140)
> [<c01a2314>] (sysfs_write_file+0x10c/0x140) from [<c014af44>] (vfs_write+0xb0/0x128)
> [<c014af44>] (vfs_write+0xb0/0x128) from [<c014b06c>] (sys_write+0x3c/0x68)
> [<c014b06c>] (sys_write+0x3c/0x68) from [<c000e0e0>] (ret_fast_syscall+0x0/0x3c)
>
> This is because store() in cpufreq.c indirectly calls
> kobject_get() via cpufreq_cpu_get() and is the last one to call
> kobject_put() via cpufreq_cpu_put(). Sysfs code should not call
> kobject_get() or kobject_put() directly (see the comment around
> sysfs_schedule_callback() for more information).
>
> Fix this deadlock by introducing two new functions:
>
> struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
> void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
>
> which do the same thing as cpufreq_cpu_{get,put}() but don't call
> kobject functions.
>
> To easily trigger this deadlock you can insert an msleep() with a
> reasonably large value right after the fail label at the bottom
> of the store() function in cpufreq.c and then write
> scaling_setspeed in one task and offline the cpu in another. The
> first task will hang and be detected by the hung task detector.
>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
Thanks, applied to the pm-cpufreq branch of the linux-pm.git tree, will be
pushed for v3.6.
Thanks,
Rafael
> ---
>
> Changes from v1:
> - switch sysfs arg to bool
> - remove patch from commit text
>
> drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 7f2f149..fb8a527 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -138,7 +138,7 @@ void disable_cpufreq(void)
> static LIST_HEAD(cpufreq_governor_list);
> static DEFINE_MUTEX(cpufreq_governor_mutex);
>
> -struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> +static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs)
> {
> struct cpufreq_policy *data;
> unsigned long flags;
> @@ -162,7 +162,7 @@ struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> if (!data)
> goto err_out_put_module;
>
> - if (!kobject_get(&data->kobj))
> + if (!sysfs && !kobject_get(&data->kobj))
> goto err_out_put_module;
>
> spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> @@ -175,16 +175,35 @@ err_out_unlock:
> err_out:
> return NULL;
> }
> +
> +struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
> +{
> + return __cpufreq_cpu_get(cpu, false);
> +}
> EXPORT_SYMBOL_GPL(cpufreq_cpu_get);
>
> +static struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
> +{
> + return __cpufreq_cpu_get(cpu, true);
> +}
>
> -void cpufreq_cpu_put(struct cpufreq_policy *data)
> +static void __cpufreq_cpu_put(struct cpufreq_policy *data, bool sysfs)
> {
> - kobject_put(&data->kobj);
> + if (!sysfs)
> + kobject_put(&data->kobj);
> module_put(cpufreq_driver->owner);
> }
> +
> +void cpufreq_cpu_put(struct cpufreq_policy *data)
> +{
> + __cpufreq_cpu_put(data, false);
> +}
> EXPORT_SYMBOL_GPL(cpufreq_cpu_put);
>
> +static void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
> +{
> + __cpufreq_cpu_put(data, true);
> +}
>
> /*********************************************************************
> * EXTERNALLY AFFECTING FREQUENCY CHANGES *
> @@ -617,7 +636,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
> struct cpufreq_policy *policy = to_policy(kobj);
> struct freq_attr *fattr = to_attr(attr);
> ssize_t ret = -EINVAL;
> - policy = cpufreq_cpu_get(policy->cpu);
> + policy = cpufreq_cpu_get_sysfs(policy->cpu);
> if (!policy)
> goto no_policy;
>
> @@ -631,7 +650,7 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
>
> unlock_policy_rwsem_read(policy->cpu);
> fail:
> - cpufreq_cpu_put(policy);
> + cpufreq_cpu_put_sysfs(policy);
> no_policy:
> return ret;
> }
> @@ -642,7 +661,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
> struct cpufreq_policy *policy = to_policy(kobj);
> struct freq_attr *fattr = to_attr(attr);
> ssize_t ret = -EINVAL;
> - policy = cpufreq_cpu_get(policy->cpu);
> + policy = cpufreq_cpu_get_sysfs(policy->cpu);
> if (!policy)
> goto no_policy;
>
> @@ -656,7 +675,7 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
>
> unlock_policy_rwsem_write(policy->cpu);
> fail:
> - cpufreq_cpu_put(policy);
> + cpufreq_cpu_put_sysfs(policy);
> no_policy:
> return ret;
> }
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCHv2] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch
2012-07-20 19:51 ` Rafael J. Wysocki
@ 2012-07-21 16:57 ` Shilimkar, Santosh
0 siblings, 0 replies; 6+ messages in thread
From: Shilimkar, Santosh @ 2012-07-21 16:57 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Stephen Boyd, linux-kernel, cpufreq, linux-pm
On Sat, Jul 21, 2012 at 1:21 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Friday, July 20, 2012, Stephen Boyd wrote:
>> Running one program that continuously hotplugs and replugs a cpu
>> concurrently with another program that continuously writes to the
>> scaling_setspeed node eventually deadlocks with:
>>
>> =============================================
>> [ INFO: possible recursive locking detected ]
>> 3.4.0 #37 Tainted: G W
>> ---------------------------------------------
>> filemonkey/122 is trying to acquire lock:
>> (s_active#13){++++.+}, at: [<c01a3d28>] sysfs_remove_dir+0x9c/0xb4
>>
>> but task is already holding lock:
>> (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
>>
>> other info that might help us debug this:
>> Possible unsafe locking scenario:
>>
>> CPU0
>> ----
>> lock(s_active#13);
>> lock(s_active#13);
>>
>> *** DEADLOCK ***
>>
>> May be due to missing lock nesting notation
>>
>> 2 locks held by filemonkey/122:
>> #0: (&buffer->mutex){+.+.+.}, at: [<c01a2230>] sysfs_write_file+0x28/0x140
>> #1: (s_active#13){++++.+}, at: [<c01a22f0>] sysfs_write_file+0xe8/0x140
>>
>> stack backtrace:
>> [<c0014fcc>] (unwind_backtrace+0x0/0x120) from [<c00ca600>] (validate_chain+0x6f8/0x1054)
>> [<c00ca600>] (validate_chain+0x6f8/0x1054) from [<c00cb778>] (__lock_acquire+0x81c/0x8d8)
>> [<c00cb778>] (__lock_acquire+0x81c/0x8d8) from [<c00cb9c0>] (lock_acquire+0x18c/0x1e8)
>> [<c00cb9c0>] (lock_acquire+0x18c/0x1e8) from [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180)
>> [<c01a3ba8>] (sysfs_addrm_finish+0xd0/0x180) from [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4)
>> [<c01a3d28>] (sysfs_remove_dir+0x9c/0xb4) from [<c02d0e5c>] (kobject_del+0x10/0x38)
>> [<c02d0e5c>] (kobject_del+0x10/0x38) from [<c02d0f74>] (kobject_release+0xf0/0x194)
>> [<c02d0f74>] (kobject_release+0xf0/0x194) from [<c0565a98>] (cpufreq_cpu_put+0xc/0x24)
>> [<c0565a98>] (cpufreq_cpu_put+0xc/0x24) from [<c05683f0>] (store+0x6c/0x74)
>> [<c05683f0>] (store+0x6c/0x74) from [<c01a2314>] (sysfs_write_file+0x10c/0x140)
>> [<c01a2314>] (sysfs_write_file+0x10c/0x140) from [<c014af44>] (vfs_write+0xb0/0x128)
>> [<c014af44>] (vfs_write+0xb0/0x128) from [<c014b06c>] (sys_write+0x3c/0x68)
>> [<c014b06c>] (sys_write+0x3c/0x68) from [<c000e0e0>] (ret_fast_syscall+0x0/0x3c)
>>
>> This is because store() in cpufreq.c indirectly calls
>> kobject_get() via cpufreq_cpu_get() and is the last one to call
>> kobject_put() via cpufreq_cpu_put(). Sysfs code should not call
>> kobject_get() or kobject_put() directly (see the comment around
>> sysfs_schedule_callback() for more information).
>>
>> Fix this deadlock by introducing two new functions:
>>
>> struct cpufreq_policy *cpufreq_cpu_get_sysfs(unsigned int cpu)
>> void cpufreq_cpu_put_sysfs(struct cpufreq_policy *data)
>>
>> which do the same thing as cpufreq_cpu_{get,put}() but don't call
>> kobject functions.
>>
>> To easily trigger this deadlock you can insert an msleep() with a
>> reasonably large value right after the fail label at the bottom
>> of the store() function in cpufreq.c and then write
>> scaling_setspeed in one task and offline the cpu in another. The
>> first task will hang and be detected by the hung task detector.
>>
>> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
>
> Thanks, applied to the pm-cpufreq branch of the linux-pm.git tree, will be
> pushed for v3.6.
>
Should this fix go to stable as well ?
Regards
Santosh
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-07-21 16:58 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-20 1:57 [PATCH] cpufreq: Fix sysfs deadlock with concurrent hotplug/frequency switch Stephen Boyd
2012-07-20 10:05 ` Rafael J. Wysocki
2012-07-20 17:34 ` Stephen Boyd
2012-07-20 18:14 ` [PATCHv2] " Stephen Boyd
2012-07-20 19:51 ` Rafael J. Wysocki
2012-07-21 16:57 ` Shilimkar, Santosh
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).