[PATCH] stop_machine: Make stop_one_cpu_nowait() return void

[PATCH] stop_machine: Make stop_one_cpu_nowait() return void

[Yury Norov]

No caller checks the return value from stop_one_cpu_nowait(). All
callers require the callback to run and arrange for the target CPU's
stopper to remain enabled while queuing the work. In particular, commit
f0498d2a54e7 ("sched: Fix stop_one_cpu_nowait() vs hotplug") added
preemption protection to the scheduler callers so that queuing must
succeed once the target CPU has been observed online.

Therefore, a failure is an unrecoverable violation rather than a condition
individual callers can recover from. Diagnose it with WARN_ON_ONCE() in
stop_one_cpu_nowait(). A check in the common helper covers current and
future callers consistently, while individual checks would duplicate
the same non-recoverable handling at every call site.

Make the function return void because there is no longer a meaningful
result for callers to consume.

On UP, warn if the supplied CPU is not the current CPU because the work
cannot be scheduled in that case.

Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
 include/linux/stop_machine.h | 20 +++++++++-----------
 kernel/stop_machine.c        | 10 +++-------
 2 files changed, 12 insertions(+), 18 deletions(-)

diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
index 01011113d226..604ab4d103a8 100644
--- a/include/linux/stop_machine.h
+++ b/include/linux/stop_machine.h
@@ -31,7 +31,7 @@ struct cpu_stop_work {
 
 int stop_one_cpu(unsigned int cpu, cpu_stop_fn_t fn, void *arg);
 int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg);
-bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
 			 struct cpu_stop_work *work_buf);
 void stop_machine_park(int cpu);
 void stop_machine_unpark(int cpu);
@@ -68,19 +68,17 @@ static void stop_one_cpu_nowait_workfn(struct work_struct *work)
 	preempt_enable();
 }
 
-static inline bool stop_one_cpu_nowait(unsigned int cpu,
+static inline void stop_one_cpu_nowait(unsigned int cpu,
 				       cpu_stop_fn_t fn, void *arg,
 				       struct cpu_stop_work *work_buf)
 {
-	if (cpu == smp_processor_id()) {
-		INIT_WORK(&work_buf->work, stop_one_cpu_nowait_workfn);
-		work_buf->fn = fn;
-		work_buf->arg = arg;
-		schedule_work(&work_buf->work);
-		return true;
-	}
-
-	return false;
+	if (WARN_ON_ONCE(cpu != smp_processor_id()))
+		return;
+
+	INIT_WORK(&work_buf->work, stop_one_cpu_nowait_workfn);
+	work_buf->fn = fn;
+	work_buf->arg = arg;
+	schedule_work(&work_buf->work);
 }
 
 static inline void print_stop_info(const char *log_lvl, struct task_struct *task) { }
diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
index 773d8e9ae30c..a8a91b1e62f1 100644
--- a/kernel/stop_machine.c
+++ b/kernel/stop_machine.c
@@ -377,16 +377,12 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
  *
  * CONTEXT:
  * Don't care.
- *
- * RETURNS:
- * true if cpu_stop_work was queued successfully and @fn will be called,
- * false otherwise.
  */
-bool stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
-			struct cpu_stop_work *work_buf)
+void stop_one_cpu_nowait(unsigned int cpu, cpu_stop_fn_t fn, void *arg,
+			 struct cpu_stop_work *work_buf)
 {
 	*work_buf = (struct cpu_stop_work){ .fn = fn, .arg = arg, .caller = _RET_IP_, };
-	return cpu_stop_queue_work(cpu, work_buf);
+	WARN_ON_ONCE(!cpu_stop_queue_work(cpu, work_buf));
 }
 
 static bool queue_stop_cpus_work(const struct cpumask *cpumask,
-- 
2.53.0

Re: [PATCH] stop_machine: Make stop_one_cpu_nowait() return void

[Bradley Morgan]

Hi Yury,

Nice. The bool was a lie anyway, nobody reads it.

[...]
> -static inline bool stop_one_cpu_nowait(unsigned int cpu,
> +static inline void stop_one_cpu_nowait(unsigned int cpu,
>  				       cpu_stop_fn_t fn, void *arg,
>  				       struct cpu_stop_work *work_buf)
>  {
[...]
> +	if (WARN_ON_ONCE(cpu != smp_processor_id()))
> +		return;

Fine, the silent false was worse. Nit: stop_machine.h never includes
linux/bug.h itself, so this WARN_ON_ONCE only builds through whatever
cpu.h happens to drag in. maybe add the include while youre in here.

[...]
>   * CONTEXT:
>   * Don't care.
> - *
> - * RETURNS:
> - * true if cpu_stop_work was queued successfully and @fn will be called,
> - * false otherwise.
>   */

"Don't care" is stale now. The old contract was: call this from wherever,
worst case you get false back and deal with it. The new contract is that
the caller must keep the target CPU's stopper from getting parked while
it queues, or it eats a WARN. Thats the whole preempt_disable() after
observing the CPU online dance from f0498d2a54e7. The changelog says it,
the comment doesnt, and future callers will read the comment. please put
something like:

 * CONTEXT:
 * Don't care, but the caller must ensure @cpu's stopper stays enabled
 * until the work is queued, e.g. by disabling preemption after having
 * observed @cpu online. See f0498d2a54e7 ("sched: Fix
 * stop_one_cpu_nowait() vs hotplug").

With the CONTEXT bit sorted this looks good to me. The bug.h include
is optional, skip it if you dont care.

Thanks!