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

[Pallipadi, Venkatesh]

>-----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?

Thanks,
Venki

>+			/*
>+			 * Success. We only needed to be added 
>to the mask.
>+			 * Call driver->exit() because only the 
>cpu parent of
>+			 * the kobj needed to call init().
>+			 */
>+			goto out_driver_exit; /* call driver->exit() */
> 		}
> 	}
> #endif
>@@ -894,25 +904,25 @@ static int cpufreq_add_dev(struct sys_de
> 	ret = kobject_init_and_add(&policy->kobj, 
>&ktype_cpufreq, &sys_dev->kobj,
> 				   "cpufreq");
> 	if (ret)
>-		goto err_out_driver_exit;
>+		goto out_driver_exit;
> 
> 	/* set up files for this cpu device */
> 	drv_attr = cpufreq_driver->attr;
> 	while ((drv_attr) && (*drv_attr)) {
> 		ret = sysfs_create_file(&policy->kobj, 
>&((*drv_attr)->attr));
> 		if (ret)
>-			goto err_out_driver_exit;
>+			goto err_out_kobj_put;
> 		drv_attr++;
> 	}
> 	if (cpufreq_driver->get) {
> 		ret = sysfs_create_file(&policy->kobj, 
>&cpuinfo_cur_freq.attr);
> 		if (ret)
>-			goto err_out_driver_exit;
>+			goto err_out_kobj_put;
> 	}
> 	if (cpufreq_driver->target) {
> 		ret = sysfs_create_file(&policy->kobj, 
>&scaling_cur_freq.attr);
> 		if (ret)
>-			goto err_out_driver_exit;
>+			goto err_out_kobj_put;
> 	}
> 
> 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
>@@ -930,12 +940,14 @@ static int cpufreq_add_dev(struct sys_de
> 			continue;
> 
> 		dprintk("CPU %u already managed, adding link\n", j);
>-		cpufreq_cpu_get(cpu);
>+		managed_policy = cpufreq_cpu_get(cpu);
> 		cpu_sys_dev = get_cpu_sysdev(j);
> 		ret = sysfs_create_link(&cpu_sys_dev->kobj, 
>&policy->kobj,
> 					"cpufreq");
>-		if (ret)
>+		if (ret) {
>+			cpufreq_cpu_put(managed_policy);
> 			goto err_out_unregister;
>+		}
> 	}
> 
> 	policy->governor = NULL; /* to assure that the starting 
>sequence is
>@@ -967,17 +979,20 @@ err_out_unregister:
> 		per_cpu(cpufreq_cpu_data, j) = NULL;
> 	spin_unlock_irqrestore(&cpufreq_driver_lock, flags);
> 
>+err_out_kobj_put:
> 	kobject_put(&policy->kobj);
> 	wait_for_completion(&policy->kobj_unregister);
> 
>-err_out_driver_exit:
>+out_driver_exit:
> 	if (cpufreq_driver->exit)
> 		cpufreq_driver->exit(policy);
> 
>-err_out:
>+err_unlock_policy:
> 	unlock_policy_rwsem_write(cpu);
>+err_free_cpumask:
>+	free_cpumask_var(policy->cpus);
>+err_free_policy:
> 	kfree(policy);
>-
> nomem_out:
> 	module_put(cpufreq_driver->owner);
> module_out:
>

--
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/