Skip to contentSkip to conversation

Commit a66b2e50 "Preserve sysfs files across suspend/resume" causes a regression in intel_pstate

REGRESSION Dirk Brandewie 4 participants 7 messages Jul 10, 2013 – Jul 11, 2013 CPU frequency scaling framework

lore
Share & tools
lore
#0 lore raw

Hi All,

Tianyu debugged into https://bugzilla.kernel.org/show_bug.cgi?id=59781 and found that commit a66b2e50 is causing the regression.

Tianyu has proposed a fix (patch attached to bugzilla) but having scaling drivers receive hotplug notifications through two paths seems weird.

Looking at the core code and some of the other scaling drivers I don't see an obvious fix. Maybe adding optional suspend/resume callbacks to the scaling driver interface?

All the scaling drivers that need to do stateful work in the init/exit callbacks are being affected by this change so I think there are other subtle side-effects out there that haven't been noticed yet.

I am not sure how we should proceed here?

Thoughts?
--Dirk

#1 lore raw
Quoted 1 line · Dirk Brandewie

Hi All,

Hi,

Quoted 10 lines

Tianyu debugged into https://bugzilla.kernel.org/show_bug.cgi?id=59781 and found that commit a66b2e50 is causing the regression.

Tianyu has proposed a fix (patch attached to bugzilla) but having scaling drivers receive hotplug notifications through two paths seems weird.

Looking at the core code and some of the other scaling drivers I don't see an obvious fix. Maybe adding optional suspend/resume callbacks to the scaling driver interface?

Quoted 5 lines

All the scaling drivers that need to do stateful work in the init/exit callbacks are being affected by this change so I think there are other subtle side-effects out there that haven't been noticed yet.

I am not sure how we should proceed here?

Well, first off, I'll queue up a revert of commit a66b2e50, as this was really about being nice to user space than anything else. And it has caused subtle problems to happen already elsewhere.

Then we can figure out how to address the original issue.

Thanks,
Rafael

Signature · 2 lines
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
#2 lore raw
Quoted 19 lines · Rafael J. Wysocki
Quoted 1 line · Dirk Brandewie

Hi All,

Hi,

Quoted 14 lines

Tianyu debugged into https://bugzilla.kernel.org/show_bug.cgi?id=59781 and found that commit a66b2e50 is causing the regression.

Tianyu has proposed a fix (patch attached to bugzilla) but having scaling drivers receive hotplug notifications through two paths seems weird.

Looking at the core code and some of the other scaling drivers I don't see an obvious fix. Maybe adding optional suspend/resume callbacks to the scaling driver interface?

All the scaling drivers that need to do stateful work in the init/exit callbacks are being affected by this change so I think there are other subtle side-effects out there that haven't been noticed yet.

Quoted 8 lines
Quoted 1 line

I am not sure how we should proceed here?

Well, first off, I'll queue up a revert of commit a66b2e50, as this was really about being nice to user space than anything else. And it has caused subtle problems to happen already elsewhere.

Then we can figure out how to address the original issue.

