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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250730175541.37cfac15@kernel.org>
Date: Wed, 30 Jul 2025 17:55:41 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Samiullah Khawaja <skhawaja@...gle.com>
Cc: "David S . Miller " <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>, willemb@...gle.com,
 almasrymina@...gle.com, netdev@...r.kernel.org, Joe Damato <joe@...a.to>
Subject: Re: [PATCH net-next v2] net: Update threaded state in napi config
 in netif_set_threaded

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