Re: [PATCH net-next v2 3/5] netdev_features: convert NETIF_F_LLTX to dev->lltx

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Alexander Lobakin wrote:
> NETIF_F_LLTX can't be changed via Ethtool and is not a feature,
> rather an attribute, very similar to IFF_NO_QUEUE (and hot).
> Free one netdev_features_t bit and make it a "hot" private flag.
> 
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com>

> diff --git a/Documentation/networking/netdev-features.rst b/Documentation/networking/netdev-features.rst
> index d7b15bb64deb..f29d982ebf5d 100644
> --- a/Documentation/networking/netdev-features.rst
> +++ b/Documentation/networking/netdev-features.rst
> @@ -139,14 +139,6 @@ chained skbs (skb->next/prev list).
>  Features contained in NETIF_F_SOFT_FEATURES are features of networking
>  stack. Driver should not change behaviour based on them.
>  
> - * LLTX driver (deprecated for hardware drivers)
> -
> -NETIF_F_LLTX is meant to be used by drivers that don't need locking at all,
> -e.g. software tunnels.
> -
> -This is also used in a few legacy drivers that implement their
> -own locking, don't use it for new (hardware) drivers.
> -
>   * netns-local device
>  
>  NETIF_F_NETNS_LOCAL is set for devices that are not allowed to move between
> diff --git a/Documentation/networking/netdevices.rst b/Documentation/networking/netdevices.rst
> index c2476917a6c3..857c9784f87e 100644
> --- a/Documentation/networking/netdevices.rst
> +++ b/Documentation/networking/netdevices.rst
> @@ -258,11 +258,11 @@ ndo_get_stats:
>  ndo_start_xmit:
>  	Synchronization: __netif_tx_lock spinlock.
>  
> -	When the driver sets NETIF_F_LLTX in dev->features this will be
> +	When the driver sets dev->lltx this will be
>  	called without holding netif_tx_lock. In this case the driver
>  	has to lock by itself when needed.
>  	The locking there should also properly protect against
> -	set_rx_mode. WARNING: use of NETIF_F_LLTX is deprecated.
> +	set_rx_mode. WARNING: use of dev->lltx is deprecated.
>  	Don't use it for new drivers.
>  
>  	Context: Process with BHs disabled or BH (timer),
> diff --git a/drivers/net/ethernet/tehuti/tehuti.h b/drivers/net/ethernet/tehuti/tehuti.h
> index 909e7296cecf..47a2d3e5f8ed 100644
> --- a/drivers/net/ethernet/tehuti/tehuti.h
> +++ b/drivers/net/ethernet/tehuti/tehuti.h

> @@ -23,8 +23,6 @@ enum {
>  	NETIF_F_HW_VLAN_CTAG_FILTER_BIT,/* Receive filtering on VLAN CTAGs */
>  	NETIF_F_VLAN_CHALLENGED_BIT,	/* Device cannot handle VLAN packets */
>  	NETIF_F_GSO_BIT,		/* Enable software GSO. */
> -	NETIF_F_LLTX_BIT,		/* LockLess TX - deprecated. Please */
> -					/* do not use LLTX in new drivers */
>  	NETIF_F_NETNS_LOCAL_BIT,	/* Does not change network namespaces */
>  	NETIF_F_GRO_BIT,		/* Generic receive offload */
>  	NETIF_F_LRO_BIT,		/* large receive offload */

> @@ -1749,6 +1749,8 @@ enum netdev_reg_state {
>   *			booleans combined, only to assert cacheline placement
>   *	@priv_flags:	flags invisible to userspace defined as bits, see
>   *			enum netdev_priv_flags for the definitions
> + *	@lltx:		device supports lockless Tx. Mainly used by logical
> + *			interfaces, such as tunnels

This loses some of the explanation in the NETIF_F_LLTX documentation.

lltx is not deprecated, for software devices, existing documentation
is imprecise on that point. But don't use it for new hardware drivers
should remain clear.

>   *
>   *	@name:	This is the first field of the "visible" part of this structure
>   *		(i.e. as seen by users in the "Space.c" file).  It is the name

> @@ -3098,7 +3098,7 @@ static void amt_link_setup(struct net_device *dev)
>  	dev->hard_header_len	= 0;
>  	dev->addr_len		= 0;
>  	dev->priv_flags		|= IFF_NO_QUEUE;
> -	dev->features		|= NETIF_F_LLTX;
> +	dev->lltx		= true;
>  	dev->features		|= NETIF_F_GSO_SOFTWARE;
>  	dev->features		|= NETIF_F_NETNS_LOCAL;
>  	dev->hw_features	|= NETIF_F_SG | NETIF_F_HW_CSUM;

Since this is an integer type, use 1 instead of true?

Type conversion will convert true to 1. But especially when these are
integer bitfields, relying on conversion is a minor unnecessary risk.

>  int dsa_user_suspend(struct net_device *user_dev)
> diff --git a/net/ethtool/common.c b/net/ethtool/common.c
> index 6b2a360dcdf0..44199d1780d5 100644
> --- a/net/ethtool/common.c
> +++ b/net/ethtool/common.c
> @@ -24,7 +24,6 @@ const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
>  	[NETIF_F_HW_VLAN_STAG_FILTER_BIT] = "rx-vlan-stag-filter",
>  	[NETIF_F_VLAN_CHALLENGED_BIT] =  "vlan-challenged",
>  	[NETIF_F_GSO_BIT] =              "tx-generic-segmentation",
> -	[NETIF_F_LLTX_BIT] =             "tx-lockless",
>  	[NETIF_F_NETNS_LOCAL_BIT] =      "netns-local",
>  	[NETIF_F_GRO_BIT] =              "rx-gro",
>  	[NETIF_F_GRO_HW_BIT] =           "rx-gro-hw",

Is tx-lockless no longer reported after this?

These features should ideally still be reported, even if not part of
the features bitmap in the kernel implementation.

This removal is what you hint at in the cover letter with

  Even shell scripts won't most likely break since the removed bits
  were always read-only, meaning nobody would try touching them from
  a script.

It is a risk. And an avoidable one?