Sure, that sounds like a good plan. Sorry about the mess it caused :( I do have a (large-ish) fix in the works (which I posted in my previous mail), but reverting the original commit first will be better, especially from the stable-tree perspective.

I can provide you the revert tomorrow after testing (the revert needs to take care of commit f51e1eb6 which went in later), but if you do it yourself by then, you can add my ACK to it.

Thanks a lot!
Regards,
Srivatsa S. Bhat

#3 lore raw
In reply to Dirk Brandewie
Quoted 13 lines · Dirk Brandewie

Hi All,

Tianyu debugged into https://bugzilla.kernel.org/show_bug.cgi?id=59781
and found
that commit a66b2e50 is causing the regression.

Tianyu has proposed a fix (patch attached to bugzilla) but having scaling drivers receive hotplug notifications through two paths seems weird.

Looking at the core code and some of the other scaling drivers I don't see an obvious fix. Maybe adding optional suspend/resume callbacks to the scaling driver interface?

Quoted 7 lines

All the scaling drivers that need to do stateful work in the init/exit callbacks are being affected by this change so I think there are other subtle side-effects out there that haven't been noticed yet.

I am not sure how we should proceed here?

Thoughts?

While writing that patch, I had taken a shortcut - instead of reorganizing the code such that I only skip the sysfs parts of cpufreq during suspend/resume, I went ahead and skipped the _entire_ add/remove calls of cpufreq subsystem. Obviously that didn't go so well, and for that I'm truly sorry.

Below is an untested patch which tries to do it the way it should have been done originally - by splitting the functionality into cpufreq low-level code and the sysfs part, and only skipping the sysfs part during suspend/resume. You might want to give it a try and see how it goes. But note that Rafael is planning to revert commit a66b2e50 altogether, so this might be moot.

(BTW, the first patch proposed in that bugzilla is already upstream, see commit f51e1eb cpufreq: Fix cpufreq regression after suspend/resume).

Regards,
Srivatsa S. Bhat

(Applies on current mainline)

---

 drivers/cpufreq/cpufreq.c |  144 ++++++++++++++++++++++++++-------------------
 1 file changed, 82 insertions(+), 62 deletions(-)
drivers/cpufreq/cpufreq.c +82 -62
index 6a015ad..28c690f 100644
@@ -834,11 +834,8 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 				     struct cpufreq_policy *policy,
 				     struct device *dev)
 {
-	struct cpufreq_policy new_policy;
 	struct freq_attr **drv_attr;
-	unsigned long flags;
 	int ret = 0;
-	unsigned int j;
 
 	/* prepare interface data */
 	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
@@ -870,31 +867,10 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 			goto err_out_kobj_put;
 	}
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	for_each_cpu(j, policy->cpus) {
-		per_cpu(cpufreq_cpu_data, j) = policy;
-		per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
-	}
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	ret = cpufreq_add_dev_symlink(cpu, policy);
 	if (ret)
 		goto err_out_kobj_put;
 
-	memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
-	/* assure that the starting sequence is run in __cpufreq_set_policy */
-	policy->governor = NULL;
-
-	/* set default policy */
-	ret = __cpufreq_set_policy(policy, &new_policy);
-	policy->user_policy.policy = policy->policy;
-	policy->user_policy.governor = policy->governor;
-
-	if (ret) {
-		pr_debug("setting policy failed\n");
-		if (cpufreq_driver->exit)
-			cpufreq_driver->exit(policy);
-	}
 	return ret;
 
 err_out_kobj_put:
@@ -905,7 +881,7 @@ err_out_kobj_put:
 
 #ifdef CONFIG_HOTPLUG_CPU
 static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
