[PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Samiullah Khawaja]

napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
before stopping the kthread. But it uses test_bit with the
NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
the flag is unset.

Use the NAPI_* variant of the NAPI state bits in test_bit instead.

Tested:
 ./tools/testing/selftests/net/nl_netdev.py
 TAP version 13
 1..7
 ok 1 nl_netdev.empty_check
 ok 2 nl_netdev.lo_check
 ok 3 nl_netdev.page_pool_check
 ok 4 nl_netdev.napi_list_check
 ok 5 nl_netdev.dev_set_threaded
 ok 6 nl_netdev.napi_set_threaded
 ok 7 nl_netdev.nsim_rxq_reset_down
 # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0

 ./tools/testing/selftests/drivers/net/napi_threaded.py
 TAP version 13
 1..2
 ok 1 napi_threaded.change_num_queues
 ok 2 napi_threaded.enable_dev_threaded_disable_napi_threaded
 # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0

Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
Signed-off-by: Samiullah Khawaja <skhawaja@google.com>

---
 net/core/dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 93a25d87b86b..8d49b2198d07 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6965,7 +6965,7 @@ static void napi_stop_kthread(struct napi_struct *napi)
 	 * the kthread.
 	 */
 	while (true) {
-		if (!test_bit(NAPIF_STATE_SCHED_THREADED, &napi->state))
+		if (!test_bit(NAPI_STATE_SCHED_THREADED, &napi->state))
 			break;
 
 		msleep(20);
-- 
2.51.0.384.g4c02a37b29-goog

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Jakub Kicinski]

On Wed, 10 Sep 2025 20:37:16 +0000 Samiullah Khawaja wrote:
> napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
> before stopping the kthread. But it uses test_bit with the
> NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
> the flag is unset.
> 
> Use the NAPI_* variant of the NAPI state bits in test_bit instead.
> 
> Tested:
>  ./tools/testing/selftests/net/nl_netdev.py
>  TAP version 13
>  1..7
>  ok 1 nl_netdev.empty_check
>  ok 2 nl_netdev.lo_check
>  ok 3 nl_netdev.page_pool_check
>  ok 4 nl_netdev.napi_list_check
>  ok 5 nl_netdev.dev_set_threaded
>  ok 6 nl_netdev.napi_set_threaded
>  ok 7 nl_netdev.nsim_rxq_reset_down
>  # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
>  ./tools/testing/selftests/drivers/net/napi_threaded.py
>  TAP version 13
>  1..2
>  ok 1 napi_threaded.change_num_queues
>  ok 2 napi_threaded.enable_dev_threaded_disable_napi_threaded
>  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>

Is this basically addressing the bug that Martin run into?

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 93a25d87b86b..8d49b2198d07 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -6965,7 +6965,7 @@ static void napi_stop_kthread(struct napi_struct *napi)
>  	 * the kthread.
>  	 */
>  	while (true) {
> -		if (!test_bit(NAPIF_STATE_SCHED_THREADED, &napi->state))
> +		if (!test_bit(NAPI_STATE_SCHED_THREADED, &napi->state))
>  			break;
>  
>  		msleep(20);

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Samiullah Khawaja]

On Thu, Sep 11, 2025 at 6:40 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 10 Sep 2025 20:37:16 +0000 Samiullah Khawaja wrote:
> > napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
> > before stopping the kthread. But it uses test_bit with the
> > NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
> > the flag is unset.
> >
> > Use the NAPI_* variant of the NAPI state bits in test_bit instead.
> >
> > Tested:
> >  ./tools/testing/selftests/net/nl_netdev.py
> >  TAP version 13
> >  1..7
> >  ok 1 nl_netdev.empty_check
> >  ok 2 nl_netdev.lo_check
> >  ok 3 nl_netdev.page_pool_check
> >  ok 4 nl_netdev.napi_list_check
> >  ok 5 nl_netdev.dev_set_threaded
> >  ok 6 nl_netdev.napi_set_threaded
> >  ok 7 nl_netdev.nsim_rxq_reset_down
> >  # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
> >
> >  ./tools/testing/selftests/drivers/net/napi_threaded.py
> >  TAP version 13
> >  1..2
> >  ok 1 napi_threaded.change_num_queues
> >  ok 2 napi_threaded.enable_dev_threaded_disable_napi_threaded
> >  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> >
> > Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
> > Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
>
> Is this basically addressing the bug that Martin run into?
Not really. That one was because the busy polling bit remained set
during kthread stop. Basically in this function when we unset
STATE_THREADED, we also needed to unset STATE_THREADED_BUSY_POLL.

