On Wed, 07 Apr 2021 16:54:29 +0200 Paolo Abeni wrote:
> > > I think in the above example even the normal processing will be
> > > fooled?!? e.g. even without the napi_disable(), napi_thread_wait() will
> > > will miss the event/will not understand to it really own the napi and
> > > will call schedule().
> > >
> > > It looks a different problem to me ?!?
> > >
> > > I *think* that replacing inside the napi_thread_wait() loop:
> > >
> > > if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken)
> > >
> > > with:
> > >
> > > unsigned long state = READ_ONCE(napi->state);
> > >
> > > if (state & NAPIF_STATE_SCHED &&
> > > !(state & (NAPIF_STATE_IN_BUSY_POLL | NAPIF_STATE_DISABLE))
> > >
> > > should solve it and should also allow removing the
> > > NAPI_STATE_SCHED_THREADED bit. I feel like I'm missing some relevant
> > > point here.
> >
> > Heh, that's closer to the proposal Eric put forward.
> >
> > I strongly dislike
>
> I guess that can't be addressed ;)
I'm not _that_ unreasonable, I hope :) if there is multiple people
disagreeing with me then so be it.
> > the idea that every NAPI consumer needs to be aware
> > of all the other consumers to make things work. That's n^2 mental
> > complexity.
>
> IMHO the overall complexity is not that bad: both napi_disable() and
> NAPI poll already set their own specific NAPI bit when acquiring the
> NAPI instance, they don't need to be aware of any other NAPI consumer
> internal.
>
> The only NAPI user that needs to be aware of others is napi threaded,
> and I guess/hope we are not going to add more different kind of NAPI
> users.
I thought we agreed that we should leave the door open for other
pollers as a condition of merging this simplistic thread thing.
> If you have strong opinion against the above, the only other option I
> can think of is patching napi_schedule_prep() to set
> both NAPI_STATE_SCHED and NAPI_STATE_SCHED_THREADED if threaded mode is
> enabled for the running NAPI. That looks more complex and error prone,
> so I really would avoid that.
>
> Any other better option?
>
> Side note: regardless of the above, I think we still need something
> similar to the code in this patch, can we address the different issues
> separately?
Not sure what issues you're referring to.
> > > Here I do not follow?!? Modulo the tiny race (which i was unable to
> > > trigger so far) above napi_disable()/napi_enable() loops work correctly
> > > here.
> > >
> > > Could you please re-phrase?
> >
> > After napi_disable() the thread will exit right? (napi_thread_wait()
> > returns -1, the loop in napi_threaded_poll() breaks, and the function
> > returns).
> >
> > napi_enable() will not re-start the thread.
> >
> > What driver are you testing with? You driver must always call
> > netif_napi_del() and netif_napi_add().
>
> veth + some XDP dummy prog - used just to enable NAPI.
>
> Yep, it does a full netif_napi_del()/netif_napi_add().
>
> Looks like plain napi_disable()/napi_enable() is not going to work in
> threaded mode.
Right, I think the problem is disable_pending check is out of place.
How about this:
diff --git a/net/core/dev.c b/net/core/dev.c
index 9d1a8fac793f..e53f8bfed6a1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7041,7 +7041,7 @@ static int napi_thread_wait(struct napi_struct *napi)
set_current_state(TASK_INTERRUPTIBLE);
- while (!kthread_should_stop() && !napi_disable_pending(napi)) {
+ while (!kthread_should_stop()) {
/* Testing SCHED_THREADED bit here to make sure the current
* kthread owns this napi and could poll on this napi.
* Testing SCHED bit is not enough because SCHED bit might be
@@ -7049,8 +7049,14 @@ static int napi_thread_wait(struct napi_struct *napi)
*/
if (test_bit(NAPI_STATE_SCHED_THREADED, &napi->state) || woken)
{
WARN_ON(!list_empty(&napi->poll_list));
- __set_current_state(TASK_RUNNING);
- return 0;
+ if (unlikely(napi_disable_pending(napi))) {
+ clear_bit(NAPI_STATE_SCHED, &napi->state);
+ clear_bit(NAPI_STATE_SCHED_THREADED,
+ &napi->state);
+ } else {
+ __set_current_state(TASK_RUNNING);
+ return 0;
+ }
}
schedule();