-				  struct device *dev)
+				  struct device *dev, bool init_sysfs)
 {
 	struct cpufreq_policy *policy;
 	int ret = 0, has_target = !!cpufreq_driver->target;
@@ -933,30 +909,25 @@ static int cpufreq_add_policy_cpu(unsigned int cpu, unsigned int sibling,
 		__cpufreq_governor(policy, CPUFREQ_GOV_LIMITS);
 	}
 
-	ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
-	if (ret) {
-		cpufreq_cpu_put(policy);
-		return ret;
+	if (init_sysfs) {
+		ret = sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
+		if (ret) {
+			cpufreq_cpu_put(policy);
+			return ret;
+		}
 	}
 
 	return 0;
 }
 #endif
 
-/**
- * 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 device *dev, struct subsys_interface *sif)
+static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
+			     bool init_sysfs)
 {
 	unsigned int j, cpu = dev->id;
 	int ret = -ENOMEM;
 	struct cpufreq_policy *policy;
+	struct cpufreq_policy new_policy;
 	unsigned long flags;
 #ifdef CONFIG_HOTPLUG_CPU
 	struct cpufreq_governor *gov;
@@ -984,7 +955,8 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		struct cpufreq_policy *cp = per_cpu(cpufreq_cpu_data, sibling);
 		if (cp && cpumask_test_cpu(cpu, cp->related_cpus)) {
 			read_unlock_irqrestore(&cpufreq_driver_lock, flags);
-			return cpufreq_add_policy_cpu(cpu, sibling, dev);
+			return cpufreq_add_policy_cpu(cpu, sibling, dev,
+						      init_sysfs);
 		}
 	}
 	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
@@ -1049,9 +1021,33 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	}
 #endif
 
-	ret = cpufreq_add_dev_interface(cpu, policy, dev);
-	if (ret)
-		goto err_out_unregister;
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	for_each_cpu(j, policy->cpus) {
+		per_cpu(cpufreq_cpu_data, j) = policy;
+		per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
+	}
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
+	if (init_sysfs) {
+		ret = cpufreq_add_dev_interface(cpu, policy, dev);
+		if (ret)
+			goto err_out_unregister;
+	}
+
+	memcpy(&new_policy, policy, sizeof(struct cpufreq_policy));
+	/* assure that the starting sequence is run in __cpufreq_set_policy */
+	policy->governor = NULL;
+
+	/* set default policy */
+	ret = __cpufreq_set_policy(policy, &new_policy);
+	policy->user_policy.policy = policy->policy;
+	policy->user_policy.governor = policy->governor;
+
+	if (ret) {
+		pr_debug("setting policy failed\n");
+		if (cpufreq_driver->exit)
+			cpufreq_driver->exit(policy);
+	}
 
 	kobject_uevent(&policy->kobj, KOBJ_ADD);
 	module_put(cpufreq_driver->owner);
@@ -1081,6 +1077,20 @@ module_out:
 	return ret;
 }
 