@@ -7000,7 +7002,8 @@ static void napi_stop_kthread(struct napi_struct *napi)
                 */
                if ((val & NAPIF_STATE_SCHED_THREADED) ||
                    !(val & NAPIF_STATE_SCHED)) {
-                       new = val & (~NAPIF_STATE_THREADED);
+                       new = val & (~(NAPIF_STATE_THREADED |
+                                      NAPIF_STATE_THREADED_BUSY_POLL));

>
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 93a25d87b86b..8d49b2198d07 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -6965,7 +6965,7 @@ static void napi_stop_kthread(struct napi_struct *napi)
> >        * the kthread.
> >        */
> >       while (true) {
> > -             if (!test_bit(NAPIF_STATE_SCHED_THREADED, &napi->state))
> > +             if (!test_bit(NAPI_STATE_SCHED_THREADED, &napi->state))
> >                       break;
> >
> >               msleep(20);
>

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Samiullah Khawaja]

On Thu, Sep 11, 2025 at 7:10 AM Samiullah Khawaja <skhawaja@google.com> wrote:
>
> On Thu, Sep 11, 2025 at 6:40 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed, 10 Sep 2025 20:37:16 +0000 Samiullah Khawaja wrote:
> > > napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
> > > before stopping the kthread. But it uses test_bit with the
> > > NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
> > > the flag is unset.
> > >
> > > Use the NAPI_* variant of the NAPI state bits in test_bit instead.
> > >
> > > Tested:
> > >  ./tools/testing/selftests/net/nl_netdev.py
> > >  TAP version 13
> > >  1..7
> > >  ok 1 nl_netdev.empty_check
> > >  ok 2 nl_netdev.lo_check
> > >  ok 3 nl_netdev.page_pool_check
> > >  ok 4 nl_netdev.napi_list_check
> > >  ok 5 nl_netdev.dev_set_threaded
> > >  ok 6 nl_netdev.napi_set_threaded
> > >  ok 7 nl_netdev.nsim_rxq_reset_down
> > >  # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
> > >
> > >  ./tools/testing/selftests/drivers/net/napi_threaded.py
> > >  TAP version 13
> > >  1..2
> > >  ok 1 napi_threaded.change_num_queues
> > >  ok 2 napi_threaded.enable_dev_threaded_disable_napi_threaded
> > >  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> > >
> > > Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
> > > Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> >
> > Is this basically addressing the bug that Martin run into?
> Not really. That one was because the busy polling bit remained set
> during kthread stop. Basically in this function when we unset
> STATE_THREADED, we also needed to unset STATE_THREADED_BUSY_POLL.
>
> @@ -7000,7 +7002,8 @@ static void napi_stop_kthread(struct napi_struct *napi)
>                  */
>                 if ((val & NAPIF_STATE_SCHED_THREADED) ||
>                     !(val & NAPIF_STATE_SCHED)) {
> -                       new = val & (~NAPIF_STATE_THREADED);
> +                       new = val & (~(NAPIF_STATE_THREADED |
> +                                      NAPIF_STATE_THREADED_BUSY_POLL));
>
Just to add to my last email: I did find this test_bit issue while
working on the fix for that other problem.
> >
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index 93a25d87b86b..8d49b2198d07 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -6965,7 +6965,7 @@ static void napi_stop_kthread(struct napi_struct *napi)
> > >        * the kthread.
> > >        */
> > >       while (true) {
> > > -             if (!test_bit(NAPIF_STATE_SCHED_THREADED, &napi->state))
> > > +             if (!test_bit(NAPI_STATE_SCHED_THREADED, &napi->state))
> > >                       break;
> > >
> > >               msleep(20);
> >

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Jakub Kicinski]

On Thu, 11 Sep 2025 07:31:11 -0700 Samiullah Khawaja wrote:
> > > Is this basically addressing the bug that Martin run into?  
> > Not really. That one was because the busy polling bit remained set
> > during kthread stop. Basically in this function when we unset
> > STATE_THREADED, we also needed to unset STATE_THREADED_BUSY_POLL.
> >
> > @@ -7000,7 +7002,8 @@ static void napi_stop_kthread(struct napi_struct *napi)
> >                  */
> >                 if ((val & NAPIF_STATE_SCHED_THREADED) ||
> >                     !(val & NAPIF_STATE_SCHED)) {
> > -                       new = val & (~NAPIF_STATE_THREADED);
> > +                       new = val & (~(NAPIF_STATE_THREADED |
> > +                                      NAPIF_STATE_THREADED_BUSY_POLL));
> >  
> Just to add to my last email: I did find this test_bit issue while
> working on the fix for that other problem.

I see, makes sense. I was curious whether your previous posting would
work with just this fix. I guess we'll wait until next week 'cause
this fix didn't make it to today's PR in time :(

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 10 Sep 2025 20:37:16 +0000 you wrote:
> napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
> before stopping the kthread. But it uses test_bit with the
> NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
> the flag is unset.
> 
> Use the NAPI_* variant of the NAPI state bits in test_bit instead.
> 
> [...]

Here is the summary with links:
  - [net] net: Use NAPI_* in test_bit when stopping napi kthread
    https://git.kernel.org/netdev/net/c/247981eecd3d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Simon Horman]

On Wed, Sep 10, 2025 at 08:37:16PM +0000, Samiullah Khawaja wrote:
> napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
> before stopping the kthread. But it uses test_bit with the
> NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
> the flag is unset.
> 
> Use the NAPI_* variant of the NAPI state bits in test_bit instead.

I think it would be useful to mention the difference between 
NAPI_STATE_SCHED_THREADED and NAPIF_STATE_SCHED_THREADED.

For me, that would be that one is a bit number, while
the other is a mask with only the corresponding bit set.

> 
> Tested:
>  ./tools/testing/selftests/net/nl_netdev.py
>  TAP version 13
>  1..7
>  ok 1 nl_netdev.empty_check
>  ok 2 nl_netdev.lo_check
>  ok 3 nl_netdev.page_pool_check
>  ok 4 nl_netdev.napi_list_check
>  ok 5 nl_netdev.dev_set_threaded
>  ok 6 nl_netdev.napi_set_threaded
>  ok 7 nl_netdev.nsim_rxq_reset_down
>  # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
>  ./tools/testing/selftests/drivers/net/napi_threaded.py
>  TAP version 13
>  1..2
>  ok 1 napi_threaded.change_num_queues
>  ok 2 napi_threaded.enable_dev_threaded_disable_napi_threaded
>  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> 
> Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
> Signed-off-by: Samiullah Khawaja <skhawaja@google.com>

With the above addressed, feel free to add:

Reviewed-by: Simon Horman <horms@kernel.org>

...

Re: [PATCH net] net: Use NAPI_* in test_bit when stopping napi kthread

[Simon Horman]

On Fri, Sep 12, 2025 at 08:26:03PM +0100, Simon Horman wrote:
> On Wed, Sep 10, 2025 at 08:37:16PM +0000, Samiullah Khawaja wrote:
> > napi_stop_kthread waits for the NAPI_STATE_SCHED_THREADED to be unset
> > before stopping the kthread. But it uses test_bit with the
> > NAPIF_STATE_SCHED_THREADED and that might stop the kthread early before
> > the flag is unset.
> > 
> > Use the NAPI_* variant of the NAPI state bits in test_bit instead.
> 
> I think it would be useful to mention the difference between 
> NAPI_STATE_SCHED_THREADED and NAPIF_STATE_SCHED_THREADED.
> 
> For me, that would be that one is a bit number, while
> the other is a mask with only the corresponding bit set.
> 
> > 
> > Tested:
> >  ./tools/testing/selftests/net/nl_netdev.py
> >  TAP version 13
> >  1..7
> >  ok 1 nl_netdev.empty_check
> >  ok 2 nl_netdev.lo_check
> >  ok 3 nl_netdev.page_pool_check
> >  ok 4 nl_netdev.napi_list_check
> >  ok 5 nl_netdev.dev_set_threaded
> >  ok 6 nl_netdev.napi_set_threaded
> >  ok 7 nl_netdev.nsim_rxq_reset_down
> >  # Totals: pass:7 fail:0 xfail:0 xpass:0 skip:0 error:0
> > 
> >  ./tools/testing/selftests/drivers/net/napi_threaded.py
> >  TAP version 13
> >  1..2
> >  ok 1 napi_threaded.change_num_queues
> >  ok 2 napi_threaded.enable_dev_threaded_disable_napi_threaded
> >  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> > 
> > Fixes: 689883de94dd ("net: stop napi kthreads when THREADED napi is disabled")
> > Signed-off-by: Samiullah Khawaja <skhawaja@google.com>
> 
> With the above addressed, feel free to add:
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> ...

I now see that this patch is present in net as
commit 247981eecd3d ("net: Use NAPI_* in test_bit when stopping napi kthread")

So I think we can safely ignore the comments in my previous email.