cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU

powernowk8_target() runs off a per-cpu work item and if the cpufreq_policy->cpu is different from the current one, it migrates the kworker to the target CPU by manipulating current->cpus_allowed. The function migrates the kworker back to the original CPU but this is still broken. Workqueue concurrency management requires the kworkers to stay on the same CPU and powernowk8_target() ends up triggerring BUG_ON(rq != this_rq()) in try_to_wake_up_local() if it contends on fidvid_mutex and sleeps.

It is unclear why this bug is being reported now. Duncan says it appeared to be a regression of 3.6-rc1 and couldn't reproduce it on 3.5. Bisection seemed to point to 63d95a91 "workqueue: use @pool instead of @gcwq or @cpu where applicable" which is an non-functional change. Given that the reproduce case sometimes took upto days to trigger, it's easy to be misled while bisecting. Maybe something made contention on fidvid_mutex more likely? I don't know.

This patch fixes the bug by punting to another per-cpu work item on the target CPU if it isn't the same as the current one. The code assumes that cpufreq_policy->cpu is kept online by the caller, which Rafael tells me is the case.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Duncan <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc [email protected]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47301
---

While it's very late in the merge cycle, the fix is limited in scope and fairly safe, so it wouldn't be too crazy to merge but then again this can go through the next -rc1 and then -stable. Linus, Rafael, what do you guys think?

 drivers/cpufreq/powernow-k8.c |   89 +++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 36 deletions(-)

drivers/cpufreq/powernow-k8.c +53 -36

index c0e8164..53db9de 100644
@@ -35,7 +35,6 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/cpumask.h>
-#include <linux/sched.h>	/* for current / set_cpus_allowed() */
 #include <linux/io.h>
 #include <linux/delay.h>
 

@@ -1139,46 +1138,43 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	return res;
 }
 
-/* Driver entry point to switch to the target frequency */
-static int powernowk8_target(struct cpufreq_policy *pol,
-		unsigned targfreq, unsigned relation)
+struct powernowk8_target_work {
+	struct work_struct		work;
+	struct cpufreq_policy		*pol;
+	unsigned			targfreq;
+	unsigned			relation;
+	int				ret;
+};
+
+static void powernowk8_target_on_cpu(struct work_struct *work)
 {
-	cpumask_var_t oldmask;
+	struct powernowk8_target_work *tw =
+		container_of(work, struct powernowk8_target_work, work);
+	struct cpufreq_policy *pol = tw->pol;
 	struct powernow_k8_data *data = per_cpu(powernow_data, pol->cpu);
 	u32 checkfid;
 	u32 checkvid;
 	unsigned int newstate;
-	int ret = -EIO;
 
+	tw->ret = -EINVAL;
 	if (!data)
-		return -EINVAL;
+		return;
+
+	tw->ret = -EIO;
 
 	checkfid = data->currfid;
 	checkvid = data->currvid;
 
-	/* only run on specific CPU from here on. */
-	/* This is poor form: use a workqueue or smp_call_function_single */
-	if (!alloc_cpumask_var(&oldmask, GFP_KERNEL))
-		return -ENOMEM;
-
-	cpumask_copy(oldmask, tsk_cpus_allowed(current));
-	set_cpus_allowed_ptr(current, cpumask_of(pol->cpu));
-
-	if (smp_processor_id() != pol->cpu) {
-		printk(KERN_ERR PFX "limiting to cpu %u failed\n", pol->cpu);
-		goto err_out;
-	}
-
 	if (pending_bit_stuck()) {
 		printk(KERN_ERR PFX "failing targ, change pending bit set\n");
-		goto err_out;
+		return;
 	}
 
 	pr_debug("targ: cpu %d, %d kHz, min %d, max %d, relation %d\n",
-		pol->cpu, targfreq, pol->min, pol->max, relation);
+		pol->cpu, tw->targfreq, pol->min, pol->max, tw->relation);
 
 	if (query_current_values_with_pending_wait(data))
