Re: [PATCH] smp: Wait for enqueued work regardless of IPI sent

[Yury Norov <yury.norov@gmail.com>]

On Wed, Jul 02, 2025 at 01:59:54PM -0400, Rik van Riel wrote:
> On Wed, 2 Jul 2025 13:44:34 -0400
> Yury Norov <yury.norov@gmail.com> wrote:
> 
> > Thank you guys for explaining that and sorry for the buggy patch.
> > I was actually under impression that run_remote duplicates nr_cpus !=0,
> > and even have a patch that removes run_remote.
> > 
> > Maybe worth to add a comment on what run_remote and nr_cpus track?
> 
> This thread did surface some useful content, and Jann also pointed out
> a good optimization that can be made, by not setting run_remote if
> "func" tells us to skip remote CPUs.
> 
> Thomas, please let me know if you already reverted Yury's patch,
> and want me to re-send this without the last hunk.
> 
> ---8<---
> From 2ae6417fa7ce16f1bfa574cbabba572436adbed9 Mon Sep 17 00:00:00 2001
> From: Rik van Riel <riel@surriel.com>
> Date: Wed, 2 Jul 2025 13:52:54 -0400
> Subject: [PATCH] smp: Wait for enqueued work regardless of IPI sent
> 
> Whenever work is enqueued with a remote CPU, smp_call_function_many_cond()
> may need to wait for that work to be completed, regardless of whether or
> not the remote CPU needed to be woken up with an IPI, or the work was
> being added to the queue of an already woken up CPU.
> 
> However, if no work is enqueued with a remote CPU, because "func"
> told us to skip all CPUs, do not wait.
> 
> Document the difference between "work enqueued", and "CPU needs to be
> woken up"
> 
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Suggested-by: Jann Horn <jannh@google.com>
> Reported-by: syzbot+084b6e5bc1016723a9c4@syzkaller.appspotmail.com
> Fixes: a12a498a9738 ("smp: Don't wait for remote work done if not needed in smp_call_function_many_cond()")
> ---
>  kernel/smp.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 84561258cd22..c5e1da7a88da 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -802,7 +802,6 @@ static void smp_call_function_many_cond(const struct cpumask *mask,
>  
>  	/* Check if we need remote execution, i.e., any CPU excluding this one. */
>  	if (cpumask_any_and_but(mask, cpu_online_mask, this_cpu) < nr_cpu_ids) {
> -		run_remote = true;
>  		cfd = this_cpu_ptr(&cfd_data);
>  		cpumask_and(cfd->cpumask, mask, cpu_online_mask);
>  		__cpumask_clear_cpu(this_cpu, cfd->cpumask);
> @@ -816,6 +815,9 @@ static void smp_call_function_many_cond(const struct cpumask *mask,
>  				continue;
>  			}
>  
> +			/* Work is enqueued on a remote CPU. */
> +			run_remote = true;
> +

I actually ended up with the same on my cratch branch:

https://github.com/norov/linux/commit/8a32ca4b60dc68ac54f3b70b4be7a5863dc3934e

So,

Reviewed-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>

>  			csd_lock(csd);
>  			if (wait)
>  				csd->node.u_flags |= CSD_TYPE_SYNC;
> @@ -827,6 +829,10 @@ static void smp_call_function_many_cond(const struct cpumask *mask,
>  #endif
>  			trace_csd_queue_cpu(cpu, _RET_IP_, func, csd);
>  
> +			/*
> +			 * Kick the remote CPU if this is the first work
> +			 * item enqueued.
> +			 */
>  			if (llist_add(&csd->node.llist, &per_cpu(call_single_queue, cpu))) {
>  				__cpumask_set_cpu(cpu, cfd->cpumask_ipi);
>  				nr_cpus++;
> @@ -843,8 +849,6 @@ static void smp_call_function_many_cond(const struct cpumask *mask,
>  			send_call_function_single_ipi(last_cpu);
>  		else if (likely(nr_cpus > 1))
>  			send_call_function_ipi_mask(cfd->cpumask_ipi);
> -		else
> -			run_remote = false;
>  	}
>  
>  	/* Check if we need local execution. */
> -- 
> 2.49.0
>