Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1622519 > unrolled thread

[patch 00/13] sched/treewide: Clean up various racy task affinity issues

Back to article view | Back to linux.kernel

Contents

  [patch 00/13] sched/treewide: Clean up various racy task affinity  issues Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
    [patch 12/13] cpufreq/sparc-us2e: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 12/13] cpufreq/sparc-us2e: Replace racy task affinity  logic Viresh Kumar <viresh.kumar@linaro.org> - 2017-04-13 05:00 +0200
      [patch V2 12/13] cpufreq/sparc-us2e: Replace racy task affinity  logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 10:20 +0200
        [patch V3 12/13] cpufreq/sparc-us2e: Replace racy task affinity  logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 10:30 +0200
          [tip:sched/core] cpufreq/sparc-us2e: Replace racy task affinity  logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
        Re: [patch V2 12/13] cpufreq/sparc-us2e: Replace racy task  affinity logic David Miller <davem@davemloft.net> - 2017-04-13 17:00 +0200
    [patch 03/13] ia64/salinfo: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      [tip:sched/core] ia64/salinfo: Replace racy task affinity logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 04/13] ia64/sn/hwperf: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      [patch V 2 04/13] ia64/sn/hwperf: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 23:00 +0200
        [tip:sched/core] ia64/sn/hwperf: Replace racy task affinity logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 11/13] cpufreq/sparc-us3: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 11/13] cpufreq/sparc-us3: Replace racy task affinity logic Viresh Kumar <viresh.kumar@linaro.org> - 2017-04-13 04:50 +0200
      [tip:sched/core] cpufreq/sparc-us3: Replace racy task affinity  logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 01/13] ia64/topology: Remove cpus_allowed manipulation Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      [tip:sched/core] ia64/topology: Remove cpus_allowed manipulation tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 07/13] ACPI/processor: Fix error handling in  __acpi_processor_start() Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      [tip:sched/core] ACPI/processor: Fix error handling in  __acpi_processor_start() tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 08/13] ACPI/processor: Replace racy task affinity logic. Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 08/13] ACPI/processor: Replace racy task affinity logic. Peter Zijlstra <peterz@infradead.org> - 2017-04-13 13:50 +0200
        Re: [patch 08/13] ACPI/processor: Replace racy task affinity  logic. Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 14:10 +0200
          Re: [patch 08/13] ACPI/processor: Replace racy task affinity logic. Peter Zijlstra <peterz@infradead.org> - 2017-04-13 15:00 +0200
      [tip:sched/core] ACPI/processor: Replace racy task affinity logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 13/13] crypto: n2 - Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 13/13] crypto: n2 - Replace racy task affinity logic Herbert Xu <herbert@gondor.apana.org.au> - 2017-04-13 07:40 +0200
      [patch V2 13/13] crypto: n2 - Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 10:30 +0200
        Re: [patch V2 13/13] crypto: n2 - Replace racy task affinity logic David Miller <davem@davemloft.net> - 2017-04-13 17:00 +0200
        [tip:sched/core] crypto: N2 - Replace racy task affinity logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:40 +0200
    [patch 05/13] powerpc/smp: Replace open coded task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 05/13] powerpc/smp: Replace open coded task affinity logic Michael Ellerman <mpe@ellerman.id.au> - 2017-04-13 07:50 +0200
      [tip:sched/core] powerpc/smp: Replace open coded task affinity  logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    [patch 06/13] sparc/sysfs: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 06/13] sparc/sysfs: Replace racy task affinity logic David Miller <davem@davemloft.net> - 2017-04-13 04:00 +0200
        [patch V2 06/13] sparc/sysfs: Replace racy task affinity logic Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 10:20 +0200
          [tip:sched/core] sparc/sysfs: Replace racy task affinity logic tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:20 +0200
    [patch 02/13] workqueue: Provide work_on_cpu_safe() Thomas Gleixner <tglx@linutronix.de> - 2017-04-12 22:30 +0200
      Re: [patch 02/13] workqueue: Provide work_on_cpu_safe() Dou Liyang <douly.fnst@cn.fujitsu.com> - 2017-04-13 13:20 +0200
        Re: [patch 02/13] workqueue: Provide work_on_cpu_safe() Thomas Gleixner <tglx@linutronix.de> - 2017-04-13 23:30 +0200
      Re: [patch 02/13] workqueue: Provide work_on_cpu_safe() Tejun Heo <tj@kernel.org> - 2017-04-14 06:20 +0200
      Re: [patch 02/13] workqueue: Provide work_on_cpu_safe() Peter Zijlstra <peterz@infradead.org> - 2017-04-14 11:00 +0200
        Re: [patch 02/13] workqueue: Provide work_on_cpu_safe() Thomas Gleixner <tglx@linutronix.de> - 2017-04-14 12:00 +0200
          Re: [patch 02/13] workqueue: Provide work_on_cpu_safe() Peter Zijlstra <peterz@infradead.org> - 2017-04-14 12:00 +0200
      [tip:sched/core] workqueue: Provide work_on_cpu_safe() tip-bot for Thomas Gleixner <tipbot@zytor.com> - 2017-04-15 16:30 +0200
    Re: [patch 00/13] sched/treewide: Clean up various racy task  affinity issues Peter Zijlstra <peterz@infradead.org> - 2017-04-13 11:10 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1622949 — Re: [patch 08/13] ACPI/processor: Replace racy task affinity logic.

On Wed, Apr 12, 2017 at 10:07:34PM +0200, Thomas Gleixner wrote:
> acpi_processor_get_throttling() requires to invoke the getter function on
> the target CPU. This is achieved by temporarily setting the affinity of the
> calling user space thread to the requested CPU and reset it to the original
> affinity afterwards.
> 
> That's racy vs. CPU hotplug and concurrent affinity settings for that
> thread resulting in code executing on the wrong CPU and overwriting the
> new affinity setting.
> 
> acpi_processor_get_throttling() is invoked in two ways:
> 
> 1) The CPU online callback, which is already running on the target CPU and
>    obviously protected against hotplug and not affected by affinity
>    settings.
> 
> 2) The ACPI driver probe function, which is not protected against hotplug
>    during modprobe.
> 
> Switch it over to work_on_cpu() and protect the probe function against CPU
> hotplug.
> 

> +static int acpi_processor_get_throttling(struct acpi_processor *pr)
> +{
>  	if (!pr)
>  		return -EINVAL;
>  
>  	if (!pr->flags.throttling)
>  		return -ENODEV;
>  
> +	 * This is either called from the CPU hotplug callback of
> +	 * processor_driver or via the ACPI probe function. In the latter
> +	 * case the CPU is not guaranteed to be online. Both call sites are
> +	 * protected against CPU hotplug.
>  	 */
> +	if (!cpu_online(pr->id))
>  		return -ENODEV;
>  
> +	return work_on_cpu(pr->id, __acpi_processor_get_throttling, pr);
>  }

That makes my machine sad...


