On Thu, 25 Feb 2021 16:16:20 -0800 Wei Wang wrote:

On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote:  

quoted

Hmm... I don't think the above patch would work. Consider a situation that:
1. At first, the kthread is in sleep mode.
2. Then someone calls napi_schedule() to schedule work on this napi.
So ____napi_schedule() is called. But at this moment, the kthread is
not yet in RUNNING state. So this function does not set SCHED_THREAD
bit.
3. Then wake_up_process() is called to wake up the thread.
4. Then napi_threaded_poll() calls napi_thread_wait().  

But how is the task not in running state outside of napi_thread_wait()?

My scheduler knowledge is rudimentary, but AFAIU off CPU tasks which
were not put to sleep are still in RUNNING state, so unless we set
INTERRUPTIBLE the task will be running, even if it's stuck in cond_resched().

I think the thread is only in RUNNING state after wake_up_process() is
called on the thread in ____napi_schedule(). Before that, it should be
in INTERRUPTIBLE state. napi_thread_wait() explicitly calls
set_current_state(TASK_INTERRUPTIBLE) when it finishes 1 round of
polling.

Are you concerned about it not being in RUNNING state after it's
spawned but before it's first parked?

quoted

quoted

woken is false
and SCHED_THREAD bit is not set. So the kthread will go to sleep again
(in INTERRUPTIBLE mode) when schedule() is called, and waits to be
woken up by the next napi_schedule().
That will introduce arbitrary delay for the napi->poll() to be called.
Isn't it? Please enlighten me if I did not understand it correctly.  

Probably just me not understanding the scheduler :)
 

quoted

I personally prefer to directly set SCHED_THREAD bit in ____napi_schedule().
Or stick with SCHED_BUSY_POLL solution and replace kthread_run() with
kthread_create().  

Well, I'm fine with that too, no point arguing further if I'm not
convincing anyone. But we need a fix which fixes the issue completely,
not just one of three incarnations.  

Alexander and Eric,
Do you guys have preference on which approach to take?
If we keep the current SCHED_BUSY_POLL patch, I think we need to
change kthread_run() to kthread_create() to address the warning Martin
reported.
Or if we choose to set SCHED_THREADED, we could keep kthread_run().
But there is 1 extra set_bit() operation.

To be clear extra set_bit() only if thread is running, which if IRQ
coalescing works should be rather rare.