-		goto err_out;
+		return;
 
 	if (cpu_family != CPU_HW_PSTATE) {
 		pr_debug("targ: curr fid 0x%x, vid 0x%x\n",

@@ -1195,23 +1191,23 @@ static int powernowk8_target(struct cpufreq_policy *pol,
 	}
 
 	if (cpufreq_frequency_table_target(pol, data->powernow_table,
-				targfreq, relation, &newstate))
-		goto err_out;
+				tw->targfreq, tw->relation, &newstate))
+		return;
 
 	mutex_lock(&fidvid_mutex);
 
 	powernow_k8_acpi_pst_values(data, newstate);
 
 	if (cpu_family == CPU_HW_PSTATE)
-		ret = transition_frequency_pstate(data,
-			data->powernow_table[newstate].index);
+		tw->ret = transition_frequency_pstate(data,
+				data->powernow_table[newstate].index);
 	else
-		ret = transition_frequency_fidvid(data, newstate);
-	if (ret) {
+		tw->ret = transition_frequency_fidvid(data, newstate);
+	if (tw->ret) {
 		printk(KERN_ERR PFX "transition frequency failed\n");
-		ret = 1;
+		tw->ret = 1;
 		mutex_unlock(&fidvid_mutex);
-		goto err_out;
+		return;
 	}
 	mutex_unlock(&fidvid_mutex);
 

@@ -1220,12 +1216,33 @@ static int powernowk8_target(struct cpufreq_policy *pol,
 				data->powernow_table[newstate].index);
 	else
 		pol->cur = find_khz_freq_from_fid(data->currfid);
-	ret = 0;
 
-err_out:
-	set_cpus_allowed_ptr(current, oldmask);
-	free_cpumask_var(oldmask);
-	return ret;
+	tw->ret = 0;
+}
+
+/* Driver entry point to switch to the target frequency */
+static int powernowk8_target(struct cpufreq_policy *pol,
+		unsigned targfreq, unsigned relation)
+{
+	struct powernowk8_target_work tw;
+
+	/*
+	 * Must run on @pol->cpu.  Bounce to workqueue if necessary.
+	 * cpufreq core is responsible for ensuring the cpu stays online.
+	 */
+	INIT_WORK_ONSTACK(&tw.work, powernowk8_target_on_cpu);
+	tw.pol = pol;
+	tw.targfreq = targfreq;
+	tw.relation = relation;
+
+	if (smp_processor_id() == pol->cpu) {
+		powernowk8_target_on_cpu(&tw.work);
+	} else {
+		schedule_work_on(pol->cpu, &tw.work);
+		flush_work(&tw.work);
+	}
+
+	return tw.ret;
 }
 
 /* Driver entry point to verify the policy and range of frequencies */

Quoted 27 lines · Tejun Heo

powernowk8_target() runs off a per-cpu work item and if the cpufreq_policy->cpu is different from the current one, it migrates the kworker to the target CPU by manipulating current->cpus_allowed. The function migrates the kworker back to the original CPU but this is still broken. Workqueue concurrency management requires the kworkers to stay on the same CPU and powernowk8_target() ends up triggerring BUG_ON(rq != this_rq()) in try_to_wake_up_local() if it contends on fidvid_mutex and sleeps.

It is unclear why this bug is being reported now. Duncan says it appeared to be a regression of 3.6-rc1 and couldn't reproduce it on 3.5. Bisection seemed to point to 63d95a91 "workqueue: use @pool instead of @gcwq or @cpu where applicable" which is an non-functional change. Given that the reproduce case sometimes took upto days to trigger, it's easy to be misled while bisecting. Maybe something made contention on fidvid_mutex more likely? I don't know.

This patch fixes the bug by punting to another per-cpu work item on the target CPU if it isn't the same as the current one. The code assumes that cpufreq_policy->cpu is kept online by the caller, which Rafael tells me is the case.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Duncan <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc [email protected]

Quoted 7 lines

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47301
---