[    9.583030] =============================================
[    9.589053] [ INFO: possible recursive locking detected ]
[    9.595079] 4.11.0-rc6-00385-g5aee78a-dirty #678 Not tainted
[    9.601393] ---------------------------------------------
[    9.607418] kworker/0:0/3 is trying to acquire lock:
[    9.612954]  ((&wfc.work)){+.+.+.}, at: [<ffffffff8110c172>] flush_work+0x12/0x2a0
[    9.621406] 
[    9.621406] but task is already holding lock:
[    9.627915]  ((&wfc.work)){+.+.+.}, at: [<ffffffff8110df17>] process_one_work+0x1e7/0x670
[    9.637044] 
[    9.637044] other info that might help us debug this:
[    9.644330]  Possible unsafe locking scenario:
[    9.644330] 
[    9.650934]        CPU0
[    9.653660]        ----
[    9.656386]   lock((&wfc.work));
[    9.659987]   lock((&wfc.work));
[    9.663586] 
[    9.663586]  *** DEADLOCK ***
[    9.663586] 
[    9.670189]  May be due to missing lock nesting notation
[    9.670189] 
[    9.677765] 2 locks held by kworker/0:0/3:
[    9.682332]  #0:  ("events"){.+.+.+}, at: [<ffffffff8110df17>] process_one_work+0x1e7/0x670
[    9.691654]  #1:  ((&wfc.work)){+.+.+.}, at: [<ffffffff8110df17>] process_one_work+0x1e7/0x670
[    9.701267] 
[    9.701267] stack backtrace:
[    9.706127] CPU: 0 PID: 3 Comm: kworker/0:0 Not tainted 4.11.0-rc6-00385-g5aee78a-dirty #678
[    9.715545] Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.02.0002.122320131210 12/23/2013
[    9.726999] Workqueue: events work_for_cpu_fn
[    9.731860] Call Trace:
[    9.734591]  dump_stack+0x86/0xcf
[    9.738290]  __lock_acquire+0x790/0x1620
[    9.742667]  ? __lock_acquire+0x4a5/0x1620
[    9.747237]  lock_acquire+0x100/0x210
[    9.751319]  ? lock_acquire+0x100/0x210
[    9.755596]  ? flush_work+0x12/0x2a0
[    9.759583]  flush_work+0x47/0x2a0
[    9.763375]  ? flush_work+0x12/0x2a0
[    9.767362]  ? queue_work_on+0x47/0xa0
[    9.771545]  ? __this_cpu_preempt_check+0x13/0x20
[    9.776792]  ? trace_hardirqs_on_caller+0xfb/0x1d0
[    9.782139]  ? trace_hardirqs_on+0xd/0x10
[    9.786610]  work_on_cpu+0x82/0x90
[    9.790404]  ? __usermodehelper_disable+0x110/0x110
[    9.795846]  ? __acpi_processor_get_throttling+0x20/0x20
[    9.801773]  acpi_processor_set_throttling+0x199/0x220
[    9.807506]  ? trace_hardirqs_on_caller+0xfb/0x1d0
[    9.812851]  acpi_processor_get_throttling_ptc+0xec/0x180
[    9.818876]  __acpi_processor_get_throttling+0xf/0x20
[    9.824511]  work_for_cpu_fn+0x14/0x20
[    9.828692]  process_one_work+0x261/0x670
[    9.833165]  worker_thread+0x21b/0x3f0
[    9.837348]  kthread+0x108/0x140
[    9.840947]  ? process_one_work+0x670/0x670
[    9.845611]  ? kthread_create_on_node+0x40/0x40
[    9.850667]  ret_from_fork+0x31/0x40

[toc] | [prev] | [next] | [standalone]

#1622959 — Re: [patch 08/13] ACPI/processor: Replace racy task affinity logic.

On Thu, 13 Apr 2017, Peter Zijlstra wrote:
> 
> That makes my machine sad...
> [    9.786610]  work_on_cpu+0x82/0x90
> [    9.790404]  ? __usermodehelper_disable+0x110/0x110
> [    9.795846]  ? __acpi_processor_get_throttling+0x20/0x20
> [    9.801773]  acpi_processor_set_throttling+0x199/0x220
> [    9.807506]  ? trace_hardirqs_on_caller+0xfb/0x1d0
> [    9.812851]  acpi_processor_get_throttling_ptc+0xec/0x180
> [    9.818876]  __acpi_processor_get_throttling+0xf/0x20
> [    9.824511]  work_for_cpu_fn+0x14/0x20
> [    9.828692]  process_one_work+0x261/0x670
> [    9.833165]  worker_thread+0x21b/0x3f0
> [    9.837348]  kthread+0x108/0x140
> [    9.840947]  ? process_one_work+0x670/0x670
> [    9.845611]  ? kthread_create_on_node+0x40/0x40
> [    9.850667]  ret_from_fork+0x31/0x40

Yuck. So the call chain is:

acpi_processor_get_throttling()
  work_on_cpu(acpi_processor_get_throttling)

That work does:

__acpi_processor_get_throttling()
    acpi_processor_get_throttling_ptc()
      acpi_processor_set_throttling()
        work_on_cpu(__acpi_processor_set_throttling)

Why the heck calls a get_throttling() function set_throttling()? I'm mildly
surprised.

Does the delta patch below cure the problem?

Thanks,

	tglx

8<--------------

--- a/drivers/acpi/processor_throttling.c
+++ b/drivers/acpi/processor_throttling.c
@@ -62,8 +62,8 @@ struct acpi_processor_throttling_arg {
 #define THROTTLING_POSTCHANGE      (2)
 
 static int acpi_processor_get_throttling(struct acpi_processor *pr);
-int acpi_processor_set_throttling(struct acpi_processor *pr,
-						int state, bool force);
+static int __acpi_processor_set_throttling(struct acpi_processor *pr,
+					   int state, bool force, bool direct);
 
 static int acpi_processor_update_tsd_coord(void)
 {
@@ -891,7 +891,8 @@ static int acpi_processor_get_throttling
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				"Invalid throttling state, reset\n"));
 			state = 0;
-			ret = acpi_processor_set_throttling(pr, state, true);
+			ret = __acpi_processor_set_throttling(pr, state, true,
+							      true);
 			if (ret)
 				return ret;
 		}
@@ -1075,8 +1076,15 @@ static long acpi_processor_throttling_fn
 			arg->target_state, arg->force);
 }
 
