Re: [PATCH] smpboot: Run the thread function only once before parking

[Sebastian Andrzej Siewior]

On 2026-11-02 16:51:24 [-0500], Mani Tofigh via B4 Relay wrote:
> From: Mani Tofigh <mt4022@xxxxxxxxxxxx>
> 
> Since commit 4a3b51aab6e2 ("smpboot: Don't park the thread if work is
> pending"), smpboot_thread_fn() parks only once thread_should_run() is
> false.  CPU offline waits for that with no timeout and with
> cpu_hotplug_lock held for write.
> 
> When the outgoing CPU keeps receiving more packets than it can process,
> ksoftirqd always has NET_RX pending and never parks.  The offline then
> waits for as long as the packets keep coming and everything that takes
> cpus_read_lock() waits with it:
> 
…
> That commit only needs the work queued before the park request to run
> once on its own CPU.  Softirqs left pending are taken over by the
> dead-CPU callbacks as before.  So on a park request, run the thread
> function once if there is work and then park.  With the fix, the
> offline finishes under the same load.
> 
> This bug was identified and confirmed with the help of a deterministic
> hypervisor currently under development.  The C reproducer below however
> hits it within about 25s.

Is this just the net_rps interface or was this used for a reproducer and
the issue is different?

On CPU down path we do first smpboot_park_threads() on the target CPU
and later takedown_cpu() is invoked which flips the CPU bit in the
cpu_online mask. This means work can be enqueued while the CPU is going
down. Something like

--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5402,7 +5402,7 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 			rflow = set_rps_cpu(dev, skb, rflow, next_cpu, hash);
 		}
 
-		if (tcpu < nr_cpu_ids && cpu_online(tcpu)) {
+		if (tcpu < nr_cpu_ids && cpu_active(tcpu)) {
 			*rflowp = rflow;
 			cpu = tcpu;
 			goto done;
@@ -5413,7 +5413,7 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 
 	if (map) {
 		tcpu = map->cpus[reciprocal_scale(hash, map->len)];
-		if (cpu_online(tcpu)) {
+		if (cpu_active(tcpu)) {
 			cpu = tcpu;
 			goto done;
 		}

Would avoid it. On the other hand networking has HP-handler which
ensures that the queues of the offlined CPU are handled.
My original issue were queued up irq-work items which were not handled
due to the park request and couldn't be handled otherwise.

The suggested change is okay in principle restores the old behaviour
where we did not care but run the work at least once.

> Fixes: 4a3b51aab6e2 ("smpboot: Don't park the thread if work is pending")
> Signed-off-by: Mani Tofigh <mt4022@xxxxxxxxxxxx>
> ---
> Reproducer: https://github.com/manitofigh/files/blob/main/linux-repros/ksoftpark.c
> ---
>  kernel/smpboot.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/smpboot.c b/kernel/smpboot.c
> index 3f60e8c6dd30..a2ddd10a8577 100644
> --- a/kernel/smpboot.c
> +++ b/kernel/smpboot.c
> @@ -119,9 +119,12 @@ static int smpboot_thread_fn(void *data)
>  		}
>  
>  		should_run = td->status == HP_THREAD_ACTIVE && ht->thread_should_run(td->cpu);
> -		if (kthread_should_park() && !should_run) {
> +		if (kthread_should_park()) {
>  			__set_current_state(TASK_RUNNING);
>  			preempt_enable();
> +			/* Run pending work once, new work may keep coming */
	/*
	 * Run pending work once to handle queued work prior the parking
	 * request. Don't try to process everything in case new work
	 * continues to come and the handler has a hotplug notifier for
	 * everything that was enqueued later.
	 */

> +			if (should_run)
> +				ht->thread_fn(td->cpu);

The thread_fn should only run in HP_THREAD_ACTIVE case.

>  			if (ht->park && td->status == HP_THREAD_ACTIVE) {
>  				BUG_ON(td->cpu != smp_processor_id());
>  				ht->park(td->cpu);
> 

Sebastian