diff mbox series

[net-next,v4,2/6] netdev_features: remove unused __UNUSED_NETIF_F_1

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

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 58 this patch: 58
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 1 of 1 maintainers
netdev/build_clang success Errors and warnings before: 116 this patch: 116
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 4962 this patch: 4962
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 7 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-08-22--03-00 (tests: 711)

Commit Message

Alexander Lobakin Aug. 21, 2024, 3:06 p.m. UTC
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(-)

Comments

Eric Dumazet Aug. 21, 2024, 3:43 p.m. UTC | #1
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.
Alexander Lobakin Aug. 22, 2024, 3:24 p.m. UTC | #2
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
Eric Dumazet Aug. 22, 2024, 4:12 p.m. UTC | #3
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)
Alexander Lobakin Aug. 22, 2024, 4:19 p.m. UTC | #4
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
Jakub Kicinski Aug. 22, 2024, 11:31 p.m. UTC | #5
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?
Alexander Lobakin Aug. 23, 2024, 12:34 p.m. UTC | #6
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
Jakub Kicinski Aug. 24, 2024, 5:43 p.m. UTC | #7
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!
Gal Pressman Aug. 25, 2024, 8:19 a.m. UTC | #8
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?
Jakub Kicinski Aug. 26, 2024, 3:09 p.m. UTC | #9
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
Gal Pressman Aug. 26, 2024, 3:38 p.m. UTC | #10
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 mbox series

Patch

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. */