While it's very late in the merge cycle, the fix is limited in scope and fairly safe, so it wouldn't be too crazy to merge but then again this can go through the next -rc1 and then -stable. Linus, Rafael, what do you guys think?

Well, I don't see much reason to wait with this, although I'd like some more people to check it.

Andre, Thomas, can you please have a look at it?

drivers/cpufreq/powernow-k8.c static int powernowk8_target(struct cpufreq_policy *pol, · line 1233

Quoted 1 line

drivers/cpufreq/powernow-k8.c +1 -0

+	INIT_WORK_ONSTACK(&tw.work, powernowk8_target_on_cpu);

powernowk8_target() runs off a per-cpu work item and if the cpufreq_policy->cpu is different from the current one, it migrates the kworker to the target CPU by manipulating current->cpus_allowed. The function migrates the kworker back to the original CPU but this is still broken. Workqueue concurrency management requires the kworkers to stay on the same CPU and powernowk8_target() ends up triggerring BUG_ON(rq != this_rq()) in try_to_wake_up_local() if it contends on fidvid_mutex and sleeps.

It is unclear why this bug is being reported now. Duncan says it appeared to be a regression of 3.6-rc1 and couldn't reproduce it on 3.5. Bisection seemed to point to 63d95a91 "workqueue: use @pool instead of @gcwq or @cpu where applicable" which is an non-functional change. Given that the reproduce case sometimes took upto days to trigger, it's easy to be misled while bisecting. Maybe something made contention on fidvid_mutex more likely? I don't know.

This patch fixes the bug by punting to another per-cpu work item on the target CPU if it isn't the same as the current one. The code assumes that cpufreq_policy->cpu is kept online by the caller, which Rafael tells me is the case.

Signed-off-by Tejun Heo <[email protected]>

Reported-by Duncan <[email protected]>

Cc Rafael J. Wysocki <[email protected]>

Cc Andreas Herrmann <[email protected]>

Cc [email protected]

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=47301
---

While it's very late in the merge cycle, the fix is limited in scope and fairly safe, so it wouldn't be too crazy to merge but then again this can go through the next -rc1 and then -stable. Linus, Rafael, what do you guys think?

 drivers/cpufreq/powernow-k8.c |   89 +++++++++++++++++++++++++-----------------
 1 file changed, 53 insertions(+), 36 deletions(-)

Wouldn't it be much simpler to carve out the piece after set_cpus_allowed_ptr(), put it in a sub-function called __powernowk8_target() and call it with smp_call_function_single instead of defining another work item?

Would the workqueue code handle that or are there any other issues?

Quoted 6 lines · Borislav Petkov

Wouldn't it be much simpler to carve out the piece after set_cpus_allowed_ptr(), put it in a sub-function called __powernowk8_target() and call it with smp_call_function_single instead of defining another work item?

Would the workqueue code handle that or are there any other issues?

The function grabs a mutex. smp_call_function wouldn't be too happy about that.

So, with work_on_cpu() reimplementation just posted[1], we can do the following instead. Functionally it's about the same but less ugly. Ugly as it may be, I think the previous open coded version is better suited as a fix and for -stable. Thoughts?

[1] https://lkml.org/lkml/2012/9/18/430

 drivers/cpufreq/powernow-k8.c |   63 ++++++++++++++++++++++--------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

drivers/cpufreq/powernow-k8.c +34 -29

index c0e8164..1a40935 100644
@@ -35,7 +35,6 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/cpumask.h>
-#include <linux/sched.h>	/* for current / set_cpus_allowed() */
 #include <linux/io.h>
 #include <linux/delay.h>
 

@@ -1139,16 +1138,23 @@ static int transition_frequency_pstate(struct powernow_k8_data *data,
 	return res;
 }
 
-/* Driver entry point to switch to the target frequency */
-static int powernowk8_target(struct cpufreq_policy *pol,
-		unsigned targfreq, unsigned relation)
+struct powernowk8_target_arg {
+	struct cpufreq_policy		*pol;
+	unsigned			targfreq;
+	unsigned			relation;
+};
+
+static long powernowk8_target_fn(void *arg)
 {
-	cpumask_var_t oldmask;
+	struct powernowk8_target_arg *pta = arg;
+	struct cpufreq_policy *pol = pta->pol;
+	unsigned targfreq = pta->targfreq;
+	unsigned relation = pta->relation;
 	struct powernow_k8_data *data = per_cpu(powernow_data, pol->cpu);
 	u32 checkfid;
 	u32 checkvid;
 	unsigned int newstate;
-	int ret = -EIO;
+	int ret;
 
 	if (!data)
 		return -EINVAL;

@@ -1156,29 +1162,16 @@ static int powernowk8_target(struct cpufreq_policy *pol,
 	checkfid = data->currfid;
 	checkvid = data->currvid;
 
-	/* only run on specific CPU from here on. */
-	/* This is poor form: use a workqueue or smp_call_function_single */
-	if (!alloc_cpumask_var(&oldmask, GFP_KERNEL))
-		return -ENOMEM;
-
-	cpumask_copy(oldmask, tsk_cpus_allowed(current));
-	set_cpus_allowed_ptr(current, cpumask_of(pol->cpu));
-
-	if (smp_processor_id() != pol->cpu) {
-		printk(KERN_ERR PFX "limiting to cpu %u failed\n", pol->cpu);
-		goto err_out;
-	}
-
 	if (pending_bit_stuck()) {
 		printk(KERN_ERR PFX "failing targ, change pending bit set\n");
-		goto err_out;
+		return -EIO;
 	}
 
 	pr_debug("targ: cpu %d, %d kHz, min %d, max %d, relation %d\n",
 		pol->cpu, targfreq, pol->min, pol->max, relation);
 
 	if (query_current_values_with_pending_wait(data))
-		goto err_out;
+		return -EIO;
 
 	if (cpu_family != CPU_HW_PSTATE) {
 		pr_debug("targ: curr fid 0x%x, vid 0x%x\n",

@@ -1196,7 +1189,7 @@ static int powernowk8_target(struct cpufreq_policy *pol,
 
 	if (cpufreq_frequency_table_target(pol, data->powernow_table,
 				targfreq, relation, &newstate))
-		goto err_out;
+		return -EIO;
 
 	mutex_lock(&fidvid_mutex);
 

@@ -1209,9 +1202,8 @@ static int powernowk8_target(struct cpufreq_policy *pol,
 		ret = transition_frequency_fidvid(data, newstate);
 	if (ret) {
 		printk(KERN_ERR PFX "transition frequency failed\n");
-		ret = 1;
 		mutex_unlock(&fidvid_mutex);
-		goto err_out;
+		return 1;
 	}
 	mutex_unlock(&fidvid_mutex);
 

@@ -1220,12 +1212,25 @@ static int powernowk8_target(struct cpufreq_policy *pol,
 				data->powernow_table[newstate].index);
 	else
 		pol->cur = find_khz_freq_from_fid(data->currfid);
-	ret = 0;
 
-err_out:
-	set_cpus_allowed_ptr(current, oldmask);
-	free_cpumask_var(oldmask);
-	return ret;
+	return 0;
+}
+
+/* Driver entry point to switch to the target frequency */
+static int powernowk8_target(struct cpufreq_policy *pol,
+		unsigned targfreq, unsigned relation)
+{
+	struct powernowk8_target_arg pta = { .pol = pol, .targfreq = targfreq,
+					     .relation = relation };
+
+	/*
+	 * Must run on @pol->cpu.  cpufreq core is responsible for ensuring
+	 * that we're bound to the current CPU and pol->cpu stays online.
+	 */
+	if (smp_processor_id() == pol->cpu)
+		return powernowk8_target_fn(&pta);
+	else
+		return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
 }
 
 /* Driver entry point to verify the policy and range of frequencies */

So, with work_on_cpu() reimplementation just posted[1], we can do the following instead. Functionally it's about the same but less ugly. Ugly as it may be, I think the previous open coded version is better suited as a fix and for -stable. Thoughts?