Commit 6889125b8b4e09c5e53e6ecab3433bed1ce198c9
(cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU)
causes powernow-k8 to trigger a preempt warning, e.g.:
BUG: using smp_processor_id() in preemptible [00000000] code: cpufreq/3776
caller is powernowk8_target+0x20/0x49
Pid: 3776, comm: cpufreq Not tainted 3.6.0 #9
Call Trace:
[<ffffffff8125b447>] debug_smp_processor_id+0xc7/0xe0
[<ffffffff814877e7>] powernowk8_target+0x20/0x49
[<ffffffff81482b02>] __cpufreq_driver_target+0x82/0x8a
[<ffffffff81484fc6>] cpufreq_governor_performance+0x4e/0x54
[<ffffffff81482c50>] __cpufreq_governor+0x8c/0xc9
[<ffffffff81482e6f>] __cpufreq_set_policy+0x1a9/0x21e
[<ffffffff814839af>] store_scaling_governor+0x16f/0x19b
[<ffffffff81484f16>] ? cpufreq_update_policy+0x124/0x124
[<ffffffff8162b4a5>] ? _raw_spin_unlock_irqrestore+0x2c/0x49
[<ffffffff81483640>] store+0x60/0x88
[<ffffffff811708c0>] sysfs_write_file+0xf4/0x130
[<ffffffff8111243b>] vfs_write+0xb5/0x151
[<ffffffff811126e0>] sys_write+0x4a/0x71
[<ffffffff816319a9>] system_call_fastpath+0x16/0x1b
Fix this by using get_cpu/put_cpu in powernowk8_target().
Cc: Tejun Heo <[email protected]>
Cc: [email protected]
Signed-off-by: Andreas Herrmann <[email protected]>
---
drivers/cpufreq/powernow-k8.c | 19 +++++++++++--------
1 file changed, 11 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index 1a40935..3d1df2a 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -1220,17 +1220,20 @@ static long powernowk8_target_fn(void *arg)
static int powernowk8_target(struct cpufreq_policy *pol,
unsigned targfreq, unsigned relation)
{
+ int this_cpu, ret;
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);
+ this_cpu = get_cpu();
+ if (this_cpu == pol->cpu) {
+ ret = powernowk8_target_fn(&pta);
+ put_cpu();
+ } else {
+ put_cpu();
+ ret = work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
+ }
+
+ return ret;
}
/* Driver entry point to verify the policy and range of frequencies */
--
1.7.12
Hello,
On Tue, Oct 09, 2012 at 09:38:44PM +0200, Andreas Herrmann wrote:
>
> Commit 6889125b8b4e09c5e53e6ecab3433bed1ce198c9
> (cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU)
> causes powernow-k8 to trigger a preempt warning, e.g.:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: cpufreq/3776
> caller is powernowk8_target+0x20/0x49
> Pid: 3776, comm: cpufreq Not tainted 3.6.0 #9
> Call Trace:
> [<ffffffff8125b447>] debug_smp_processor_id+0xc7/0xe0
> [<ffffffff814877e7>] powernowk8_target+0x20/0x49
> [<ffffffff81482b02>] __cpufreq_driver_target+0x82/0x8a
> [<ffffffff81484fc6>] cpufreq_governor_performance+0x4e/0x54
> [<ffffffff81482c50>] __cpufreq_governor+0x8c/0xc9
> [<ffffffff81482e6f>] __cpufreq_set_policy+0x1a9/0x21e
> [<ffffffff814839af>] store_scaling_governor+0x16f/0x19b
> [<ffffffff81484f16>] ? cpufreq_update_policy+0x124/0x124
> [<ffffffff8162b4a5>] ? _raw_spin_unlock_irqrestore+0x2c/0x49
> [<ffffffff81483640>] store+0x60/0x88
> [<ffffffff811708c0>] sysfs_write_file+0xf4/0x130
> [<ffffffff8111243b>] vfs_write+0xb5/0x151
> [<ffffffff811126e0>] sys_write+0x4a/0x71
> [<ffffffff816319a9>] system_call_fastpath+0x16/0x1b
...
> - /*
> - * Must run on @pol->cpu. cpufreq core is responsible for ensuring
> - * that we're bound to the current CPU and pol->cpu stays online.
> - */
Urgh... so this wasn't true? Well, the perils of the last minute
changes.
> - if (smp_processor_id() == pol->cpu)
> - return powernowk8_target_fn(&pta);
> - else
> - return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
> + this_cpu = get_cpu();
> + if (this_cpu == pol->cpu) {
> + ret = powernowk8_target_fn(&pta);
> + put_cpu();
> + } else {
> + put_cpu();
> + ret = work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
> + }
> +
> + return ret;
Looking at the code, yes, I think the above is correct. Rafael, can
you please confirm?
Thanks.
--
tejun
Hello,
On Wed, Oct 10, 2012 at 9:48 PM, Andreas Herrmann
<[email protected]> wrote:
> Please ignore my patch. It was insufficiently tested -- sorry for this.
> (powernowk8_target_fn might sleep).
>
> Have to take a closer look how to avoid below issue.
Probably the only thing which can be done is always bouncing to the
percpu work item.
Thanks.
--
tejun
Rafael,
Please ignore my patch. It was insufficiently tested -- sorry for this.
(powernowk8_target_fn might sleep).
Have to take a closer look how to avoid below issue.
Regards,
Andreas
On Tue, Oct 09, 2012 at 09:38:44PM +0200, Andreas Herrmann wrote:
>
> Commit 6889125b8b4e09c5e53e6ecab3433bed1ce198c9
> (cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU)
> causes powernow-k8 to trigger a preempt warning, e.g.:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: cpufreq/3776
> caller is powernowk8_target+0x20/0x49
> Pid: 3776, comm: cpufreq Not tainted 3.6.0 #9
> Call Trace:
> [<ffffffff8125b447>] debug_smp_processor_id+0xc7/0xe0
> [<ffffffff814877e7>] powernowk8_target+0x20/0x49
> [<ffffffff81482b02>] __cpufreq_driver_target+0x82/0x8a
> [<ffffffff81484fc6>] cpufreq_governor_performance+0x4e/0x54
> [<ffffffff81482c50>] __cpufreq_governor+0x8c/0xc9
> [<ffffffff81482e6f>] __cpufreq_set_policy+0x1a9/0x21e
> [<ffffffff814839af>] store_scaling_governor+0x16f/0x19b
> [<ffffffff81484f16>] ? cpufreq_update_policy+0x124/0x124
> [<ffffffff8162b4a5>] ? _raw_spin_unlock_irqrestore+0x2c/0x49
> [<ffffffff81483640>] store+0x60/0x88
> [<ffffffff811708c0>] sysfs_write_file+0xf4/0x130
> [<ffffffff8111243b>] vfs_write+0xb5/0x151
> [<ffffffff811126e0>] sys_write+0x4a/0x71
> [<ffffffff816319a9>] system_call_fastpath+0x16/0x1b
>
> Fix this by using get_cpu/put_cpu in powernowk8_target().
>
> Cc: Tejun Heo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andreas Herrmann <[email protected]>
> ---
> drivers/cpufreq/powernow-k8.c | 19 +++++++++++--------
> 1 file changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index 1a40935..3d1df2a 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -1220,17 +1220,20 @@ static long powernowk8_target_fn(void *arg)
> static int powernowk8_target(struct cpufreq_policy *pol,
> unsigned targfreq, unsigned relation)
> {
> + int this_cpu, ret;
> 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);
> + this_cpu = get_cpu();
> + if (this_cpu == pol->cpu) {
> + ret = powernowk8_target_fn(&pta);
> + put_cpu();
> + } else {
> + put_cpu();
> + ret = work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
> + }
> +
> + return ret;
> }
>
> /* Driver entry point to verify the policy and range of frequencies */
> --
> 1.7.12
>
--
Operating | Advanced Micro Devices GmbH
System | Einsteinring 24, 85609 Dornach/Aschheim, Germany
Research | Geschäftsführer: Alberto Bozzo
Center | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
(OSRC) | Registergericht München, HRB Nr. 43632, WEEE-Reg-Nr: DE 12919551
Commit 6889125b8b4e09c5e53e6ecab3433bed1ce198c9
(cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU)
causes powernow-k8 to trigger a preempt warning, e.g.:
BUG: using smp_processor_id() in preemptible [00000000] code: cpufreq/3776
caller is powernowk8_target+0x20/0x49
Pid: 3776, comm: cpufreq Not tainted 3.6.0 #9
Call Trace:
[<ffffffff8125b447>] debug_smp_processor_id+0xc7/0xe0
[<ffffffff814877e7>] powernowk8_target+0x20/0x49
[<ffffffff81482b02>] __cpufreq_driver_target+0x82/0x8a
[<ffffffff81484fc6>] cpufreq_governor_performance+0x4e/0x54
[<ffffffff81482c50>] __cpufreq_governor+0x8c/0xc9
[<ffffffff81482e6f>] __cpufreq_set_policy+0x1a9/0x21e
[<ffffffff814839af>] store_scaling_governor+0x16f/0x19b
[<ffffffff81484f16>] ? cpufreq_update_policy+0x124/0x124
[<ffffffff8162b4a5>] ? _raw_spin_unlock_irqrestore+0x2c/0x49
[<ffffffff81483640>] store+0x60/0x88
[<ffffffff811708c0>] sysfs_write_file+0xf4/0x130
[<ffffffff8111243b>] vfs_write+0xb5/0x151
[<ffffffff811126e0>] sys_write+0x4a/0x71
[<ffffffff816319a9>] system_call_fastpath+0x16/0x1b
Fix this by by always using work_on_cpu().
Cc: Tejun Heo <[email protected]>
Cc: [email protected]
Signed-off-by: Andreas Herrmann <[email protected]>
---
drivers/cpufreq/powernow-k8.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
index 129e80b..c16a3a5 100644
--- a/drivers/cpufreq/powernow-k8.c
+++ b/drivers/cpufreq/powernow-k8.c
@@ -1052,14 +1052,7 @@ static int powernowk8_target(struct cpufreq_policy *pol,
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);
+ return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
}
/* Driver entry point to verify the policy and range of frequencies */
--
1.7.12
Hi,
Thanks for the patch! I'll queue it up for v3.7 when I get back home from
the current trip (around the -rc3 time frame I suppose).
In future please don't send patches directly to [email protected].
That doesn't make -stable pick them up anyway and confuses things.
Thanks,
Rafael
On Friday 12 of October 2012 17:18:41 Andreas Herrmann wrote:
>
> Commit 6889125b8b4e09c5e53e6ecab3433bed1ce198c9
> (cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU)
> causes powernow-k8 to trigger a preempt warning, e.g.:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: cpufreq/3776
> caller is powernowk8_target+0x20/0x49
> Pid: 3776, comm: cpufreq Not tainted 3.6.0 #9
> Call Trace:
> [<ffffffff8125b447>] debug_smp_processor_id+0xc7/0xe0
> [<ffffffff814877e7>] powernowk8_target+0x20/0x49
> [<ffffffff81482b02>] __cpufreq_driver_target+0x82/0x8a
> [<ffffffff81484fc6>] cpufreq_governor_performance+0x4e/0x54
> [<ffffffff81482c50>] __cpufreq_governor+0x8c/0xc9
> [<ffffffff81482e6f>] __cpufreq_set_policy+0x1a9/0x21e
> [<ffffffff814839af>] store_scaling_governor+0x16f/0x19b
> [<ffffffff81484f16>] ? cpufreq_update_policy+0x124/0x124
> [<ffffffff8162b4a5>] ? _raw_spin_unlock_irqrestore+0x2c/0x49
> [<ffffffff81483640>] store+0x60/0x88
> [<ffffffff811708c0>] sysfs_write_file+0xf4/0x130
> [<ffffffff8111243b>] vfs_write+0xb5/0x151
> [<ffffffff811126e0>] sys_write+0x4a/0x71
> [<ffffffff816319a9>] system_call_fastpath+0x16/0x1b
>
> Fix this by by always using work_on_cpu().
>
> Cc: Tejun Heo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andreas Herrmann <[email protected]>
> ---
> drivers/cpufreq/powernow-k8.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index 129e80b..c16a3a5 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -1052,14 +1052,7 @@ static int powernowk8_target(struct cpufreq_policy *pol,
> 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);
> + return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
> }
>
> /* Driver entry point to verify the policy and range of frequencies */
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Sun, Oct 14, 2012 at 10:27:22AM +0200, Rafael J. Wysocki wrote:
> Hi,
>
> Thanks for the patch! I'll queue it up for v3.7 when I get back home from
> the current trip (around the -rc3 time frame I suppose).
>
> In future please don't send patches directly to [email protected].
> That doesn't make -stable pick them up anyway and confuses things.
That happens anyway if you tag the patch for stable and use git
send-email. Unless you go the extra mile and filter out the cc list,
which is tedious.
Besides, I'm pretty sure stable maintainers verify a patch is actually
upstream before applying it anyway.
--
Regards/Gruss,
Boris.
> On Sunday 14 of October 2012 10:27:22 Rafael J. Wysocki wrote:
> > Hi,
> >
> > Thanks for the patch! I'll queue it up for v3.7 when I get back home from
> > the current trip (around the -rc3 time frame I suppose).
> >
> > In future please don't send patches directly to [email protected].
> > That doesn't make -stable pick them up anyway and confuses things.
>
> That happens anyway if you tag the patch for stable and use git
> send-email. Unless you go the extra mile and filter out the cc list,
> which is tedious.
Well, please don't tag patches for -stable, because -stable doesn't take
_patches_. It takes commits from the Linus' tree and backports them and
that's maintainer's job to tag them for -stable, not yours.
You can give the maintainer a hint that you _think_ it's -stable material
(e.g. in the additional patch description that goes after the changelog),
but the maintainer may still disagree with you and may not tag the commit
for -stable after all.
> Besides, I'm pretty sure stable maintainers verify a patch is actually
> upstream before applying it anyway.
Yes, they do, but that means it doesn't make sense to send them stuff
before it's been merged, right?
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Mon, Oct 15, 2012 at 07:50:13AM +0200, Rafael J. Wysocki wrote:
> Well, please don't tag patches for -stable, because -stable doesn't
> take _patches_.
Really, I didn't know that?! :-)
> It takes commits from the Linus' tree and backports them and that's
> maintainer's job to tag them for -stable, not yours.
You're not serious, right? This is not the case in at least 50% of the
cases.
And this is OK because maintainers don't always know whether the patch
should be tagged for stable. So yes, people should add the stable tag
and yes, committers still have a veto over it.
And yes, Andreas and I *know* how stable patches get applied, thank you
very much.
[ … ]
> Yes, they do, but that means it doesn't make sense to send them stuff
> before it's been merged, right?
Ok, I get it, you don't want people to send patches to stable@vger
*before* they've hit mainline.
Nothing in <Documentation/stable_kernel_rules.txt> states that
stable@vger shouldn't get CCed on submissions unless the patch is
upstream and besides, stable@vger gets CCed in a lot of discussions
anyway so there's other traffic just the same.
Bottomline: If you think people shouldn't spam stable@vger, then tough
luck - I don't think you can stop people from accidentally/due to the
automated nature of the process, CC stable. Even if it said so in the
above doc file.
As a result, stable maintainers simply rely on scripts which verify the
patch is actually upstream before applying it to stable.
--
Regards/Gruss,
Boris.
On Monday 15 of October 2012 10:40:11 Borislav Petkov wrote:
> On Mon, Oct 15, 2012 at 07:50:13AM +0200, Rafael J. Wysocki wrote:
> > Well, please don't tag patches for -stable, because -stable doesn't
> > take _patches_.
>
> Really, I didn't know that?! :-)
>
> > It takes commits from the Linus' tree and backports them and that's
> > maintainer's job to tag them for -stable, not yours.
>
> You're not serious, right? This is not the case in at least 50% of the
> cases.
>
> And this is OK because maintainers don't always know whether the patch
> should be tagged for stable. So yes, people should add the stable tag
> and yes, committers still have a veto over it.
>
> And yes, Andreas and I *know* how stable patches get applied, thank you
> very much.
I didn't say you didn't know that.
> [ … ]
>
> > Yes, they do, but that means it doesn't make sense to send them stuff
> > before it's been merged, right?
>
> Ok, I get it, you don't want people to send patches to stable@vger
> *before* they've hit mainline.
>
> Nothing in <Documentation/stable_kernel_rules.txt> states that
> stable@vger shouldn't get CCed on submissions unless the patch is
> upstream and besides, stable@vger gets CCed in a lot of discussions
> anyway so there's other traffic just the same.
>
> Bottomline: If you think people shouldn't spam stable@vger, then tough
> luck - I don't think you can stop people from accidentally/due to the
> automated nature of the process, CC stable. Even if it said so in the
> above doc file.
>
> As a result, stable maintainers simply rely on scripts which verify the
> patch is actually upstream before applying it to stable.
Well, perhaps I shouldn't care too, then. :-)
Thanks,
Rafael
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Friday 12 of October 2012 17:18:41 Andreas Herrmann wrote:
>
> Commit 6889125b8b4e09c5e53e6ecab3433bed1ce198c9
> (cpufreq/powernow-k8: workqueue user shouldn't migrate the kworker to another CPU)
> causes powernow-k8 to trigger a preempt warning, e.g.:
>
> BUG: using smp_processor_id() in preemptible [00000000] code: cpufreq/3776
> caller is powernowk8_target+0x20/0x49
> Pid: 3776, comm: cpufreq Not tainted 3.6.0 #9
> Call Trace:
> [<ffffffff8125b447>] debug_smp_processor_id+0xc7/0xe0
> [<ffffffff814877e7>] powernowk8_target+0x20/0x49
> [<ffffffff81482b02>] __cpufreq_driver_target+0x82/0x8a
> [<ffffffff81484fc6>] cpufreq_governor_performance+0x4e/0x54
> [<ffffffff81482c50>] __cpufreq_governor+0x8c/0xc9
> [<ffffffff81482e6f>] __cpufreq_set_policy+0x1a9/0x21e
> [<ffffffff814839af>] store_scaling_governor+0x16f/0x19b
> [<ffffffff81484f16>] ? cpufreq_update_policy+0x124/0x124
> [<ffffffff8162b4a5>] ? _raw_spin_unlock_irqrestore+0x2c/0x49
> [<ffffffff81483640>] store+0x60/0x88
> [<ffffffff811708c0>] sysfs_write_file+0xf4/0x130
> [<ffffffff8111243b>] vfs_write+0xb5/0x151
> [<ffffffff811126e0>] sys_write+0x4a/0x71
> [<ffffffff816319a9>] system_call_fastpath+0x16/0x1b
>
> Fix this by by always using work_on_cpu().
>
> Cc: Tejun Heo <[email protected]>
> Cc: [email protected]
> Signed-off-by: Andreas Herrmann <[email protected]>
Applied to linux-pm.git/linux-next as v3.7 material, will be included in the
next linux-pm push.
Thanks,
Rafael
> ---
> drivers/cpufreq/powernow-k8.c | 9 +--------
> 1 file changed, 1 insertion(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/powernow-k8.c b/drivers/cpufreq/powernow-k8.c
> index 129e80b..c16a3a5 100644
> --- a/drivers/cpufreq/powernow-k8.c
> +++ b/drivers/cpufreq/powernow-k8.c
> @@ -1052,14 +1052,7 @@ static int powernowk8_target(struct cpufreq_policy *pol,
> 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);
> + return work_on_cpu(pol->cpu, powernowk8_target_fn, &pta);
> }
>
> /* Driver entry point to verify the policy and range of frequencies */
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.