netdev - Re: [PATCH net-next v5 2/4] net: define an enum for the napi threaded state

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: <aArL5Lac5evBAvz1@LQ3V64L9R2>
Date: Thu, 24 Apr 2025 16:40:20 -0700
From: Joe Damato <jdamato@...tly.com>
To: Samiullah Khawaja <skhawaja@...gle.com>
Cc: Jakub Kicinski <kuba@...nel.org>,
	"David S . Miller " <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
	almasrymina@...gle.com, willemb@...gle.com, mkarsten@...terloo.ca,
	netdev@...r.kernel.org
Subject: Re: [PATCH net-next v5 2/4] net: define an enum for the napi
 threaded state

On Thu, Apr 24, 2025 at 08:02:20PM +0000, Samiullah Khawaja wrote:
> Instead of using '0' and '1' for napi threaded state, use an enum with
> 'disable' and 'enable' states, preparing for the next patch to add a new
> 'busy-poll-enable' state. Also update the 'threaded' field in struct
> net_device to u8 instead of bool.

[...]

> 
> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> index c9d190fe1f05..c8834161e8ec 100644
> --- a/Documentation/netlink/specs/netdev.yaml
> +++ b/Documentation/netlink/specs/netdev.yaml

[...]

> @@ -283,11 +287,10 @@ attribute-sets:
>        -
>          name: threaded
>          doc: Whether the napi is configured to operate in threaded polling
> -             mode. If this is set to `1` then the NAPI context operates
> +             mode. If this is set to `enable` then the NAPI context operates
>               in threaded polling mode.
> -        type: uint
> -        checks:
> -          max: 1
> +        type: u32
> +        enum: napi-threaded

I think this can still be uint even if associated with an enum? IIUC
(I could be wrong) uint is preferred.

> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 3817720e8b24..2eda563307f9 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h

> @@ -2428,7 +2429,7 @@ struct net_device {
>  	struct sfp_bus		*sfp_bus;
>  	struct lock_class_key	*qdisc_tx_busylock;
>  	bool			proto_down;
> -	bool			threaded;
> +	u8			threaded;

Looks like there's a 1 byte hole in a cacheline further up after

  unsigned char              lower_level

Not sure if putting the u8 there and filling that cacheline makes you
feel anything in particular.

(I feel nothing.)

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3ff275bbf6e2..41d809f2a7f7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c

[...]

> @@ -6924,12 +6938,15 @@ int dev_set_threaded(struct net_device *dev, bool threaded)
>  	if (dev->threaded == threaded)
>  		return 0;
>  
> +	val = 0;
>  	if (threaded) {
> +		val |= NAPIF_STATE_THREADED;
> +
>  		list_for_each_entry(napi, &dev->napi_list, dev_list) {
>  			if (!napi->thread) {
>  				err = napi_kthread_create(napi);
>  				if (err) {
> -					threaded = false;
> +					threaded = NETDEV_NAPI_THREADED_DISABLE;
>  					break;
>  				}
>  			}

Re the feedback on the per-NAPI threading setting patch, I think if
you used napi_set_threaded in dev_set_threaded (as mentioned in the
feedback to that patch) you might reduce the changes here.