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

[Shrikanth Hegde <sshegde@linux.ibm.com>]

Hi Yury.

On 7/29/26 7:53 AM, Yury Norov wrote:
> 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.
> 
> CC: Bradley Morgan <include@grrlz.net>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
> ---
> v1: https://lore.kernel.org/all/20260724232325.594212-1-ynorov@nvidia.com/
> v2:
>   - carefully include linux/bug.h (Bradley);
>   - update the CONTEXT section (Bradley).
> 
>   include/linux/stop_machine.h | 21 ++++++++++-----------
>   kernel/stop_machine.c        | 14 ++++++--------
>   2 files changed, 16 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/stop_machine.h b/include/linux/stop_machine.h
> index 01011113d226..84e7fb627ba4 100644
> --- a/include/linux/stop_machine.h
> +++ b/include/linux/stop_machine.h
> @@ -2,6 +2,7 @@
>   #ifndef _LINUX_STOP_MACHINE
>   #define _LINUX_STOP_MACHINE
>   
> +#include <linux/bug.h>
>   #include <linux/cpu.h>
>   #include <linux/cpumask_types.h>
>   #include <linux/smp.h>
> @@ -31,7 +32,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 +69,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..d085ba1f4b44 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -7,6 +7,7 @@
>    * Copyright (C) 2010		SUSE Linux Products GmbH
>    * Copyright (C) 2010		Tejun Heo <tj@kernel.org>
>    */
> +#include <linux/bug.h>
>   #include <linux/compiler.h>
>   #include <linux/completion.h>
>   #include <linux/cpu.h>
> @@ -376,17 +377,14 @@ int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *
>    * and will remain untouched until stopper starts executing @fn.
>    *
>    * CONTEXT:
> - * Don't care.
> - *
> - * RETURNS:
> - * true if cpu_stop_work was queued successfully and @fn will be called,
> - * false otherwise.
> + * Don't care, but the caller must ensure @cpu's stopper stays enabled
> + * until the work is queued, e.g. by preempt_disable().

nit:
* until the work is queued, i.e. ensuring preemption/irq disabled. ?

Because, if irq are disabled it is equally good enough.
#define preemptible()   (preempt_count() == 0 && !irqs_disabled())


>    */
> -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,

Other than that,
Reviewed-by: Shrikanth Hegde <sshegde@linux.ibm.com>