Re: [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess

[Pallipadi, Venkatesh]

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/