+/**
+ * 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 device *dev, struct subsys_interface *sif)
+{
+	return __cpufreq_add_dev(dev, sif, true);
+}
+
 static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
 	int j;
@@ -1106,7 +1116,7 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
  * This routine frees the rwsem before returning.
  */
 static int __cpufreq_remove_dev(struct device *dev,
-		struct subsys_interface *sif)
+				struct subsys_interface *sif, bool remove_sysfs)
 {
 	unsigned int cpu = dev->id, ret, cpus;
 	unsigned long flags;
@@ -1145,9 +1155,9 @@ static int __cpufreq_remove_dev(struct device *dev,
 		cpumask_clear_cpu(cpu, data->cpus);
 	unlock_policy_rwsem_write(cpu);
 
-	if (cpu != data->cpu) {
+	if (cpu != data->cpu && remove_sysfs) {
 		sysfs_remove_link(&dev->kobj, "cpufreq");
-	} else if (cpus > 1) {
+	} else if (cpus > 1 && remove_sysfs) {
 		/* first sibling now owns the new sysfs dir */
 		cpu_dev = get_cpu_device(cpumask_first(data->cpus));
 		sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
@@ -1184,26 +1194,25 @@ static int __cpufreq_remove_dev(struct device *dev,
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
-		lock_policy_rwsem_read(cpu);
-		kobj = &data->kobj;
-		cmp = &data->kobj_unregister;
-		unlock_policy_rwsem_read(cpu);
-		kobject_put(kobj);
-
-		/* we need to make sure that the underlying kobj is actually
-		 * not referenced anymore by anybody before we proceed with
-		 * unloading.
-		 */
-		pr_debug("waiting for dropping of refcount\n");
-		wait_for_completion(cmp);
-		pr_debug("wait complete\n");
-
 		if (cpufreq_driver->exit)
 			cpufreq_driver->exit(data);
 
 		free_cpumask_var(data->related_cpus);
 		free_cpumask_var(data->cpus);
 		kfree(data);
+
+		if (remove_sysfs) {
+			lock_policy_rwsem_read(cpu);
+			kobj = &data->kobj;
+			cmp = &data->kobj_unregister;
+			unlock_policy_rwsem_read(cpu);
+			kobject_put(kobj);
+
+			pr_debug("waiting for dropping of refcount\n");
+			wait_for_completion(cmp);
+			pr_debug("wait complete\n");
+		}
+
 	} else if (cpufreq_driver->target) {
 		__cpufreq_governor(data, CPUFREQ_GOV_START);
 		__cpufreq_governor(data, CPUFREQ_GOV_LIMITS);
@@ -1221,7 +1230,7 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 	if (cpu_is_offline(cpu))
 		return 0;
 
-	retval = __cpufreq_remove_dev(dev, sif);
+	retval = __cpufreq_remove_dev(dev, sif, true);
 	return retval;
 }
 
@@ -1943,13 +1952,24 @@ static int __cpuinit cpufreq_cpu_callback(struct notifier_block *nfb,
 		case CPU_ONLINE:
 			cpufreq_add_dev(dev, NULL);
 			break;
+		case CPU_ONLINE_FROZEN:
+			__cpufreq_add_dev(dev, NULL, false);
+			break;
+
 		case CPU_DOWN_PREPARE:
 		case CPU_UP_CANCELED_FROZEN:
-			__cpufreq_remove_dev(dev, NULL);
+			__cpufreq_remove_dev(dev, NULL, true);
+			break;
+		case CPU_DOWN_PREPARE_FROZEN:
+			__cpufreq_remove_dev(dev, NULL, false);
 			break;
+
 		case CPU_DOWN_FAILED:
 			cpufreq_add_dev(dev, NULL);
 			break;
+		case CPU_DOWN_FAILED_FROZEN:
+			__cpufreq_add_dev(dev, NULL, false);
+			break;
 		}
 	}
 	return NOTIFY_OK;
maintainer
#4 lore raw

On Thu, Jul 11, 2013 at 3:48 AM, Srivatsa S. Bhat
<[email protected]> wrote:

I haven't followed the minute details of this patch and will do that once you post it after testing it a bit.

BTW, I found a funny bug in there :)

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct device *dev, · line 1209

Quoted 8 lines
                kfree(data);
drivers/cpufreq/cpufreq.c +7 -0
+
+               if (remove_sysfs) {
+                       lock_policy_rwsem_read(cpu);
+                       kobj = &data->kobj;
+                       cmp = &data->kobj_unregister;
+                       unlock_policy_rwsem_read(cpu);
+                       kobject_put(kobj);

You just freed data and still want to use it later :)

#5 lore raw
Quoted 17 lines · Viresh Kumar

On Thu, Jul 11, 2013 at 3:48 AM, Srivatsa S. Bhat
<[email protected]> wrote:

I haven't followed the minute details of this patch and will do that once you post it after testing it a bit.

BTW, I found a funny bug in there :)

drivers/cpufreq/cpufreq.c static int __cpufreq_remove_dev(struct device *dev, · line 1209

Quoted 8 lines
                kfree(data);
drivers/cpufreq/cpufreq.c +7 -0
+
+               if (remove_sysfs) {
+                       lock_policy_rwsem_read(cpu);
+                       kobj = &data->kobj;
+                       cmp = &data->kobj_unregister;
+                       unlock_policy_rwsem_read(cpu);
+                       kobject_put(kobj);
Quoted 2 lines

You just freed data and still want to use it later :)

Yeah, Tianyu pointed that out to me in the other thread :)

http://marc.info/?l=linux-pm&m=137352402807997&w=2

... and I almost gave up trying to fix this mess, as I mentioned there :-(

If you have any smart solutions in mind to get this right, do share your thoughts! :)

Regards,
Srivatsa S. Bhat

maintainer
#6 lore raw
Quoted 9 lines · Srivatsa S. Bhat

Yeah, Tianyu pointed that out to me in the other thread :)

http://marc.info/?l=linux-pm&m=137352402807997&w=2

... and I almost gave up trying to fix this mess, as I mentioned there :-(

If you have any smart solutions in mind to get this right, do share your thoughts! :)

Busy currently with Linaro connect and so wasn't able to take up most of the stuff this week. Would have implement something similar to what you did.