-int acpi_processor_set_throttling(struct acpi_processor *pr,
-						int state, bool force)
+static int call_on_cpu(int cpu, long (*fn)(void *), void *arg, bool direct)
+{
+	if (direct)
+		return fn(arg);
+	return work_on_cpu(cpu, fn, arg);
+}
+
+static int __acpi_processor_set_throttling(struct acpi_processor *pr,
+					   int state, bool force, bool direct)
 {
 	int ret = 0;
 	unsigned int i;
@@ -1125,7 +1133,8 @@ int acpi_processor_set_throttling(struct
 		arg.pr = pr;
 		arg.target_state = state;
 		arg.force = force;
-		ret = work_on_cpu(pr->id, acpi_processor_throttling_fn, &arg);
+		ret = call_on_cpu(pr->id, acpi_processor_throttling_fn, &arg,
+				  direct);
 	} else {
 		/*
 		 * When the T-state coordination is SW_ALL or HW_ALL,
@@ -1158,8 +1167,8 @@ int acpi_processor_set_throttling(struct
 			arg.pr = match_pr;
 			arg.target_state = state;
 			arg.force = force;
-			ret = work_on_cpu(pr->id, acpi_processor_throttling_fn,
-				&arg);
+			ret = call_on_cpu(pr->id, acpi_processor_throttling_fn,
+					  &arg, direct);
 		}
 	}
 	/*
@@ -1177,6 +1186,12 @@ int acpi_processor_set_throttling(struct
 	return ret;
 }
 
+int acpi_processor_set_throttling(struct acpi_processor *pr, int state,
+				  bool force)
+{
+	return __acpi_processor_set_throttling(pr, state, force, false);
+}
+
 int acpi_processor_get_throttling_info(struct acpi_processor *pr)
 {
 	int result = 0;

[toc] | [prev] | [next] | [standalone]

#1623002 — Re: [patch 08/13] ACPI/processor: Replace racy task affinity logic.

On Thu, Apr 13, 2017 at 02:01:42PM +0200, Thomas Gleixner wrote:
> On Thu, 13 Apr 2017, Peter Zijlstra wrote:
> > 
> > That makes my machine sad...
> > [    9.786610]  work_on_cpu+0x82/0x90
> > [    9.790404]  ? __usermodehelper_disable+0x110/0x110
> > [    9.795846]  ? __acpi_processor_get_throttling+0x20/0x20
> > [    9.801773]  acpi_processor_set_throttling+0x199/0x220
> > [    9.807506]  ? trace_hardirqs_on_caller+0xfb/0x1d0
> > [    9.812851]  acpi_processor_get_throttling_ptc+0xec/0x180
> > [    9.818876]  __acpi_processor_get_throttling+0xf/0x20
> > [    9.824511]  work_for_cpu_fn+0x14/0x20
> > [    9.828692]  process_one_work+0x261/0x670
> > [    9.833165]  worker_thread+0x21b/0x3f0
> > [    9.837348]  kthread+0x108/0x140
> > [    9.840947]  ? process_one_work+0x670/0x670
> > [    9.845611]  ? kthread_create_on_node+0x40/0x40
> > [    9.850667]  ret_from_fork+0x31/0x40
> 
> Yuck. So the call chain is:
> 
> acpi_processor_get_throttling()
>   work_on_cpu(acpi_processor_get_throttling)
> 
> That work does:
> 
> __acpi_processor_get_throttling()
>     acpi_processor_get_throttling_ptc()
>       acpi_processor_set_throttling()
>         work_on_cpu(__acpi_processor_set_throttling)
> 
> Why the heck calls a get_throttling() function set_throttling()? I'm mildly
> surprised.
> 
> Does the delta patch below cure the problem?

Yes, aside from a compile warn.

But that's disgusting... then again, you merely reintroduced this hack.

[toc] | [prev] | [next] | [standalone]

#1624075 — [tip:sched/core] ACPI/processor: Replace racy task affinity logic

Commit-ID:  8153f9ac43897f9f4786b30badc134fcc1a4fb11
Gitweb:     http://git.kernel.org/tip/8153f9ac43897f9f4786b30badc134fcc1a4fb11
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 12 Apr 2017 22:07:34 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 15 Apr 2017 12:20:54 +0200

ACPI/processor: Replace racy task affinity logic

acpi_processor_get_throttling() requires to invoke the getter function on
the target CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. CPU hotplug and concurrent affinity settings for that
thread resulting in code executing on the wrong CPU and overwriting the
new affinity setting.

acpi_processor_get_throttling() is invoked in two ways:

1) The CPU online callback, which is already running on the target CPU and
   obviously protected against hotplug and not affected by affinity
   settings.

2) The ACPI driver probe function, which is not protected against hotplug
   during modprobe.

Switch it over to work_on_cpu() and protect the probe function against CPU
hotplug.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: linux-acpi@vger.kernel.org
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Tejun Heo <tj@kernel.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Len Brown <lenb@kernel.org>
Link: http://lkml.kernel.org/r/20170412201042.785920903@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/acpi/processor_driver.c     |  7 ++++-
 drivers/acpi/processor_throttling.c | 62 +++++++++++++++++++++----------------
 2 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index eab8cda..8697a82 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -262,11 +262,16 @@ err_power_exit:
 static int acpi_processor_start(struct device *dev)
 {
 	struct acpi_device *device = ACPI_COMPANION(dev);
+	int ret;
 
 	if (!device)
 		return -ENODEV;
 
-	return __acpi_processor_start(device);
+	/* Protect against concurrent CPU hotplug operations */
+	get_online_cpus();
+	ret = __acpi_processor_start(device);
+	put_online_cpus();
+	return ret;
 }
 
 static int acpi_processor_stop(struct device *dev)
diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
index a12f96c..3de34633 100644
--- a/drivers/acpi/processor_throttling.c
+++ b/drivers/acpi/processor_throttling.c
@@ -62,8 +62,8 @@ struct acpi_processor_throttling_arg {
 #define THROTTLING_POSTCHANGE      (2)
 
 static int acpi_processor_get_throttling(struct acpi_processor *pr);
-int acpi_processor_set_throttling(struct acpi_processor *pr,
-						int state, bool force);
+static int __acpi_processor_set_throttling(struct acpi_processor *pr,
+					   int state, bool force, bool direct);
 
 static int acpi_processor_update_tsd_coord(void)
 {
@@ -891,7 +891,8 @@ static int acpi_processor_get_throttling_ptc(struct acpi_processor *pr)
 			ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 				"Invalid throttling state, reset\n"));
 			state = 0;
-			ret = acpi_processor_set_throttling(pr, state, true);
+			ret = __acpi_processor_set_throttling(pr, state, true,
+							      true);
 			if (ret)
 				return ret;
 		}
@@ -901,36 +902,31 @@ static int acpi_processor_get_throttling_ptc(struct acpi_processor *pr)
 	return 0;
 }
 
-static int acpi_processor_get_throttling(struct acpi_processor *pr)
+static long __acpi_processor_get_throttling(void *data)
 {
-	cpumask_var_t saved_mask;
-	int ret;
+	struct acpi_processor *pr = data;
+
+	return pr->throttling.acpi_processor_get_throttling(pr);
+}
 
+static int acpi_processor_get_throttling(struct acpi_processor *pr)
+{
 	if (!pr)
 		return -EINVAL;
 
 	if (!pr->flags.throttling)
 		return -ENODEV;
 
-	if (!alloc_cpumask_var(&saved_mask, GFP_KERNEL))
-		return -ENOMEM;
-
 	/*
-	 * Migrate task to the cpu pointed by pr.
+	 * This is either called from the CPU hotplug callback of
+	 * processor_driver or via the ACPI probe function. In the latter
+	 * case the CPU is not guaranteed to be online. Both call sites are
+	 * protected against CPU hotplug.
 	 */
-	cpumask_copy(saved_mask, &current->cpus_allowed);
-	/* FIXME: use work_on_cpu() */
-	if (set_cpus_allowed_ptr(current, cpumask_of(pr->id))) {
-		/* Can't migrate to the target pr->id CPU. Exit */
-		free_cpumask_var(saved_mask);
+	if (!cpu_online(pr->id))
 		return -ENODEV;
-	}
-	ret = pr->throttling.acpi_processor_get_throttling(pr);
-	/* restore the previous state */
-	set_cpus_allowed_ptr(current, saved_mask);
-	free_cpumask_var(saved_mask);
 
-	return ret;
+	return work_on_cpu(pr->id, __acpi_processor_get_throttling, pr);
 }
 
 static int acpi_processor_get_fadt_info(struct acpi_processor *pr)
@@ -1080,8 +1076,15 @@ static long acpi_processor_throttling_fn(void *data)
 			arg->target_state, arg->force);
 }
 
