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

On Fri, Feb 26, 2021 at 4:48 PM Jakub Kicinski [off-list ref] wrote:

On Fri, 26 Feb 2021 16:30:47 -0800 Wei Wang wrote:

quoted

              thread = READ_ONCE(napi->thread);
              if (thread) {
+                     set_bit(NAPI_STATE_SCHED_THREADED, &napi->state);
                      wake_up_process(thread);

What about the version which checks RUNNING? As long as
wake_up_process() implies a barrier I _think_ it should
work as well. Am I missing some case, or did you decide
to go with the simpler/safer approach?

I assume you are referring to the following proposed patch in your
previous email right?

--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -4294,6 +4294,8 @@ static inline void ____napi_schedule(struct

softnet_data *sd,
                 */
                thread = READ_ONCE(napi->thread);
                if (thread) {
+                       if (thread->state == TASK_RUNNING)
+                               set_bit(NAPIF_STATE_SCHED_THREAD, &napi->state);
                        wake_up_process(thread);
                        return;
                }

@@ -6486,7 +6488,8 @@ bool napi_complete_done(struct napi_struct *n,

int work_done)
                WARN_ON_ONCE(!(val & NAPIF_STATE_SCHED));

                new = val & ~(NAPIF_STATE_MISSED | NAPIF_STATE_SCHED |
-                             NAPIF_STATE_PREFER_BUSY_POLL);
+                             NAPIF_STATE_PREFER_BUSY_POLL |
+                             NAPIF_STATE_SCHED_THREAD);

                /* If STATE_MISSED was set, leave STATE_SCHED set,
                 * because we will call napi->poll() one more time.

@@ -6968,16 +6971,24 @@ static int napi_poll(struct napi_struct *n,

struct list_head *repoll)

 static int napi_thread_wait(struct napi_struct *napi)
 {
+       bool woken = false;
+
        set_current_state(TASK_INTERRUPTIBLE);

        while (!kthread_should_stop() && !napi_disable_pending(napi)) {
-               if (test_bit(NAPI_STATE_SCHED, &napi->state)) {
+               unsigned long state = READ_ONCE(napi->state);
+
+               if ((state & NAPIF_STATE_SCHED) &&
+                   ((state & NAPIF_STATE_SCHED_THREAD) || woken)) {
                        WARN_ON(!list_empty(&napi->poll_list));
                        __set_current_state(TASK_RUNNING);
                        return 0;
+               } else {
+                       WARN_ON(woken);
                }

                schedule();
+               woken = true;
                set_current_state(TASK_INTERRUPTIBLE);
        }
        __set_current_state(TASK_RUNNING);

I don't think it is sufficient to only set SCHED_THREADED bit when the
thread is in RUNNING state.
In fact, the thread is most likely NOT in RUNNING mode before we call
wake_up_process() in ____napi_schedule(), because it has finished the
previous round of napi->poll() and SCHED bit was cleared, so
napi_thread_wait() sets the state to INTERRUPTIBLE and schedule() call
should already put it in sleep.