lkml.org 
[lkml]   [2009]   [Jul]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess
From
Date
On Fri, 2009-07-03 at 12:41 -0700, Mathieu Desnoyers wrote:
> * Pallipadi, Venkatesh (venkatesh.pallipadi@intel.com) wrote:
> >
> >
> > >-----Original Message-----
> > >From: Mathieu Desnoyers [mailto:mathieu.desnoyers@polymtl.ca]
> > >Sent: Friday, July 03, 2009 8:25 AM
> > >To: linux-kernel@vger.kernel.org; Pallipadi, Venkatesh; Dave
> > >Jones; Thomas Renninger; cpufreq@vger.kernel.org;
> > >kernel-testers@vger.kernel.org; Ingo Molnar; rjw@sisk.pl; Dave
> > >Young; Pekka Enberg
> > >Cc: Mathieu Desnoyers; Li, Shaohua; Rusty Russell;
> > >sven.wegener@stealer.net
> > >Subject: [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess
> > >
> > >OK, I've tried to clean it up the best I could, but please
> > >test this with
> > >concurrent cpu hotplug and cpufreq add/remove in loops. I'm
> > >sure we will make
> > >other interesting findings.
> > >
> >
> > This is a good and needed cleanup of cpufreq_add_dev.
> >
> >
> > >This is step one of fixing the overall locking dependency mess
> > >in cpufreq.
> > >
> > >Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>
> > >CC: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> > >CC: rjw@sisk.pl
> > >CC: mingo@elte.hu
> > >CC: Shaohua Li <shaohua.li@intel.com>
> > >CC: Pekka Enberg <penberg@cs.helsinki.fi>
> > >CC: Dave Young <hidave.darkstar@gmail.com>
> > >CC: "Rafael J. Wysocki" <rjw@sisk.pl>
> > >CC: Rusty Russell <rusty@rustcorp.com.au>
> > >CC: sven.wegener@stealer.net
> > >CC: cpufreq@vger.kernel.org
> > >CC: Thomas Renninger <trenn@suse.de>
> > >---
> > > drivers/cpufreq/cpufreq.c | 65
> > >++++++++++++++++++++++++++++------------------
> > > 1 file changed, 40 insertions(+), 25 deletions(-)
> > >
> > >Index: linux-2.6-lttng/drivers/cpufreq/cpufreq.c
> > >===================================================================
> > >--- linux-2.6-lttng.orig/drivers/cpufreq/cpufreq.c
> > >2009-07-02 23:59:08.000000000 -0400
> > >+++ linux-2.6-lttng/drivers/cpufreq/cpufreq.c 2009-07-02
> > >23:59:09.000000000 -0400
> > >@@ -763,6 +763,10 @@ static struct kobj_type ktype_cpufreq =
> > > * cpufreq_add_dev - add a CPU device
> > > *
> > > * Adds the cpufreq interface for a CPU device.
> > >+ *
> > >+ * The Oracle says: try running cpufreq
> > >registration/unregistration concurrently
> > >+ * with with cpu hotplugging and all hell will break loose.
> > >Tried to clean this
> > >+ * mess up, but more thorough testing is needed. - Mathieu
> > > */
> > > static int cpufreq_add_dev(struct sys_device *sys_dev)
> > > {
> > >@@ -806,15 +810,12 @@ static int cpufreq_add_dev(struct sys_de
> > > goto nomem_out;
> > > }
> > > if (!alloc_cpumask_var(&policy->cpus, GFP_KERNEL)) {
> > >- kfree(policy);
> > > ret = -ENOMEM;
> > >- goto nomem_out;
> > >+ goto err_free_policy;
> > > }
> > > if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL)) {
> > >- free_cpumask_var(policy->cpus);
> > >- kfree(policy);
> > > ret = -ENOMEM;
> > >- goto nomem_out;
> > >+ goto err_free_cpumask;
> > > }
> > >
> > > policy->cpu = cpu;
> > >@@ -822,7 +823,8 @@ static int cpufreq_add_dev(struct sys_de
> > >
> > > /* Initially set CPU itself as the policy_cpu */
> > > per_cpu(policy_cpu, cpu) = cpu;
> > >- lock_policy_rwsem_write(cpu);
> > >+ ret = (lock_policy_rwsem_write(cpu) < 0);
> > >+ WARN_ON(ret);
> > >
> > > init_completion(&policy->kobj_unregister);
> > > INIT_WORK(&policy->update, handle_update);
> > >@@ -835,7 +837,7 @@ static int cpufreq_add_dev(struct sys_de
> > > ret = cpufreq_driver->init(policy);
> > > if (ret) {
> > > dprintk("initialization failed\n");
> > >- goto err_out;
> > >+ goto err_unlock_policy;
> > > }
> > > policy->user_policy.min = policy->min;
> > > policy->user_policy.max = policy->max;
> > >@@ -860,15 +862,21 @@ static int cpufreq_add_dev(struct sys_de
> > > /* Check for existing affected CPUs.
> > > * They may not be aware of it due to CPU Hotplug.
> > > */
> > >- managed_policy = cpufreq_cpu_get(j);
> > >/* FIXME: Where is this released? What about error paths? */
> > >+ managed_policy = cpufreq_cpu_get(j);
> > > if (unlikely(managed_policy)) {
> > >
> > > /* Set proper policy_cpu */
> > > unlock_policy_rwsem_write(cpu);
> > > per_cpu(policy_cpu, cpu) = managed_policy->cpu;
> > >
> > >- if (lock_policy_rwsem_write(cpu) < 0)
> > >- goto err_out_driver_exit;
> > >+ if (lock_policy_rwsem_write(cpu) < 0) {
> > >+ /* Should not go through policy
> > >unlock path */
> > >+ if (cpufreq_driver->exit)
> > >+ cpufreq_driver->exit(policy);
> > >+ ret = -EBUSY;
> > >+ cpufreq_cpu_put(managed_policy);
> > >+ goto err_free_cpumask;
> > >+ }
> > >
> > > spin_lock_irqsave(&cpufreq_driver_lock, flags);
> > > cpumask_copy(managed_policy->cpus,
> > >policy->cpus);
> > >@@ -879,12 +887,14 @@ static int cpufreq_add_dev(struct sys_de
> > > ret = sysfs_create_link(&sys_dev->kobj,
> > > &managed_policy->kobj,
> > > "cpufreq");
> > >- if (ret)
> > >- goto err_out_driver_exit;
> > >-
> > >- cpufreq_debug_enable_ratelimit();
> > >- ret = 0;
> > >- goto err_out_driver_exit; /* call
> > >driver->exit() */
> > >+ if (!ret)
> > >+ cpufreq_cpu_put(managed_policy);
> >
> > Looks like cpufreq_cpu_put is needed both with ret and !ret. No?
> >
>
> No. ret == 0 path is a "success path" only creating a symlink, and
> therefore __cpufreq_remove_dev() will take care of calling the
> cpufreq_cpu_put() to decrement the reference count :
>
> static int __cpufreq_remove_dev(struct sys_device *sys_dev)
> {
> ...
>
> if (unlikely(cpu != data->cpu)) {
> dprintk("removing link\n");
> cpumask_clear_cpu(cpu, data->cpus);
> spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> sysfs_remove_link(&sys_dev->kobj, "cpufreq");
> cpufreq_cpu_put(data);
> cpufreq_debug_enable_ratelimit();
> unlock_policy_rwsem_write(cpu);
> return 0;
> }
>
> This is, at least, how I understand what is happening here.
>

Agreed.

Thanks,
Venki



\
 
 \ /
  Last update: 2009-07-06 20:07    [from the cache]
©2003-2011 Jasper Spaans