| Message ID | 20240821150700.1760518-3-aleksander.lobakin@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdev_features: start cleaning netdev_features_t up | expand |
On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 > ("net: remove NETIF_F_NO_CSUM feature bit") and became > __UNUSED_NETIF_F_1. It's not used anywhere in the code. > Remove this bit waste. > > It wasn't needed to rename the flag instead of removing it as > netdev features are not uAPI/ABI. Ethtool passes their names > and values separately with no fixed positions and the userspace > Ethtool code doesn't have any hardcoded feature names/bits, so > that new Ethtool will work on older kernels and vice versa. This is only true for recent enough ethtool (>= 3.4) You might refine the changelog to not claim this "was not needed". Back in 2011 (and linux-2.6.39) , this was needed for sure. I am not sure we have a documented requirement about ethtool versions.
From: Eric Dumazet <edumazet@google.com> Date: Wed, 21 Aug 2024 17:43:16 +0200 > On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin > <aleksander.lobakin@intel.com> wrote: >> >> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 >> ("net: remove NETIF_F_NO_CSUM feature bit") and became >> __UNUSED_NETIF_F_1. It's not used anywhere in the code. >> Remove this bit waste. >> >> It wasn't needed to rename the flag instead of removing it as >> netdev features are not uAPI/ABI. Ethtool passes their names >> and values separately with no fixed positions and the userspace >> Ethtool code doesn't have any hardcoded feature names/bits, so >> that new Ethtool will work on older kernels and vice versa. > > This is only true for recent enough ethtool (>= 3.4) > > You might refine the changelog to not claim this "was not needed". > > Back in 2011 (and linux-2.6.39) , this was needed for sure. > > I am not sure we have a documented requirement about ethtool versions. But how then Ethtool < 3.4 works with the latest kernels? I believe we already moved some bits and/or removed some features or it's not true? I could try building it, not sure it would build though. How do you think then we should approach this? Maybe document the requirement? I don't think we should leave the features as they are and sit with no bits available only to support ancient Ethtool versions. Thanks, Olek
On Thu, Aug 22, 2024 at 5:24 PM Alexander Lobakin <aleksander.lobakin@intel.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > Date: Wed, 21 Aug 2024 17:43:16 +0200 > > > On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin > > <aleksander.lobakin@intel.com> wrote: > >> > >> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 > >> ("net: remove NETIF_F_NO_CSUM feature bit") and became > >> __UNUSED_NETIF_F_1. It's not used anywhere in the code. > >> Remove this bit waste. > >> > >> It wasn't needed to rename the flag instead of removing it as > >> netdev features are not uAPI/ABI. Ethtool passes their names > >> and values separately with no fixed positions and the userspace > >> Ethtool code doesn't have any hardcoded feature names/bits, so > >> that new Ethtool will work on older kernels and vice versa. > > > > This is only true for recent enough ethtool (>= 3.4) > > > > You might refine the changelog to not claim this "was not needed". > > > > Back in 2011 (and linux-2.6.39) , this was needed for sure. > > > > I am not sure we have a documented requirement about ethtool versions. > > But how then Ethtool < 3.4 works with the latest kernels? I believe we > already moved some bits and/or removed some features or it's not true? > Presumably most of the 'old and useful' bits are at the same location, or ethtool has been updated by distros. > I could try building it, not sure it would build though. How do you > think then we should approach this? Maybe document the requirement? > I don't think we should leave the features as they are and sit with no > bits available only to support ancient Ethtool versions. I was simply suggesting to correct the changelog, and make clear we need a recent enough ethtool. We can not simply say that ethtool always supported the modern way (ETH_SS_FEATURES)
From: Eric Dumazet <edumazet@google.com> Date: Thu, 22 Aug 2024 18:12:18 +0200 > On Thu, Aug 22, 2024 at 5:24 PM Alexander Lobakin > <aleksander.lobakin@intel.com> wrote: >> >> From: Eric Dumazet <edumazet@google.com> >> Date: Wed, 21 Aug 2024 17:43:16 +0200 >> >>> On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin >>> <aleksander.lobakin@intel.com> wrote: >>>> >>>> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 >>>> ("net: remove NETIF_F_NO_CSUM feature bit") and became >>>> __UNUSED_NETIF_F_1. It's not used anywhere in the code. >>>> Remove this bit waste. >>>> >>>> It wasn't needed to rename the flag instead of removing it as >>>> netdev features are not uAPI/ABI. Ethtool passes their names >>>> and values separately with no fixed positions and the userspace >>>> Ethtool code doesn't have any hardcoded feature names/bits, so >>>> that new Ethtool will work on older kernels and vice versa. >>> >>> This is only true for recent enough ethtool (>= 3.4) >>> >>> You might refine the changelog to not claim this "was not needed". >>> >>> Back in 2011 (and linux-2.6.39) , this was needed for sure. >>> >>> I am not sure we have a documented requirement about ethtool versions. >> >> But how then Ethtool < 3.4 works with the latest kernels? I believe we >> already moved some bits and/or removed some features or it's not true? >> > > Presumably most of the 'old and useful' bits are at the same location, > or ethtool has been updated by distros. > >> I could try building it, not sure it would build though. How do you >> think then we should approach this? Maybe document the requirement? >> I don't think we should leave the features as they are and sit with no >> bits available only to support ancient Ethtool versions. > > I was simply suggesting to correct the changelog, and make clear we > need a recent enough ethtool. Yeah I got it, thanks. Will reword. > > We can not simply say that ethtool always supported the modern way > (ETH_SS_FEATURES) I didn't work with Linux at all back in 2011, so I didn't even know there were older ways of handling this :D Always something to learn, nice. Thanks, Olek
On Thu, 22 Aug 2024 18:19:24 +0200 Alexander Lobakin wrote: > > I was simply suggesting to correct the changelog, and make clear we > > need a recent enough ethtool. > > Yeah I got it, thanks. Will reword. > > > We can not simply say that ethtool always supported the modern way > > (ETH_SS_FEATURES) > > I didn't work with Linux at all back in 2011, so I didn't even know > there were older ways of handling this :D Always something to learn, nice. Are we removing the bit definitions just for code cleanliness? On one hand it may be good to make any potential breakage obvious, on the other we could avoid regressions if we stick to reserving the bits, and reusing them, but the bits we don't delete could remain at their current position?
From: Jakub Kicinski <kuba@kernel.org> Date: Thu, 22 Aug 2024 16:31:29 -0700 > On Thu, 22 Aug 2024 18:19:24 +0200 Alexander Lobakin wrote: >>> I was simply suggesting to correct the changelog, and make clear we >>> need a recent enough ethtool. >> >> Yeah I got it, thanks. Will reword. >> >>> We can not simply say that ethtool always supported the modern way >>> (ETH_SS_FEATURES) >> >> I didn't work with Linux at all back in 2011, so I didn't even know >> there were older ways of handling this :D Always something to learn, nice. > > Are we removing the bit definitions just for code cleanliness? Uhm, no, to free more bits to be able to add new features. > On one hand it may be good to make any potential breakage obvious, > on the other we could avoid regressions if we stick to reserving > the bits, and reusing them, but the bits we don't delete could remain > at their current position? Hmm, sounds fine. IOW just rename all the bits I remove to __UNUSED_NETIF_F_xx? Thanks, Olek
On Fri, 23 Aug 2024 14:34:13 +0200 Alexander Lobakin wrote: > > On one hand it may be good to make any potential breakage obvious, > > on the other we could avoid regressions if we stick to reserving > > the bits, and reusing them, but the bits we don't delete could remain > > at their current position? > > Hmm, sounds fine. IOW just rename all the bits I remove to > __UNUSED_NETIF_F_xx? Yup!
On 21/08/2024 18:43, Eric Dumazet wrote: > On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin > <aleksander.lobakin@intel.com> wrote: >> >> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 >> ("net: remove NETIF_F_NO_CSUM feature bit") and became >> __UNUSED_NETIF_F_1. It's not used anywhere in the code. >> Remove this bit waste. >> >> It wasn't needed to rename the flag instead of removing it as >> netdev features are not uAPI/ABI. Ethtool passes their names >> and values separately with no fixed positions and the userspace >> Ethtool code doesn't have any hardcoded feature names/bits, so >> that new Ethtool will work on older kernels and vice versa. > > This is only true for recent enough ethtool (>= 3.4) > > You might refine the changelog to not claim this "was not needed". > > Back in 2011 (and linux-2.6.39) , this was needed for sure. > > I am not sure we have a documented requirement about ethtool versions. > This is a nice history lesson, so before the features infrastructure the feature bits were considered as "ABI"? I couldn't find a point in time where they were actually defined in the uapi files?
On Sun, 25 Aug 2024 11:19:49 +0300 Gal Pressman wrote: > On 21/08/2024 18:43, Eric Dumazet wrote: > > On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin > > <aleksander.lobakin@intel.com> wrote: > >> > >> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 > >> ("net: remove NETIF_F_NO_CSUM feature bit") and became > >> __UNUSED_NETIF_F_1. It's not used anywhere in the code. > >> Remove this bit waste. > >> > >> It wasn't needed to rename the flag instead of removing it as > >> netdev features are not uAPI/ABI. Ethtool passes their names > >> and values separately with no fixed positions and the userspace > >> Ethtool code doesn't have any hardcoded feature names/bits, so > >> that new Ethtool will work on older kernels and vice versa. > > > > This is only true for recent enough ethtool (>= 3.4) > > > > You might refine the changelog to not claim this "was not needed". > > > > Back in 2011 (and linux-2.6.39) , this was needed for sure. > > > > I am not sure we have a documented requirement about ethtool versions. > > > > This is a nice history lesson, so before the features infrastructure the > feature bits were considered as "ABI"? > > I couldn't find a point in time where they were actually defined in the > uapi files? Keep in mind that include/uapi was introduced around v3.7, before that IIUC everything under include/linux that wasn't protected by ifdef __KERNEL__ was uAPI. So all of include/linux/netdev_features.h
On 26/08/2024 18:09, Jakub Kicinski wrote: > On Sun, 25 Aug 2024 11:19:49 +0300 Gal Pressman wrote: >> On 21/08/2024 18:43, Eric Dumazet wrote: >>> On Wed, Aug 21, 2024 at 5:07 PM Alexander Lobakin >>> <aleksander.lobakin@intel.com> wrote: >>>> >>>> NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 >>>> ("net: remove NETIF_F_NO_CSUM feature bit") and became >>>> __UNUSED_NETIF_F_1. It's not used anywhere in the code. >>>> Remove this bit waste. >>>> >>>> It wasn't needed to rename the flag instead of removing it as >>>> netdev features are not uAPI/ABI. Ethtool passes their names >>>> and values separately with no fixed positions and the userspace >>>> Ethtool code doesn't have any hardcoded feature names/bits, so >>>> that new Ethtool will work on older kernels and vice versa. >>> >>> This is only true for recent enough ethtool (>= 3.4) >>> >>> You might refine the changelog to not claim this "was not needed". >>> >>> Back in 2011 (and linux-2.6.39) , this was needed for sure. >>> >>> I am not sure we have a documented requirement about ethtool versions. >>> >> >> This is a nice history lesson, so before the features infrastructure the >> feature bits were considered as "ABI"? >> >> I couldn't find a point in time where they were actually defined in the >> uapi files? > > Keep in mind that include/uapi was introduced around v3.7, before > that IIUC everything under include/linux that wasn't protected by > ifdef __KERNEL__ was uAPI. So all of include/linux/netdev_features.h TIL, thanks!
diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h index 7c2d77d75a88..44c428d62db4 100644 --- a/include/linux/netdev_features.h +++ b/include/linux/netdev_features.h @@ -14,7 +14,6 @@ typedef u64 netdev_features_t; enum { NETIF_F_SG_BIT, /* Scatter/gather IO. */ NETIF_F_IP_CSUM_BIT, /* Can checksum TCP/UDP over IPv4. */ - __UNUSED_NETIF_F_1, NETIF_F_HW_CSUM_BIT, /* Can checksum all the packets. */ NETIF_F_IPV6_CSUM_BIT, /* Can checksum TCP/UDP over IPV6 */ NETIF_F_HIGHDMA_BIT, /* Can DMA to high memory. */
NETIF_F_NO_CSUM was removed in 3.2-rc2 by commit 34324dc2bf27 ("net: remove NETIF_F_NO_CSUM feature bit") and became __UNUSED_NETIF_F_1. It's not used anywhere in the code. Remove this bit waste. It wasn't needed to rename the flag instead of removing it as netdev features are not uAPI/ABI. Ethtool passes their names and values separately with no fixed positions and the userspace Ethtool code doesn't have any hardcoded feature names/bits, so that new Ethtool will work on older kernels and vice versa. Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> --- include/linux/netdev_features.h | 1 - 1 file changed, 1 deletion(-)