-int acpi_processor_set_throttling(struct acpi_processor *pr,
-						int state, bool force)
+static int call_on_cpu(int cpu, long (*fn)(void *), void *arg, bool direct)
+{
+	if (direct)
+		return fn(arg);
+	return work_on_cpu(cpu, fn, arg);
+}
+
+static int __acpi_processor_set_throttling(struct acpi_processor *pr,
+					   int state, bool force, bool direct)
 {
 	int ret = 0;
 	unsigned int i;
@@ -1130,7 +1133,8 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 		arg.pr = pr;
 		arg.target_state = state;
 		arg.force = force;
-		ret = work_on_cpu(pr->id, acpi_processor_throttling_fn, &arg);
+		ret = call_on_cpu(pr->id, acpi_processor_throttling_fn, &arg,
+				  direct);
 	} else {
 		/*
 		 * When the T-state coordination is SW_ALL or HW_ALL,
@@ -1163,8 +1167,8 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 			arg.pr = match_pr;
 			arg.target_state = state;
 			arg.force = force;
-			ret = work_on_cpu(pr->id, acpi_processor_throttling_fn,
-				&arg);
+			ret = call_on_cpu(pr->id, acpi_processor_throttling_fn,
+					  &arg, direct);
 		}
 	}
 	/*
@@ -1182,6 +1186,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
 	return ret;
 }
 
+int acpi_processor_set_throttling(struct acpi_processor *pr, int state,
+				  bool force)
+{
+	return __acpi_processor_set_throttling(pr, state, force, false);
+}
+
 int acpi_processor_get_throttling_info(struct acpi_processor *pr)
 {
 	int result = 0;

[toc] | [prev] | [next] | [standalone]

#1622531 — [patch 13/13] crypto: n2 - Replace racy task affinity logic

spu_queue_register() needs to invoke setup functions on a particular
CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. CPU hotplug and concurrent affinity settings for that
thread resulting in code executing on the wrong CPU and overwriting the
new affinity setting.

Replace it by using work_on_cpu_safe() which guarantees to run the code on
the requested CPU or to fail in case the CPU is offline.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
---
 drivers/crypto/n2_core.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

--- a/drivers/crypto/n2_core.c
+++ b/drivers/crypto/n2_core.c
@@ -65,6 +65,11 @@ struct spu_queue {
 	struct list_head	list;
 };
 
+struct spu_qreg {
+	struct spu_queue	*queue;
+	unsigned long		type;
+};
+
 static struct spu_queue **cpu_to_cwq;
 static struct spu_queue **cpu_to_mau;
 
@@ -1631,31 +1636,27 @@ static void queue_cache_destroy(void)
 	kmem_cache_destroy(queue_cache[HV_NCS_QTYPE_CWQ - 1]);
 }
 
-static int spu_queue_register(struct spu_queue *p, unsigned long q_type)
+static long spu_queue_register_workfn(void *arg)
 {
-	cpumask_var_t old_allowed;
+	struct spu_qreg *qr = arg;
+	struct spu_queue *p = qr->queue;
+	unsigned long q_type = qr->type;
 	unsigned long hv_ret;
 
-	if (cpumask_empty(&p->sharing))
-		return -EINVAL;
-
-	if (!alloc_cpumask_var(&old_allowed, GFP_KERNEL))
-		return -ENOMEM;
-
-	cpumask_copy(old_allowed, &current->cpus_allowed);
-
-	set_cpus_allowed_ptr(current, &p->sharing);
-
 	hv_ret = sun4v_ncs_qconf(q_type, __pa(p->q),
 				 CWQ_NUM_ENTRIES, &p->qhandle);
 	if (!hv_ret)
 		sun4v_ncs_sethead_marker(p->qhandle, 0);
 
-	set_cpus_allowed_ptr(current, old_allowed);
+	return hv_ret ? -EINVAL : 0;
+}
 
-	free_cpumask_var(old_allowed);
+static int spu_queue_register(struct spu_queue *p, unsigned long q_type)
+{
+	int cpu = cpumask_any_and(&p->sharing, cpu_online_mask);
+	struct spu_qreg qr = { .queue = p, .type = q_type };
 
-	return (hv_ret ? -EINVAL : 0);
+	return work_on_cpu_safe(cpu, &qr);
 }
 
 static int spu_queue_setup(struct spu_queue *p)

[toc] | [prev] | [next] | [standalone]

#1622721 — Re: [patch 13/13] crypto: n2 - Replace racy task affinity logic

On Wed, Apr 12, 2017 at 10:07:39PM +0200, Thomas Gleixner wrote:
> spu_queue_register() needs to invoke setup functions on a particular
> CPU. This is achieved by temporarily setting the affinity of the
> calling user space thread to the requested CPU and reset it to the original
> affinity afterwards.
> 
> That's racy vs. CPU hotplug and concurrent affinity settings for that
> thread resulting in code executing on the wrong CPU and overwriting the
> new affinity setting.
> 
> Replace it by using work_on_cpu_safe() which guarantees to run the code on
> the requested CPU or to fail in case the CPU is offline.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: linux-crypto@vger.kernel.org

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[toc] | [prev] | [next] | [standalone]

#1622815 — [patch V2 13/13] crypto: n2 - Replace racy task affinity logic

spu_queue_register() needs to invoke setup functions on a particular
CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. CPU hotplug and concurrent affinity settings for that
thread resulting in code executing on the wrong CPU and overwriting the
new affinity setting.

Replace it by using work_on_cpu_safe() which guarantees to run the code on
the requested CPU or to fail in case the CPU is offline.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-crypto@vger.kernel.org
---

V2: Fixup build-bot complaints

 drivers/crypto/n2_core.c |   31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

--- a/drivers/crypto/n2_core.c
+++ b/drivers/crypto/n2_core.c
@@ -65,6 +65,11 @@ struct spu_queue {
 	struct list_head	list;
 };
 
+struct spu_qreg {
+	struct spu_queue	*queue;
+	unsigned long		type;
+};
+
 static struct spu_queue **cpu_to_cwq;
 static struct spu_queue **cpu_to_mau;
 
@@ -1631,31 +1636,27 @@ static void queue_cache_destroy(void)
 	kmem_cache_destroy(queue_cache[HV_NCS_QTYPE_CWQ - 1]);
 }
 
-static int spu_queue_register(struct spu_queue *p, unsigned long q_type)
+static long spu_queue_register_workfn(void *arg)
 {
-	cpumask_var_t old_allowed;
+	struct spu_qreg *qr = arg;
+	struct spu_queue *p = qr->queue;
+	unsigned long q_type = qr->type;
 	unsigned long hv_ret;
 
-	if (cpumask_empty(&p->sharing))
-		return -EINVAL;
-
-	if (!alloc_cpumask_var(&old_allowed, GFP_KERNEL))
-		return -ENOMEM;
-
-	cpumask_copy(old_allowed, &current->cpus_allowed);
-
-	set_cpus_allowed_ptr(current, &p->sharing);
-
 	hv_ret = sun4v_ncs_qconf(q_type, __pa(p->q),
 				 CWQ_NUM_ENTRIES, &p->qhandle);
 	if (!hv_ret)
 		sun4v_ncs_sethead_marker(p->qhandle, 0);
 
-	set_cpus_allowed_ptr(current, old_allowed);
+	return hv_ret ? -EINVAL : 0;
+}
 
-	free_cpumask_var(old_allowed);
+static int spu_queue_register(struct spu_queue *p, unsigned long q_type)
+{
+	int cpu = cpumask_any_and(&p->sharing, cpu_online_mask);
+	struct spu_qreg qr = { .queue = p, .type = q_type };
 
-	return (hv_ret ? -EINVAL : 0);
+	return work_on_cpu_safe(cpu, spu_queue_register_workfn, &qr);
 }
 
 static int spu_queue_setup(struct spu_queue *p)

[toc] | [prev] | [next] | [standalone]

#1623096 — Re: [patch V2 13/13] crypto: n2 - Replace racy task affinity logic

From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 13 Apr 2017 10:20:23 +0200 (CEST)

> spu_queue_register() needs to invoke setup functions on a particular
> CPU. This is achieved by temporarily setting the affinity of the
> calling user space thread to the requested CPU and reset it to the original
> affinity afterwards.
> 
> That's racy vs. CPU hotplug and concurrent affinity settings for that
> thread resulting in code executing on the wrong CPU and overwriting the
> new affinity setting.
> 
> Replace it by using work_on_cpu_safe() which guarantees to run the code on
> the requested CPU or to fail in case the CPU is offline.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: David S. Miller <davem@davemloft.net>

[toc] | [prev] | [next] | [standalone]

#1624078 — [tip:sched/core] crypto: N2 - Replace racy task affinity logic

Commit-ID:  73810a069120aa831debb4d967310ab900f628ad
Gitweb:     http://git.kernel.org/tip/73810a069120aa831debb4d967310ab900f628ad
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 13 Apr 2017 10:20:23 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 15 Apr 2017 12:20:56 +0200

crypto: N2 - Replace racy task affinity logic

spu_queue_register() needs to invoke setup functions on a particular
CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. CPU hotplug and concurrent affinity settings for that
thread resulting in code executing on the wrong CPU and overwriting the
new affinity setting.

Replace it by using work_on_cpu_safe() which guarantees to run the code on
the requested CPU or to fail in case the CPU is offline.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Herbert Xu <herbert@gondor.apana.org.au>
Acked-by: "David S. Miller" <davem@davemloft.net>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-crypto@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Tejun Heo <tj@kernel.org>
Cc: Len Brown <lenb@kernel.org>
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1704131019420.2408@nanos
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 drivers/crypto/n2_core.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/crypto/n2_core.c b/drivers/crypto/n2_core.c
index c5aac25..4ecb77a 100644
--- a/drivers/crypto/n2_core.c
+++ b/drivers/crypto/n2_core.c
@@ -65,6 +65,11 @@ struct spu_queue {
 	struct list_head	list;
 };
 
+struct spu_qreg {
+	struct spu_queue	*queue;
+	unsigned long		type;
+};
+
 static struct spu_queue **cpu_to_cwq;
 static struct spu_queue **cpu_to_mau;
 
@@ -1631,31 +1636,27 @@ static void queue_cache_destroy(void)
 	kmem_cache_destroy(queue_cache[HV_NCS_QTYPE_CWQ - 1]);
 }
 
-static int spu_queue_register(struct spu_queue *p, unsigned long q_type)
+static long spu_queue_register_workfn(void *arg)
 {
-	cpumask_var_t old_allowed;
+	struct spu_qreg *qr = arg;
+	struct spu_queue *p = qr->queue;
+	unsigned long q_type = qr->type;
 	unsigned long hv_ret;
 
-	if (cpumask_empty(&p->sharing))
-		return -EINVAL;
-
-	if (!alloc_cpumask_var(&old_allowed, GFP_KERNEL))
-		return -ENOMEM;
-
-	cpumask_copy(old_allowed, &current->cpus_allowed);
-
-	set_cpus_allowed_ptr(current, &p->sharing);
-
 	hv_ret = sun4v_ncs_qconf(q_type, __pa(p->q),
 				 CWQ_NUM_ENTRIES, &p->qhandle);
 	if (!hv_ret)
 		sun4v_ncs_sethead_marker(p->qhandle, 0);
 
-	set_cpus_allowed_ptr(current, old_allowed);
+	return hv_ret ? -EINVAL : 0;
+}
 
-	free_cpumask_var(old_allowed);
+static int spu_queue_register(struct spu_queue *p, unsigned long q_type)
+{
+	int cpu = cpumask_any_and(&p->sharing, cpu_online_mask);
+	struct spu_qreg qr = { .queue = p, .type = q_type };
 
-	return (hv_ret ? -EINVAL : 0);
+	return work_on_cpu_safe(cpu, spu_queue_register_workfn, &qr);
 }
 
 static int spu_queue_setup(struct spu_queue *p)

[toc] | [prev] | [next] | [standalone]

#1622532 — [patch 05/13] powerpc/smp: Replace open coded task affinity logic

Init task invokes smp_ops->setup_cpu() from smp_cpus_done(). Init task can
run on any online CPU at this point, but the setup_cpu() callback requires
to be invoked on the boot CPU. This is achieved by temporarily setting the
affinity of the calling user space thread to the requested CPU and reset it
to the original affinity afterwards.

That's racy vs. CPU hotplug and concurrent affinity settings for that
thread resulting in code executing on the wrong CPU and overwriting the
new affinity setting.

That's actually not a problem in this context as neither CPU hotplug nor
affinity settings can happen, but the access to task_struct::cpus_allowed
is about to restricted.

Replace it with a call to work_on_cpu_safe() which achieves the same result.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
---
 arch/powerpc/kernel/smp.c |   26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -787,24 +787,21 @@ static struct sched_domain_topology_leve
 	{ NULL, },
 };
 
-void __init smp_cpus_done(unsigned int max_cpus)
+static __init long smp_setup_cpu_workfn(void *data __always_unused)
 {
-	cpumask_var_t old_mask;
+	smp_ops->setup_cpu(boot_cpuid);
+	return 0;
+}
 
-	/* We want the setup_cpu() here to be called from CPU 0, but our
-	 * init thread may have been "borrowed" by another CPU in the meantime
-	 * se we pin us down to CPU 0 for a short while
+void __init smp_cpus_done(unsigned int max_cpus)
+{
+	/*
+	 * We want the setup_cpu() here to be called on the boot CPU, but
+	 * init might run on any CPU, so make sure it's invoked on the boot
+	 * CPU.
 	 */
-	alloc_cpumask_var(&old_mask, GFP_NOWAIT);
-	cpumask_copy(old_mask, &current->cpus_allowed);
-	set_cpus_allowed_ptr(current, cpumask_of(boot_cpuid));
-	
 	if (smp_ops && smp_ops->setup_cpu)
-		smp_ops->setup_cpu(boot_cpuid);
-
-	set_cpus_allowed_ptr(current, old_mask);
-
-	free_cpumask_var(old_mask);
+		work_on_cpu_safe(boot_cpuid, smp_setup_cpu_workfn, NULL);
 
 	if (smp_ops && smp_ops->bringup_done)
 		smp_ops->bringup_done();
@@ -812,7 +809,6 @@ void __init smp_cpus_done(unsigned int m
 	dump_numa_cpu_topology();
 
 	set_sched_topology(powerpc_topology);
-
 }
 
 #ifdef CONFIG_HOTPLUG_CPU

[toc] | [prev] | [next] | [standalone]

#1622726 — Re: [patch 05/13] powerpc/smp: Replace open coded task affinity logic

Thomas Gleixner <tglx@linutronix.de> writes:

> Init task invokes smp_ops->setup_cpu() from smp_cpus_done(). Init task can
> run on any online CPU at this point, but the setup_cpu() callback requires
> to be invoked on the boot CPU. This is achieved by temporarily setting the
> affinity of the calling user space thread to the requested CPU and reset it
> to the original affinity afterwards.
>
> That's racy vs. CPU hotplug and concurrent affinity settings for that
> thread resulting in code executing on the wrong CPU and overwriting the
> new affinity setting.
>
> That's actually not a problem in this context as neither CPU hotplug nor
> affinity settings can happen, but the access to task_struct::cpus_allowed
> is about to restricted.
>
> Replace it with a call to work_on_cpu_safe() which achieves the same result.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
>  arch/powerpc/kernel/smp.c |   26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)

