Re: [PATCH net v2] net: fix race between napi kthread mode and busy poll

On Sun, Feb 28, 2021 at 11:17 AM Jakub Kicinski [off-list ref] wrote:

On Sat, 27 Feb 2021 15:23:56 -0800 Wei Wang wrote:

quoted

quoted

quoted

Indeed, looks like the task will be in WAKING state until it runs?
We can switch the check in ____napi_schedule() from

        if (thread->state == TASK_RUNNING)

to

        if (!(thread->state & TASK_INTERRUPTIBLE))

?

Hmm... I am not very sure what state the thread will be put in after
kthread_create(). Could it be in TASK_INTERRUPTIBLE?

I did a printk and confirmed that the thread->state is
TASK_UNINTERRUPTIBLE after kthread_create() is called.
So I think if we change the above state to:
          if (thread->state != TASK_INTERRUPTIBLE)
                  set_bit(NAPI_STATE_SCHED_THREADED, &napi->state);
It should work.

quoted

diff --git a/net/core/dev.c b/net/core/dev.c
index 6c5967e80132..43607523ee99 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -1501,17 +1501,18 @@ static int napi_kthread_create(struct napi_struct *n)
 {
        int err = 0;

-       /* Create and wake up the kthread once to put it in
-        * TASK_INTERRUPTIBLE mode to avoid the blocked task
-        * warning and work with loadavg.
+       /* Avoid waking up the kthread during creation to prevent
+        * potential race.
         */
-       n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
-                               n->dev->name, n->napi_id);
+       n->thread = kthread_create(napi_threaded_poll, n, "napi/%s-%d",
+                                  n->dev->name, n->napi_id);

Does kthread_run() make the thread go into TASK_INTERRUPTIBLE ?
It just calls wake_up_process(), which according to a comment in the
kdoc..

 * Conceptually does:
 *
 *   If (@state & @p->state) @p->state = TASK_RUNNING.

So I think we could safely stick to kthread_run() if the condition in
at the NAPI wake point checks for INTERRUPTIBLE?

I think so. kthread_run() wakes up the kthread and kthread_wait_poll()
should put it to INTERRUPTIBLE mode and schedule() will make it go to
sleep, and wait for the next napi_schedule().
I've also tested on my setup and saw no issues.