linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code
@ 2012-10-09 19:38 Andreas Herrmann
  2012-10-10  0:18 ` Tejun Heo
  2012-10-10 12:48 ` Andreas Herrmann
  0 siblings, 2 replies; 11+ messages in thread
From: Andreas Herrmann @ 2012-10-09 19:38 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: linux-kernel, cpufreq, Tejun Heo, stable


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 <tj@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code
  2012-10-09 19:38 [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code Andreas Herrmann
@ 2012-10-10  0:18 ` Tejun Heo
  2012-10-10 12:48 ` Andreas Herrmann
  1 sibling, 0 replies; 11+ messages in thread
From: Tejun Heo @ 2012-10-10  0:18 UTC (permalink / raw)
  To: Andreas Herrmann; +Cc: Rafael J. Wysocki, linux-kernel, cpufreq, stable

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code
  2012-10-09 19:38 [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code Andreas Herrmann
  2012-10-10  0:18 ` Tejun Heo
@ 2012-10-10 12:48 ` Andreas Herrmann
  2012-10-10 12:51   ` Tejun Heo
  2012-10-12 15:18   ` [PATCH] cpufreq, powernow-k8: Remove " Andreas Herrmann
  1 sibling, 2 replies; 11+ messages in thread
From: Andreas Herrmann @ 2012-10-10 12:48 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: linux-kernel, cpufreq, Tejun Heo, stable

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 <tj@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code
  2012-10-10 12:48 ` Andreas Herrmann
@ 2012-10-10 12:51   ` Tejun Heo
  2012-10-12 15:18   ` [PATCH] cpufreq, powernow-k8: Remove " Andreas Herrmann
  1 sibling, 0 replies; 11+ messages in thread
From: Tejun Heo @ 2012-10-10 12:51 UTC (permalink / raw)
  To: Andreas Herrmann; +Cc: Rafael J. Wysocki, linux-kernel, cpufreq, stable

Hello,

On Wed, Oct 10, 2012 at 9:48 PM, Andreas Herrmann
<andreas.herrmann3@amd.com> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-10 12:48 ` Andreas Herrmann
  2012-10-10 12:51   ` Tejun Heo
@ 2012-10-12 15:18   ` Andreas Herrmann
  2012-10-14  8:27     ` Rafael J. Wysocki
  2012-10-24 21:57     ` Rafael J. Wysocki
  1 sibling, 2 replies; 11+ messages in thread
From: Andreas Herrmann @ 2012-10-12 15:18 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: linux-kernel, cpufreq, Tejun Heo, stable


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 <tj@kernel.org>
Cc: stable@vger.kernel.org
Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
---
 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



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-12 15:18   ` [PATCH] cpufreq, powernow-k8: Remove " Andreas Herrmann
@ 2012-10-14  8:27     ` Rafael J. Wysocki
  2012-10-14 15:40       ` Borislav Petkov
  2012-10-15  5:50       ` Rafael J. Wysocki
  2012-10-24 21:57     ` Rafael J. Wysocki
  1 sibling, 2 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2012-10-14  8:27 UTC (permalink / raw)
  To: Andreas Herrmann; +Cc: linux-kernel, cpufreq, Tejun Heo, linux-pm

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 stable@vger.kernel.org.
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 <tj@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>
> ---
>  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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-14  8:27     ` Rafael J. Wysocki
@ 2012-10-14 15:40       ` Borislav Petkov
  2012-10-15  5:50       ` Rafael J. Wysocki
  1 sibling, 0 replies; 11+ messages in thread
From: Borislav Petkov @ 2012-10-14 15:40 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andreas Herrmann, linux-kernel, cpufreq, Tejun Heo, linux-pm

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 stable@vger.kernel.org.
> 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-14  8:27     ` Rafael J. Wysocki
  2012-10-14 15:40       ` Borislav Petkov
@ 2012-10-15  5:50       ` Rafael J. Wysocki
  2012-10-15  8:40         ` Borislav Petkov
  1 sibling, 1 reply; 11+ messages in thread
From: Rafael J. Wysocki @ 2012-10-15  5:50 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Andreas Herrmann, linux-kernel, cpufreq, Tejun Heo, linux-pm

> 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 stable@vger.kernel.org.
> > 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-15  5:50       ` Rafael J. Wysocki
@ 2012-10-15  8:40         ` Borislav Petkov
  2012-10-15 17:56           ` Rafael J. Wysocki
  0 siblings, 1 reply; 11+ messages in thread
From: Borislav Petkov @ 2012-10-15  8:40 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Andreas Herrmann, linux-kernel, cpufreq, Tejun Heo, linux-pm

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-15  8:40         ` Borislav Petkov
@ 2012-10-15 17:56           ` Rafael J. Wysocki
  0 siblings, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2012-10-15 17:56 UTC (permalink / raw)
  To: Borislav Petkov
  Cc: Andreas Herrmann, linux-kernel, cpufreq, Tejun Heo, linux-pm

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] cpufreq, powernow-k8: Remove usage of smp_processor_id() in preemptible code
  2012-10-12 15:18   ` [PATCH] cpufreq, powernow-k8: Remove " Andreas Herrmann
  2012-10-14  8:27     ` Rafael J. Wysocki
@ 2012-10-24 21:57     ` Rafael J. Wysocki
  1 sibling, 0 replies; 11+ messages in thread
From: Rafael J. Wysocki @ 2012-10-24 21:57 UTC (permalink / raw)
  To: Andreas Herrmann; +Cc: linux-kernel, cpufreq, Tejun Heo, stable

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 <tj@kernel.org>
> Cc: stable@vger.kernel.org
> Signed-off-by: Andreas Herrmann <andreas.herrmann3@amd.com>

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2012-10-24 21:53 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-10-09 19:38 [PATCH] cpufreq, powernow-k8: Fix usage of smp_processor_id() in preemptible code Andreas Herrmann
2012-10-10  0:18 ` Tejun Heo
2012-10-10 12:48 ` Andreas Herrmann
2012-10-10 12:51   ` Tejun Heo
2012-10-12 15:18   ` [PATCH] cpufreq, powernow-k8: Remove " Andreas Herrmann
2012-10-14  8:27     ` Rafael J. Wysocki
2012-10-14 15:40       ` Borislav Petkov
2012-10-15  5:50       ` Rafael J. Wysocki
2012-10-15  8:40         ` Borislav Petkov
2012-10-15 17:56           ` Rafael J. Wysocki
2012-10-24 21:57     ` Rafael J. Wysocki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).