LGTM.

Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)

cheers

[toc] | [prev] | [next] | [standalone]

#1624076 — [tip:sched/core] powerpc/smp: Replace open coded task affinity logic

Commit-ID:  6d11b87d55eb75007a3721c2de5938f5bbf607fb
Gitweb:     http://git.kernel.org/tip/6d11b87d55eb75007a3721c2de5938f5bbf607fb
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Wed, 12 Apr 2017 22:07:31 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 15 Apr 2017 12:20:54 +0200

powerpc/smp: Replace open coded task affinity logic

Init task invokes smp_ops->setup_cpu() from smp_cpus_done(). Init task can
run on any online CPU at this point, but the setup_cpu() callback requires
to be invoked on the boot CPU. This is achieved by temporarily setting the
affinity of the calling user space thread to the requested CPU and reset it
to the original affinity afterwards.

That's racy vs. CPU hotplug and concurrent affinity settings for that
thread resulting in code executing on the wrong CPU and overwriting the
new affinity setting.

That's actually not a problem in this context as neither CPU hotplug nor
affinity settings can happen, but the access to task_struct::cpus_allowed
is about to restricted.

Replace it with a call to work_on_cpu_safe() which achieves the same result.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Len Brown <lenb@kernel.org>
Link: http://lkml.kernel.org/r/20170412201042.518053336@linutronix.de
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/powerpc/kernel/smp.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 46f89e6..d68ed1f 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -787,24 +787,21 @@ static struct sched_domain_topology_level powerpc_topology[] = {
 	{ NULL, },
 };
 
