On Thu, Feb 25, 2021 at 5:20 PM Jakub Kicinski <k...@kernel.org> wrote: > > On Thu, 25 Feb 2021 16:16:20 -0800 Wei Wang wrote: > > On Thu, Feb 25, 2021 at 3:00 PM Jakub Kicinski <k...@kernel.org> wrote: > > > On Thu, 25 Feb 2021 10:29:47 -0800 Wei Wang wrote: > > > > 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? > > > > > 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 :) > > > > > > > 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.
I was good with either approach. My preference would be to probably use kthread_create regardless as it doesn't make much sense to have the thread running until we really need it anyway.
