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

[Mani Tofigh <mt4022@columbia.edu>]

On 11/3/26 10:15 AM, Sebastian Andrzej Siewior wrote:
> Is this just the net_rps interface or was this used for a reproducer and
> the issue is different?


It's RPS, just because it was the easiest way to keep NET_RX pending on
one CPU from userspace. The issue is any softirq that keeps getting
raised while ksoftirqd is asked to park.

> Would avoid it. On the other hand networking has HP-handler which
> ensures that the queues of the offlined CPU are handled.

True, cpu_active() is the better check there (and the RFS check a few
lines up probably wants the same). I'm happy to send that to netdev
separately.

It only covers RPS though. A NIC IRQ still affine to that CPU, or a
tasklet that's scheduled while disabled, keeps ksoftirqd busy the same
way. So I'd keep the smpboot change and do the get_rps_cpu() one on top.

> My original issue were queued up irq-work items which were not handled
> due to the park request and couldn't be handled otherwise.
> 

That still works, run_irq_workd() runs once and takes the whole
lazy_list before the thread parks.

> 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@columbia.edu>
>> ---
>> 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.
> 	 */

Thanks! I will use that in v2.

> The thread_fn should only run in HP_THREAD_ACTIVE case.
> 
It does already, should_run includes td->status == HP_THREAD_ACTIVE.

Mani