On Wed, 2021-04-07 at 11:13 -0700, Jakub Kicinski wrote:
> 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.
I'm sorry, I mean no offence! Just joking about the fact that is
usually hard changing preferences ;)
> > 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.
The patch that started this thread was ment to address a slightly
different race: napi_disable() hanging because napi_threaded_poll()
don't clear the NAPI_STATE_SCHED even when owning the napi instance.
> 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();
It looks like the above works, and also fixes the problem I originally
reported.
I think it can be simplified as the following - if NAPIF_STATE_DISABLE
is set, napi_threaded_poll()/__napi_poll() will return clearing the
SCHEDS bits after trying to poll the device:
---
diff --git a/net/core/dev.c b/net/core/dev.c
index d9db02d4e044..5cb6f411043d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6985,7 +6985,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
---
And works as intended here. I'll submit that formally, unless there are
objection.
Thanks!
Paolo
>
