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;
+
 			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

On Wed, Jul 02 2025 at 13:59, Rik van Riel wrote:
> Thomas, please let me know if you already reverted Yury's patch,
> and want me to re-send this without the last hunk.

I did so immediately after saying so in my previous reply. It's gone in
tip and next.

Thanks,

        tglx

On Thu, 03 Jul 2025 18:56:11 +0200
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, Jul 02 2025 at 13:59, Rik van Riel wrote:
> > Thomas, please let me know if you already reverted Yury's patch,
> > and want me to re-send this without the last hunk.  
> 
> I did so immediately after saying so in my previous reply. It's gone in
> tip and next.

Here is v2 of the patch, with the last hunk removed, and
the changelog adjusted to match the new context.

---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 only if work was enqueued

Whenever work is enqueued with a remote CPU, smp_call_function_many_cond()
may need to wait for that work to be completed. However, if no work is
enqueued with a remote CPU, because "func" told us to skip all CPUs,
there is no need to wait.

Set run_remote only if work was enqueued on remote CPUs.

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>
Reviewed-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
---
 kernel/smp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 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;
+
 			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++;
-- 
2.49.0

The following commit has been merged into the smp/core branch of tip:

Commit-ID:     946a7281982530d333eaee62bd1726f25908b3a9
Gitweb:        https://git.kernel.org/tip/946a7281982530d333eaee62bd1726f25908b3a9
Author:        Rik van Riel <riel@surriel.com>
AuthorDate:    Wed, 02 Jul 2025 13:52:54 -04:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Sun, 06 Jul 2025 11:57:39 +02:00

smp: Wait only if work was enqueued

Whenever work is enqueued for a remote CPU, smp_call_function_many_cond()
may need to wait for that work to be completed. However, if no work is
enqueued for a remote CPU, because the condition func() evaluated to false
for all CPUs, there is no need to wait.

Set run_remote only if work was enqueued on remote CPUs.

Document the difference between "work enqueued", and "CPU needs to be
woken up"

Suggested-by: Jann Horn <jannh@google.com>
Signed-off-by: Rik van Riel <riel@surriel.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Yury Norov (NVIDIA) <yury.norov@gmail.com>
Link: https://lore.kernel.org/all/20250703203019.11331ac3@fangorn

---
 kernel/smp.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 99d1fd0..c5e1da7 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;
+
 			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++;

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
>