-void __init smp_cpus_done(unsigned int max_cpus)
+static __init long smp_setup_cpu_workfn(void *data __always_unused)
 {
-	cpumask_var_t old_mask;
+	smp_ops->setup_cpu(boot_cpuid);
+	return 0;
+}
 
-	/* We want the setup_cpu() here to be called from CPU 0, but our
-	 * init thread may have been "borrowed" by another CPU in the meantime
-	 * se we pin us down to CPU 0 for a short while
+void __init smp_cpus_done(unsigned int max_cpus)
+{
+	/*
+	 * We want the setup_cpu() here to be called on the boot CPU, but
+	 * init might run on any CPU, so make sure it's invoked on the boot
+	 * CPU.
 	 */
-	alloc_cpumask_var(&old_mask, GFP_NOWAIT);
-	cpumask_copy(old_mask, &current->cpus_allowed);
-	set_cpus_allowed_ptr(current, cpumask_of(boot_cpuid));
-	
 	if (smp_ops && smp_ops->setup_cpu)
-		smp_ops->setup_cpu(boot_cpuid);
-
-	set_cpus_allowed_ptr(current, old_mask);
-
-	free_cpumask_var(old_mask);
+		work_on_cpu_safe(boot_cpuid, smp_setup_cpu_workfn, NULL);
 
 	if (smp_ops && smp_ops->bringup_done)
 		smp_ops->bringup_done();
@@ -812,7 +809,6 @@ void __init smp_cpus_done(unsigned int max_cpus)
 	dump_numa_cpu_topology();
 
 	set_sched_topology(powerpc_topology);
-
 }
 
 #ifdef CONFIG_HOTPLUG_CPU

[toc] | [prev] | [next] | [standalone]

#1622533 — [patch 06/13] sparc/sysfs: Replace racy task affinity logic

The mmustat_enable sysfs file accessor functions must run code on the
target CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. concurrent affinity settings for that thread resulting in
code executing on the wrong CPU and overwriting the new affinity setting.

Replace it by using work_on_cpu() which guarantees to run the code on the
requested CPU.

Protection against CPU hotplug is not required as the open sysfs file
already prevents the removal from the CPU offline callback. Using the
hotplug protected version would actually be wrong because it would deadlock
against a CPU hotplug operation of the CPU associated to the sysfs file in
progress.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
---
 arch/sparc/kernel/sysfs.c |   36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

--- a/arch/sparc/kernel/sysfs.c
+++ b/arch/sparc/kernel/sysfs.c
@@ -98,27 +98,7 @@ static struct attribute_group mmu_stat_g
 	.name = "mmu_stats",
 };
 
-/* XXX convert to rusty's on_one_cpu */
-static unsigned long run_on_cpu(unsigned long cpu,
-			        unsigned long (*func)(unsigned long),
-				unsigned long arg)
-{
-	cpumask_t old_affinity;
-	unsigned long ret;
-
-	cpumask_copy(&old_affinity, &current->cpus_allowed);
-	/* should return -EINVAL to userspace */
-	if (set_cpus_allowed_ptr(current, cpumask_of(cpu)))
-		return 0;
-
-	ret = func(arg);
-
-	set_cpus_allowed_ptr(current, &old_affinity);
-
-	return ret;
-}
-
-static unsigned long read_mmustat_enable(unsigned long junk)
+static long read_mmustat_enable(void *data __maybe_unused)
 {
 	unsigned long ra = 0;
 
@@ -127,11 +107,11 @@ static unsigned long read_mmustat_enable
 	return ra != 0;
 }
 
