* [Patch] cpufreq: fix a deadlock during shutting down
@ 2010-02-09 9:10 Amerigo Wang
2010-02-09 9:22 ` David Rientjes
2010-02-09 9:23 ` Xiaotian Feng
0 siblings, 2 replies; 4+ messages in thread
From: Amerigo Wang @ 2010-02-09 9:10 UTC (permalink / raw)
To: linux-kernel
Cc: Xiaotian Feng, Benjamin Herrenschmidt, Dave Jones, cpufreq,
Amerigo Wang, Prarit Bhargava, Thomas Renninger, akpm,
Venkatesh Pallipadi
Benjamin reported that, the machine deadlocks right after printing the
following when doing a shutdown:
halt/4071 is trying to acquire lock:
(s_active){++++.+}, at: [<c0000000001ef868>] .sysfs_addrm_finish+0x58/0xc0
but task is already holding lock:
(&per_cpu(cpu_policy_rwsem, cpu)){+.+.+.}, at: [<c0000000004cd6ac>] .lock_policy_rwsem_write+0x84/0xf4
which lock already depends on the new lock.
the existing dependency chain (in reverse order) is:
<nothing else ... machine deadlocked here>
This is because we are trying to kobject_put() a kobject while
we are holding cpu policy rwsem. So just move kobject_put()
down after releasing the rwsem.
Totally untested.
Reported-by: Xiaotian Feng <xtfeng@gmail.com>
Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Dave Jones <davej@redhat.com>
Cc: Thomas Renninger <trenn@suse.de>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 67bc2ec..222b35f 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1113,6 +1113,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
unsigned int cpu = sys_dev->id;
unsigned long flags;
struct cpufreq_policy *data;
+ struct kobject *kobj;
#ifdef CONFIG_SMP
struct sys_device *cpu_sys_dev;
unsigned int j;
@@ -1192,7 +1193,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
if (cpufreq_driver->target)
__cpufreq_governor(data, CPUFREQ_GOV_STOP);
- kobject_put(&data->kobj);
+ kobj = &data->kobj;
/* we need to make sure that the underlying kobj is actually
* not referenced anymore by anybody before we proceed with
@@ -1207,6 +1208,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
unlock_policy_rwsem_write(cpu);
+ kobject_put(kobj);
free_cpumask_var(data->related_cpus);
free_cpumask_var(data->cpus);
kfree(data);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Patch] cpufreq: fix a deadlock during shutting down
2010-02-09 9:10 [Patch] cpufreq: fix a deadlock during shutting down Amerigo Wang
@ 2010-02-09 9:22 ` David Rientjes
2010-02-10 6:56 ` Cong Wang
2010-02-09 9:23 ` Xiaotian Feng
1 sibling, 1 reply; 4+ messages in thread
From: David Rientjes @ 2010-02-09 9:22 UTC (permalink / raw)
To: Amerigo Wang
Cc: linux-kernel, Xiaotian Feng, Benjamin Herrenschmidt, Dave Jones,
cpufreq, Prarit Bhargava, Thomas Renninger, Andrew Morton,
Venkatesh Pallipadi
On Tue, 9 Feb 2010, Amerigo Wang wrote:
> Benjamin reported that, the machine deadlocks right after printing the
> following when doing a shutdown:
>
> halt/4071 is trying to acquire lock:
> (s_active){++++.+}, at: [<c0000000001ef868>] .sysfs_addrm_finish+0x58/0xc0
>
> but task is already holding lock:
> (&per_cpu(cpu_policy_rwsem, cpu)){+.+.+.}, at: [<c0000000004cd6ac>] .lock_policy_rwsem_write+0x84/0xf4
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> <nothing else ... machine deadlocked here>
>
>
> This is because we are trying to kobject_put() a kobject while
> we are holding cpu policy rwsem. So just move kobject_put()
> down after releasing the rwsem.
>
> Totally untested.
>
> Reported-by: Xiaotian Feng <xtfeng@gmail.com>
> Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Dave Jones <davej@redhat.com>
> Cc: Thomas Renninger <trenn@suse.de>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>
> ---
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 67bc2ec..222b35f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1113,6 +1113,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
> unsigned int cpu = sys_dev->id;
> unsigned long flags;
> struct cpufreq_policy *data;
> + struct kobject *kobj;
> #ifdef CONFIG_SMP
> struct sys_device *cpu_sys_dev;
> unsigned int j;
> @@ -1192,7 +1193,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
> if (cpufreq_driver->target)
> __cpufreq_governor(data, CPUFREQ_GOV_STOP);
>
> - kobject_put(&data->kobj);
> + kobj = &data->kobj;
>
> /* we need to make sure that the underlying kobj is actually
> * not referenced anymore by anybody before we proceed with
NACK.
If you read this comment, it says:
/* we need to make sure that the underlying kobj is actually
* not referenced anymore by anybody before we proceed with
* unloading.
*/
That would suggest that the wait_for_completion(&data->kobj_unregister);
would never actually return if you're holding a reference to it in your
patch since it only completes when the last reference is dropped (the
->release function is cpufreq_sysfs_release()).
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] cpufreq: fix a deadlock during shutting down
2010-02-09 9:10 [Patch] cpufreq: fix a deadlock during shutting down Amerigo Wang
2010-02-09 9:22 ` David Rientjes
@ 2010-02-09 9:23 ` Xiaotian Feng
1 sibling, 0 replies; 4+ messages in thread
From: Xiaotian Feng @ 2010-02-09 9:23 UTC (permalink / raw)
To: Amerigo Wang
Cc: linux-kernel, Benjamin Herrenschmidt, Dave Jones, cpufreq,
Prarit Bhargava, Thomas Renninger, akpm, Venkatesh Pallipadi
On Tue, Feb 9, 2010 at 5:10 PM, Amerigo Wang <amwang@redhat.com> wrote:
>
> Benjamin reported that, the machine deadlocks right after printing the
> following when doing a shutdown:
>
> halt/4071 is trying to acquire lock:
> (s_active){++++.+}, at: [<c0000000001ef868>] .sysfs_addrm_finish+0x58/0xc0
>
> but task is already holding lock:
> (&per_cpu(cpu_policy_rwsem, cpu)){+.+.+.}, at: [<c0000000004cd6ac>] .lock_policy_rwsem_write+0x84/0xf4
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> <nothing else ... machine deadlocked here>
>
>
> This is because we are trying to kobject_put() a kobject while
> we are holding cpu policy rwsem. So just move kobject_put()
> down after releasing the rwsem.
>
> Totally untested.
>
> Reported-by: Xiaotian Feng <xtfeng@gmail.com>
> Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Dave Jones <davej@redhat.com>
> Cc: Thomas Renninger <trenn@suse.de>
> Cc: Prarit Bhargava <prarit@redhat.com>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>
> ---
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 67bc2ec..222b35f 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1113,6 +1113,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
> unsigned int cpu = sys_dev->id;
> unsigned long flags;
> struct cpufreq_policy *data;
> + struct kobject *kobj;
> #ifdef CONFIG_SMP
> struct sys_device *cpu_sys_dev;
> unsigned int j;
> @@ -1192,7 +1193,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
> if (cpufreq_driver->target)
> __cpufreq_governor(data, CPUFREQ_GOV_STOP);
>
> - kobject_put(&data->kobj);
> + kobj = &data->kobj;
>
> /* we need to make sure that the underlying kobj is actually
> * not referenced anymore by anybody before we proceed with
Then kernel will wait_for_completion(&data->kobj_unregister); forever.....
> @@ -1207,6 +1208,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
>
> unlock_policy_rwsem_write(cpu);
>
> + kobject_put(kobj);
> free_cpumask_var(data->related_cpus);
> free_cpumask_var(data->cpus);
> kfree(data);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Patch] cpufreq: fix a deadlock during shutting down
2010-02-09 9:22 ` David Rientjes
@ 2010-02-10 6:56 ` Cong Wang
0 siblings, 0 replies; 4+ messages in thread
From: Cong Wang @ 2010-02-10 6:56 UTC (permalink / raw)
To: David Rientjes
Cc: linux-kernel, Xiaotian Feng, Benjamin Herrenschmidt, Dave Jones,
cpufreq, Prarit Bhargava, Thomas Renninger, Andrew Morton,
Venkatesh Pallipadi
David Rientjes wrote:
> On Tue, 9 Feb 2010, Amerigo Wang wrote:
>
>> Benjamin reported that, the machine deadlocks right after printing the
>> following when doing a shutdown:
>>
>> halt/4071 is trying to acquire lock:
>> (s_active){++++.+}, at: [<c0000000001ef868>] .sysfs_addrm_finish+0x58/0xc0
>>
>> but task is already holding lock:
>> (&per_cpu(cpu_policy_rwsem, cpu)){+.+.+.}, at: [<c0000000004cd6ac>] .lock_policy_rwsem_write+0x84/0xf4
>>
>> which lock already depends on the new lock.
>>
>> the existing dependency chain (in reverse order) is:
>>
>> <nothing else ... machine deadlocked here>
>>
>>
>> This is because we are trying to kobject_put() a kobject while
>> we are holding cpu policy rwsem. So just move kobject_put()
>> down after releasing the rwsem.
>>
>> Totally untested.
>>
>> Reported-by: Xiaotian Feng <xtfeng@gmail.com>
>> Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Signed-off-by: WANG Cong <amwang@redhat.com>
>> Cc: Dave Jones <davej@redhat.com>
>> Cc: Thomas Renninger <trenn@suse.de>
>> Cc: Prarit Bhargava <prarit@redhat.com>
>> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>
>> ---
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 67bc2ec..222b35f 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1113,6 +1113,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
>> unsigned int cpu = sys_dev->id;
>> unsigned long flags;
>> struct cpufreq_policy *data;
>> + struct kobject *kobj;
>> #ifdef CONFIG_SMP
>> struct sys_device *cpu_sys_dev;
>> unsigned int j;
>> @@ -1192,7 +1193,7 @@ static int __cpufreq_remove_dev(struct sys_device *sys_dev)
>> if (cpufreq_driver->target)
>> __cpufreq_governor(data, CPUFREQ_GOV_STOP);
>>
>> - kobject_put(&data->kobj);
>> + kobj = &data->kobj;
>>
>> /* we need to make sure that the underlying kobj is actually
>> * not referenced anymore by anybody before we proceed with
>
> NACK.
>
> If you read this comment, it says:
>
> /* we need to make sure that the underlying kobj is actually
> * not referenced anymore by anybody before we proceed with
> * unloading.
> */
>
> That would suggest that the wait_for_completion(&data->kobj_unregister);
> would never actually return if you're holding a reference to it in your
> patch since it only completes when the last reference is dropped (the
> ->release function is cpufreq_sysfs_release()).
Oh, my bad.
Then this case seems to be more complex... But anyway, this is _not_ a
bogus.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2010-02-10 6:54 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-09 9:10 [Patch] cpufreq: fix a deadlock during shutting down Amerigo Wang
2010-02-09 9:22 ` David Rientjes
2010-02-10 6:56 ` Cong Wang
2010-02-09 9:23 ` Xiaotian Feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox