On Fri, 2009-07-03 at 12:41 -0700, Mathieu Desnoyers wrote: > * Pallipadi, Venkatesh (venkatesh.pallipadi@xxxxxxxxx) wrote: > > > > > > >-----Original Message----- > > >From: Mathieu Desnoyers [mailto:mathieu.desnoyers@xxxxxxxxxx] > > >Sent: Friday, July 03, 2009 8:25 AM > > >To: linux-kernel@xxxxxxxxxxxxxxx; Pallipadi, Venkatesh; Dave > > >Jones; Thomas Renninger; cpufreq@xxxxxxxxxxxxxxx; > > >kernel-testers@xxxxxxxxxxxxxxx; Ingo Molnar; rjw@xxxxxxx; Dave > > >Young; Pekka Enberg > > >Cc: Mathieu Desnoyers; Li, Shaohua; Rusty Russell; > > >sven.wegener@xxxxxxxxxxx > > >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@xxxxxxxxxx> > > >CC: Venkatesh Pallipadi <venkatesh.pallipadi@xxxxxxxxx> > > >CC: rjw@xxxxxxx > > >CC: mingo@xxxxxxx > > >CC: Shaohua Li <shaohua.li@xxxxxxxxx> > > >CC: Pekka Enberg <penberg@xxxxxxxxxxxxxx> > > >CC: Dave Young <hidave.darkstar@xxxxxxxxx> > > >CC: "Rafael J. Wysocki" <rjw@xxxxxxx> > > >CC: Rusty Russell <rusty@xxxxxxxxxxxxxxx> > > >CC: sven.wegener@xxxxxxxxxxx > > >CC: cpufreq@xxxxxxxxxxxxxxx > > >CC: Thomas Renninger <trenn@xxxxxxx> > > >--- > > > 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 -- To unsubscribe from this list: send the line "unsubscribe kernel-testers" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