-static unsigned long write_mmustat_enable(unsigned long val)
+static long write_mmustat_enable(void *data)
 {
-	unsigned long ra, orig_ra;
+	unsigned long ra, orig_ra, *val = data;
 
-	if (val)
+	if (*val)
 		ra = __pa(&per_cpu(mmu_stats, smp_processor_id()));
 	else
 		ra = 0UL;
@@ -142,7 +122,8 @@ static unsigned long write_mmustat_enabl
 static ssize_t show_mmustat_enable(struct device *s,
 				struct device_attribute *attr, char *buf)
 {
-	unsigned long val = run_on_cpu(s->id, read_mmustat_enable, 0);
+	long val = work_on_cpu(s->id, read_mmustat_enable, 0);
+
 	return sprintf(buf, "%lx\n", val);
 }
 
@@ -150,13 +131,14 @@ static ssize_t store_mmustat_enable(stru
 			struct device_attribute *attr, const char *buf,
 			size_t count)
 {
-	unsigned long val, err;
 	int ret = sscanf(buf, "%lu", &val);
+	unsigned long val;
+	long err;
 
 	if (ret != 1)
 		return -EINVAL;
 
-	err = run_on_cpu(s->id, write_mmustat_enable, val);
+	err = work_on_cpu(s->id, write_mmustat_enable, &val);
 	if (err)
 		return -EIO;
 

[toc] | [prev] | [next] | [standalone]

#1622662 — Re: [patch 06/13] sparc/sysfs: Replace racy task affinity logic

From: Thomas Gleixner <tglx@linutronix.de>
Date: Wed, 12 Apr 2017 22:07:32 +0200

> @@ -142,7 +122,8 @@ static unsigned long write_mmustat_enabl
>  static ssize_t show_mmustat_enable(struct device *s,
>  				struct device_attribute *attr, char *buf)
>  {
> -	unsigned long val = run_on_cpu(s->id, read_mmustat_enable, 0);
> +	long val = work_on_cpu(s->id, read_mmustat_enable, 0);

Last argument to work_on_cpu() should be NULL since it's a pointer
I guess.

But otherwise I have no problems with this:

Acked-by: David S. Miller <davem@davemloft.net>

[toc] | [prev] | [next] | [standalone]

#1622807 — [patch V2 06/13] sparc/sysfs: Replace racy task affinity logic

The mmustat_enable sysfs file accessor functions must run code on the
target CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. concurrent affinity settings for that thread resulting in
code executing on the wrong CPU and overwriting the new affinity setting.

Replace it by using work_on_cpu() which guarantees to run the code on the
requested CPU.

Protection against CPU hotplug is not required as the open sysfs file
already prevents the removal from the CPU offline callback. Using the
hotplug protected version would actually be wrong because it would deadlock
against a CPU hotplug operation of the CPU associated to the sysfs file in
progress.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: David S. Miller <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
---

V2: Use NULL instead of 0 (David), fix compile breakage (build-bot)

 arch/sparc/kernel/sysfs.c |   39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

Index: b/arch/sparc/kernel/sysfs.c
===================================================================
--- a/arch/sparc/kernel/sysfs.c
+++ b/arch/sparc/kernel/sysfs.c
@@ -98,27 +98,7 @@ static struct attribute_group mmu_stat_g
 	.name = "mmu_stats",
 };
 
-/* XXX convert to rusty's on_one_cpu */
-static unsigned long run_on_cpu(unsigned long cpu,
-			        unsigned long (*func)(unsigned long),
-				unsigned long arg)
-{
-	cpumask_t old_affinity;
-	unsigned long ret;
-
-	cpumask_copy(&old_affinity, &current->cpus_allowed);
-	/* should return -EINVAL to userspace */
-	if (set_cpus_allowed_ptr(current, cpumask_of(cpu)))
-		return 0;
-
-	ret = func(arg);
-
-	set_cpus_allowed_ptr(current, &old_affinity);
-
-	return ret;
-}
-
-static unsigned long read_mmustat_enable(unsigned long junk)
+static long read_mmustat_enable(void *data __maybe_unused)
 {
 	unsigned long ra = 0;
 
@@ -127,11 +107,11 @@ static unsigned long read_mmustat_enable
 	return ra != 0;
 }
 
-static unsigned long write_mmustat_enable(unsigned long val)
+static long write_mmustat_enable(void *data)
 {
-	unsigned long ra, orig_ra;
+	unsigned long ra, orig_ra, *val = data;
 
-	if (val)
+	if (*val)
 		ra = __pa(&per_cpu(mmu_stats, smp_processor_id()));
 	else
 		ra = 0UL;
@@ -142,7 +122,8 @@ static unsigned long write_mmustat_enabl
 static ssize_t show_mmustat_enable(struct device *s,
 				struct device_attribute *attr, char *buf)
 {
-	unsigned long val = run_on_cpu(s->id, read_mmustat_enable, 0);
+	long val = work_on_cpu(s->id, read_mmustat_enable, NULL);
+
 	return sprintf(buf, "%lx\n", val);
 }
 
@@ -150,13 +131,15 @@ static ssize_t store_mmustat_enable(stru
 			struct device_attribute *attr, const char *buf,
 			size_t count)
 {
-	unsigned long val, err;
-	int ret = sscanf(buf, "%lu", &val);
+	unsigned long val;
+	long err;
+	int ret;
 
+	ret = sscanf(buf, "%lu", &val);
 	if (ret != 1)
 		return -EINVAL;
 
-	err = run_on_cpu(s->id, write_mmustat_enable, val);
+	err = work_on_cpu(s->id, write_mmustat_enable, &val);
 	if (err)
 		return -EIO;
 

[toc] | [prev] | [next] | [standalone]

#1624063 — [tip:sched/core] sparc/sysfs: Replace racy task affinity logic

Commit-ID:  ea875ec94eafb858990f3fe9528501f983105653
Gitweb:     http://git.kernel.org/tip/ea875ec94eafb858990f3fe9528501f983105653
Author:     Thomas Gleixner <tglx@linutronix.de>
AuthorDate: Thu, 13 Apr 2017 10:17:07 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sat, 15 Apr 2017 12:20:54 +0200

sparc/sysfs: Replace racy task affinity logic

The mmustat_enable sysfs file accessor functions must run code on the
target CPU. This is achieved by temporarily setting the affinity of the
calling user space thread to the requested CPU and reset it to the original
affinity afterwards.

That's racy vs. concurrent affinity settings for that thread resulting in
code executing on the wrong CPU and overwriting the new affinity setting.

Replace it by using work_on_cpu() which guarantees to run the code on the
requested CPU.

Protection against CPU hotplug is not required as the open sysfs file
already prevents the removal from the CPU offline callback. Using the
hotplug protected version would actually be wrong because it would deadlock
against a CPU hotplug operation of the CPU associated to the sysfs file in
progress.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: David S. Miller <davem@davemloft.net>
Cc: fenghua.yu@intel.com
Cc: tony.luck@intel.com
Cc: herbert@gondor.apana.org.au
Cc: rjw@rjwysocki.net
Cc: peterz@infradead.org
Cc: benh@kernel.crashing.org
Cc: bigeasy@linutronix.de
Cc: jiangshanlai@gmail.com
Cc: sparclinux@vger.kernel.org
Cc: viresh.kumar@linaro.org
Cc: mpe@ellerman.id.au
Cc: tj@kernel.org
Cc: lenb@kernel.org
Link: http://lkml.kernel.org/r/alpine.DEB.2.20.1704131001270.2408@nanos
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

---
 arch/sparc/kernel/sysfs.c | 39 +++++++++++----------------------------
 1 file changed, 11 insertions(+), 28 deletions(-)

diff --git a/arch/sparc/kernel/sysfs.c b/arch/sparc/kernel/sysfs.c
index d63fc61..5fd352b 100644
--- a/arch/sparc/kernel/sysfs.c
+++ b/arch/sparc/kernel/sysfs.c
@@ -98,27 +98,7 @@ static struct attribute_group mmu_stat_group = {
 	.name = "mmu_stats",
 };
 
-/* XXX convert to rusty's on_one_cpu */
-static unsigned long run_on_cpu(unsigned long cpu,
-			        unsigned long (*func)(unsigned long),
-				unsigned long arg)
-{
-	cpumask_t old_affinity;
-	unsigned long ret;
-
-	cpumask_copy(&old_affinity, &current->cpus_allowed);
-	/* should return -EINVAL to userspace */
-	if (set_cpus_allowed_ptr(current, cpumask_of(cpu)))
-		return 0;
-
-	ret = func(arg);
-
-	set_cpus_allowed_ptr(current, &old_affinity);
-
-	return ret;
-}
-
-static unsigned long read_mmustat_enable(unsigned long junk)
+static long read_mmustat_enable(void *data __maybe_unused)
 {
 	unsigned long ra = 0;
 
@@ -127,11 +107,11 @@ static unsigned long read_mmustat_enable(unsigned long junk)
 	return ra != 0;
 }
 
-static unsigned long write_mmustat_enable(unsigned long val)
+static long write_mmustat_enable(void *data)
 {
-	unsigned long ra, orig_ra;
+	unsigned long ra, orig_ra, *val = data;
 
-	if (val)
+	if (*val)
 		ra = __pa(&per_cpu(mmu_stats, smp_processor_id()));
 	else
 		ra = 0UL;
@@ -142,7 +122,8 @@ static unsigned long write_mmustat_enable(unsigned long val)
 static ssize_t show_mmustat_enable(struct device *s,
 				struct device_attribute *attr, char *buf)
 {
-	unsigned long val = run_on_cpu(s->id, read_mmustat_enable, 0);
+	long val = work_on_cpu(s->id, read_mmustat_enable, NULL);
+
 	return sprintf(buf, "%lx\n", val);
 }
 
@@ -150,13 +131,15 @@ static ssize_t store_mmustat_enable(struct device *s,
 			struct device_attribute *attr, const char *buf,
 			size_t count)
 {
-	unsigned long val, err;
-	int ret = sscanf(buf, "%lu", &val);
+	unsigned long val;
+	long err;
+	int ret;
 
+	ret = sscanf(buf, "%lu", &val);
 	if (ret != 1)
 		return -EINVAL;
 
-	err = run_on_cpu(s->id, write_mmustat_enable, val);
+	err = work_on_cpu(s->id, write_mmustat_enable, &val);
 	if (err)
 		return -EIO;
 

[toc] | [prev] | [next] | [standalone]

#1622534 — [patch 02/13] workqueue: Provide work_on_cpu_safe()

work_on_cpu() is not protected against CPU hotplug. For code which requires
to be either executed on an online CPU or to fail if the CPU is not
available the callsite would have to protect against CPU hotplug.

Provide a function which does get/put_online_cpus() around the call to
work_on_cpu() and fails the call with -ENODEV if the target CPU is not
online.

Preparatory patch to convert several racy task affinity manipulations.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Tejun Heo <tj@kernel.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
---
 include/linux/workqueue.h |    5 +++++
 kernel/workqueue.c        |   23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+)

--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -608,8 +608,13 @@ static inline long work_on_cpu(int cpu,
 {
 	return fn(arg);
 }
+static inline long work_on_cpu_safe(int cpu, long (*fn)(void *), void *arg)
+{
+	return fn(arg);
+}
 #else
 long work_on_cpu(int cpu, long (*fn)(void *), void *arg);
+long work_on_cpu_safe(int cpu, long (*fn)(void *), void *arg);
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_FREEZER
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -4735,6 +4735,29 @@ long work_on_cpu(int cpu, long (*fn)(voi
 	return wfc.ret;
 }
 EXPORT_SYMBOL_GPL(work_on_cpu);
+
+/**
+ * work_on_cpu_safe - run a function in thread context on a particular cpu
+ * @cpu: the cpu to run on
+ * @fn:  the function to run
+ * @arg: the function argument
+ *
+ * Disables CPU hotplug and calls work_on_cpu(). The caller must not hold
+ * any locks which would prevent @fn from completing.
+ *
+ * Return: The value @fn returns.
+ */
+long work_on_cpu_safe(int cpu, long (*fn)(void *), void *arg)
+{
+	long ret = -ENODEV;
+
+	get_online_cpus();
+	if (cpu_online(cpu))
+		ret = work_on_cpu(cpu, fn, arg);
+	put_online_cpus();
+	return ret;
+}
+EXPORT_SYMBOL_GPL(work_on_cpu_safe);
 #endif /* CONFIG_SMP */
 
 #ifdef CONFIG_FREEZER

[toc] | [prev] | [next] | [standalone]

#1622927 — Re: [patch 02/13] workqueue: Provide work_on_cpu_safe()

At 04/13/2017 04:07 AM, Thomas Gleixner wrote:
> work_on_cpu() is not protected against CPU hotplug. For code which requires
> to be either executed on an online CPU or to fail if the CPU is not
> available the callsite would have to protect against CPU hotplug.
>
> Provide a function which does get/put_online_cpus() around the call to
> work_on_cpu() and fails the call with -ENODEV if the target CPU is not
> online.
>
> Preparatory patch to convert several racy task affinity manipulations.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>
> ---
>  include/linux/workqueue.h |    5 +++++
>  kernel/workqueue.c        |   23 +++++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -608,8 +608,13 @@ static inline long work_on_cpu(int cpu,
>  {
>  	return fn(arg);
>  }
> +static inline long work_on_cpu_safe(int cpu, long (*fn)(void *), void *arg)
> +{
> +	return fn(arg);
> +}
>  #else
>  long work_on_cpu(int cpu, long (*fn)(void *), void *arg);
> +long work_on_cpu_safe(int cpu, long (*fn)(void *), void *arg);
>  #endif /* CONFIG_SMP */
>
>  #ifdef CONFIG_FREEZER
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -4735,6 +4735,29 @@ long work_on_cpu(int cpu, long (*fn)(voi
>  	return wfc.ret;
>  }
>  EXPORT_SYMBOL_GPL(work_on_cpu);
> +
> +/**

There is an extra '*' :)

Thanks,
	Liyang

> + * work_on_cpu_safe - run a function in thread context on a particular cpu
> + * @cpu: the cpu to run on
> + * @fn:  the function to run
> + * @arg: the function argument
> + *
> + * Disables CPU hotplug and calls work_on_cpu(). The caller must not hold
> + * any locks which would prevent @fn from completing.
> + *
> + * Return: The value @fn returns.
> + */
> +long work_on_cpu_safe(int cpu, long (*fn)(void *), void *arg)
> +{
> +	long ret = -ENODEV;
> +
> +	get_online_cpus();
> +	if (cpu_online(cpu))
> +		ret = work_on_cpu(cpu, fn, arg);
> +	put_online_cpus();
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(work_on_cpu_safe);
>  #endif /* CONFIG_SMP */
>
>  #ifdef CONFIG_FREEZER

[toc] | [prev] | [next] | [standalone]

#1623360 — Re: [patch 02/13] workqueue: Provide work_on_cpu_safe()

On Thu, 13 Apr 2017, Dou Liyang wrote:
> At 04/13/2017 04:07 AM, Thomas Gleixner wrote:
> > +
> > +/**
> 
> There is an extra '*' :)

On purpose. It's the begin of a kernel-doc format comment for interface
documentation.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1623500 — Re: [patch 02/13] workqueue: Provide work_on_cpu_safe()

On Wed, Apr 12, 2017 at 10:07:28PM +0200, Thomas Gleixner wrote:
> work_on_cpu() is not protected against CPU hotplug. For code which requires
> to be either executed on an online CPU or to fail if the CPU is not
> available the callsite would have to protect against CPU hotplug.
> 
> Provide a function which does get/put_online_cpus() around the call to
> work_on_cpu() and fails the call with -ENODEV if the target CPU is not
> online.
> 
> Preparatory patch to convert several racy task affinity manipulations.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Lai Jiangshan <jiangshanlai@gmail.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web