[PATCH net-next v2] net: Update threaded state in napi config in netif_set_threaded

[PATCH net-next v2] net: Update threaded state in napi config in netif_set_threaded

[Samiullah Khawaja]

Commit 2677010e7793 ("Add support to set NAPI threaded for individual
NAPI") added support to enable/disable threaded napi using netlink. This
also extended the napi config save/restore functionality to set the napi
threaded state. This breaks netdev reset for drivers that use napi
threaded at device level and also use napi config save/restore on
napi_disable/napi_enable. Basically on netdev with napi threaded enabled
at device level, a napi_enable call will get stuck trying to stop the
napi kthread. This is because the napi->config->threaded is set to
disabled when threaded is enabled at device level.

The issue can be reproduced on virtio-net device using qemu. To
reproduce the issue run following,

  echo 1 > /sys/class/net/threaded
  ethtool -L eth0 combined 1

Update the threaded state in napi config in netif_set_threaded and add a
new test that verifies this scenario.

Tested on qemu with virtio-net:
 NETIF=eth0 ./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: 2677010e7793 ("Add support to set NAPI threaded for individual NAPI")
Signed-off-by: Samiullah Khawaja <skhawaja@google.com>

---

v2:
 - Setting napi threaded state in napi config when enabled at device
   level instead of skipping napi_set_threaded in napi_restore_config.
 - Added a new test in tools/testing/selftest/drivers/net.
---
 net/core/dev.c                                |   3 +
 .../selftests/drivers/net/napi_threaded.py    | 108 ++++++++++++++++++
 2 files changed, 111 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/napi_threaded.py

diff --git a/net/core/dev.c b/net/core/dev.c
index 1c6e755841ce..1abba4fc1eec 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7023,6 +7023,9 @@ int netif_set_threaded(struct net_device *dev,
 	 * This should not cause hiccups/stalls to the live traffic.
 	 */
 	list_for_each_entry(napi, &dev->napi_list, dev_list) {
+		if (napi->config)
+			napi->config->threaded = threaded;
+
 		if (!threaded && napi->thread)
 			napi_stop_kthread(napi);
 		else
diff --git a/tools/testing/selftests/drivers/net/napi_threaded.py b/tools/testing/selftests/drivers/net/napi_threaded.py
new file mode 100755
index 000000000000..f2168864568e
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/napi_threaded.py
@@ -0,0 +1,108 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""
+Test napi threaded states.
+"""
+
+from lib.py import ksft_run, ksft_exit
+from lib.py import ksft_eq, ksft_ne
+from lib.py import NetDrvEnv, NetdevFamily
+from lib.py import cmd, defer, ip
+
+
+def _assert_napi_threaded_enabled(nl, napi_id) -> None:
+    napi = nl.napi_get({'id': napi_id})
+    ksft_eq(napi['threaded'], 'enabled')
+    ksft_ne(napi.get('pid'), None)
+
+
+def _assert_napi_threaded_disabled(nl, napi_id) -> None:
+    napi = nl.napi_get({'id': napi_id})
+    ksft_eq(napi['threaded'], 'disabled')
+    ksft_eq(napi.get('pid'), None)
+
+
+def _set_threaded_state(cfg, threaded) -> None:
+    cmd(f"echo {threaded} > /sys/class/net/{cfg.ifname}/threaded")
+
+
+def enable_dev_threaded_disable_napi_threaded(cfg, nl) -> None:
+    """
+    Test that when napi threaded is enabled at device level and
+    then disabled at napi level for one napi, the threaded state
+    of all napis is preserved after a change in number of queues.
+    """
+
+    ip(f"link set dev {cfg.ifname} up")
+
+    napis = nl.napi_get({'ifindex': cfg.ifindex}, dump=True)
+    ksft_eq(len(napis), 2)
+
+    napi0_id = napis[0]['id']
+    napi1_id = napis[1]['id']
+
+    threaded = cmd(f"cat /sys/class/net/{cfg.ifname}/threaded").stdout
+    defer(_set_threaded_state, cfg, threaded)
+
+    # set threaded
+    _set_threaded_state(cfg, 1)
+
+    # check napi threaded is set for both napis
+    _assert_napi_threaded_enabled(nl, napi0_id)
+    _assert_napi_threaded_enabled(nl, napi1_id)
+
+    # disable threaded for napi1
+    nl.napi_set({'id': napi1_id, 'threaded': 'disabled'})
+
+    cmd(f"ethtool -L {cfg.ifname} combined 1")
+    cmd(f"ethtool -L {cfg.ifname} combined 2")
+    _assert_napi_threaded_enabled(nl, napi0_id)
+    _assert_napi_threaded_disabled(nl, napi1_id)
+
+
+def change_num_queues(cfg, nl) -> None:
+    """
+    Test that when napi threaded is enabled at device level,
+    the napi threaded state is preserved after a change in
+    number of queues.
+    """
+
+    ip(f"link set dev {cfg.ifname} up")
+
+    napis = nl.napi_get({'ifindex': cfg.ifindex}, dump=True)
+    ksft_eq(len(napis), 2)
+
+    napi0_id = napis[0]['id']
+    napi1_id = napis[1]['id']
+
+    threaded = cmd(f"cat /sys/class/net/{cfg.ifname}/threaded").stdout
+    defer(_set_threaded_state, cfg, threaded)
+
+    # set threaded
+    _set_threaded_state(cfg, 1)
+
+    # check napi threaded is set for both napis
+    _assert_napi_threaded_enabled(nl, napi0_id)
+    _assert_napi_threaded_enabled(nl, napi1_id)
+
+    cmd(f"ethtool -L {cfg.ifname} combined 1")
+    cmd(f"ethtool -L {cfg.ifname} combined 2")
+
+    # check napi threaded is set for both napis
+    _assert_napi_threaded_enabled(nl, napi0_id)
+    _assert_napi_threaded_enabled(nl, napi1_id)
+
+
+def main() -> None:
+    """ Ksft boiler plate main """
+
+    with NetDrvEnv(__file__, queue_count=2) as cfg:
+        ksft_run([change_num_queues,
+                  enable_dev_threaded_disable_napi_threaded],
+                 args=(cfg, NetdevFamily()))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()

base-commit: fa582ca7e187a15e772e6a72fe035f649b387a60
-- 
2.50.1.552.g942d659e1b-goog

Re: [PATCH net-next v2] net: Update threaded state in napi config in netif_set_threaded

[Jakub Kicinski]

On Wed, 30 Jul 2025 18:25:11 +0000 Samiullah Khawaja wrote:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1c6e755841ce..1abba4fc1eec 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7023,6 +7023,9 @@ int netif_set_threaded(struct net_device *dev,
>  	 * This should not cause hiccups/stalls to the live traffic.
>  	 */
>  	list_for_each_entry(napi, &dev->napi_list, dev_list) {
> +		if (napi->config)
> +			napi->config->threaded = threaded;
> +
>  		if (!threaded && napi->thread)
>  			napi_stop_kthread(napi);
>  		else

Could we possibly just call napi_set_threaded() instead of having the
body of this loop? The first "if (threaded)" in napi_set_threaded()
should do nothing.. and the rest is pretty much copy & paste.
The barrier is not a concern, it's a control path. WDYT?

The test needs to be added to a Makefile, a few more comments on the
contents..

> +def enable_dev_threaded_disable_napi_threaded(cfg, nl) -> None:
> +    """
> +    Test that when napi threaded is enabled at device level and
> +    then disabled at napi level for one napi, the threaded state
> +    of all napis is preserved after a change in number of queues.
> +    """
> +
> +    ip(f"link set dev {cfg.ifname} up")

Why the up? Env should ifup the netdevsim for you..

> +    napis = nl.napi_get({'ifindex': cfg.ifindex}, dump=True)
> +    ksft_eq(len(napis), 2)

So.. the tests under drivers/net are supposed to be run on HW devices
and netdevsim (both must work). Not sure if running this on real HW
adds much value TBH, up to you if you care about that.

If you don't move the test to net/, and use NetdevSimDev() directly.

If you do let's allow any number of queues >= 2, so ksft_ge() here.
Real drivers are unlikely to have exactly 2 queues.

> +    napi0_id = napis[0]['id']
> +    napi1_id = napis[1]['id']
> +
> +    threaded = cmd(f"cat /sys/class/net/{cfg.ifname}/threaded").stdout
> +    defer(_set_threaded_state, cfg, threaded)
> +
> +    # set threaded
> +    _set_threaded_state(cfg, 1)
> +
> +    # check napi threaded is set for both napis
> +    _assert_napi_threaded_enabled(nl, napi0_id)
> +    _assert_napi_threaded_enabled(nl, napi1_id)
> +
> +    # disable threaded for napi1
> +    nl.napi_set({'id': napi1_id, 'threaded': 'disabled'})
> +
> +    cmd(f"ethtool -L {cfg.ifname} combined 1")
> +    cmd(f"ethtool -L {cfg.ifname} combined 2")

And if you decide to keep this in drivers/net it would be good to defer
resetting the queue count to original value too. You can do (untested):

	cur = ethtool(f"-l {cfg.ifname}", json=True).get("combined")

or simply assume there is as many queues as there was napis earlier.

And then a defer(ethtool..

> +    _assert_napi_threaded_enabled(nl, napi0_id)
> +    _assert_napi_threaded_disabled(nl, napi1_id)
-- 
pw-bot: cr

Re: [PATCH net-next v2] net: Update threaded state in napi config in netif_set_threaded

[Samiullah Khawaja]

On Wed, Jul 30, 2025 at 5:55 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed, 30 Jul 2025 18:25:11 +0000 Samiullah Khawaja wrote:
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 1c6e755841ce..1abba4fc1eec 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -7023,6 +7023,9 @@ int netif_set_threaded(struct net_device *dev,
> >        * This should not cause hiccups/stalls to the live traffic.
> >        */
> >       list_for_each_entry(napi, &dev->napi_list, dev_list) {
> > +             if (napi->config)
> > +                     napi->config->threaded = threaded;
> > +
> >               if (!threaded && napi->thread)
> >                       napi_stop_kthread(napi);
> >               else
>
> Could we possibly just call napi_set_threaded() instead of having the
> body of this loop? The first "if (threaded)" in napi_set_threaded()
> should do nothing.. and the rest is pretty much copy & paste.
> The barrier is not a concern, it's a control path. WDYT?
+1
>
> The test needs to be added to a Makefile, a few more comments on the
> contents..
>
> > +def enable_dev_threaded_disable_napi_threaded(cfg, nl) -> None:
> > +    """
> > +    Test that when napi threaded is enabled at device level and
> > +    then disabled at napi level for one napi, the threaded state
> > +    of all napis is preserved after a change in number of queues.
> > +    """
> > +
> > +    ip(f"link set dev {cfg.ifname} up")
>
> Why the up? Env should ifup the netdevsim for you..
Will remove
>
> > +    napis = nl.napi_get({'ifindex': cfg.ifindex}, dump=True)
> > +    ksft_eq(len(napis), 2)
>
> So.. the tests under drivers/net are supposed to be run on HW devices
> and netdevsim (both must work). Not sure if running this on real HW
> adds much value TBH, up to you if you care about that.
The addition of a test to drivers/net was suggested by you in the last
review and I sort of prefer that also as different drivers handle the
netdev resize in a different way, so having this test in drivers/net/
gives more coverage. For example the issue this patch fixes is not
reproducible with NetDevSim.
>
> If you don't move the test to net/, and use NetdevSimDev() directly.
>
> If you do let's allow any number of queues >= 2, so ksft_ge() here.
> Real drivers are unlikely to have exactly 2 queues.
>
> > +    napi0_id = napis[0]['id']
> > +    napi1_id = napis[1]['id']
> > +
> > +    threaded = cmd(f"cat /sys/class/net/{cfg.ifname}/threaded").stdout
> > +    defer(_set_threaded_state, cfg, threaded)
> > +
> > +    # set threaded
> > +    _set_threaded_state(cfg, 1)
> > +
> > +    # check napi threaded is set for both napis
> > +    _assert_napi_threaded_enabled(nl, napi0_id)
> > +    _assert_napi_threaded_enabled(nl, napi1_id)
> > +
> > +    # disable threaded for napi1
> > +    nl.napi_set({'id': napi1_id, 'threaded': 'disabled'})
> > +
> > +    cmd(f"ethtool -L {cfg.ifname} combined 1")
> > +    cmd(f"ethtool -L {cfg.ifname} combined 2")
>
> And if you decide to keep this in drivers/net it would be good to defer
> resetting the queue count to original value too. You can do (untested):
>
>         cur = ethtool(f"-l {cfg.ifname}", json=True).get("combined")
>
> or simply assume there is as many queues as there was napis earlier.
>
> And then a defer(ethtool..
+1 to restore the channel count.


>
> > +    _assert_napi_threaded_enabled(nl, napi0_id)
> > +    _assert_napi_threaded_disabled(nl, napi1_id)
> --
> pw-bot: cr