Patchwork ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

login
register
mail settings
Submitter Michał Mirosław
Date May 16, 2011, 1:28 p.m.
Message ID <20110516132807.1A89F13A6A@rere.qmqm.pl>
Download mbox | patch
Permalink /patch/95753/
State Deferred
Delegated to: David Miller
Headers show

Comments

Michał Mirosław - May 16, 2011, 1:28 p.m.
Remove NETIF_F_COMPAT since it's redundant and will be unused after
all drivers are converted to fix/set_features.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---

For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.

 include/linux/ethtool.h |    5 -----
 net/core/ethtool.c      |    2 +-
 2 files changed, 1 insertions(+), 6 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 16, 2011, 1:37 p.m.
On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> Remove NETIF_F_COMPAT since it's redundant and will be unused after
> all drivers are converted to fix/set_features.
> 
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> 
> For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
[...]

ETHTOOL_F_WISH means that the requested features could not all be
enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
remembered.

Ben.
Michał Mirosław - May 16, 2011, 2:23 p.m.
On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > all drivers are converted to fix/set_features.
> > 
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> > 
> > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> [...]
> ETHTOOL_F_WISH means that the requested features could not all be
> enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> remembered.

Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
(net: Implement SFEATURES compatibility for not updated drivers).

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 16, 2011, 2:53 p.m.
On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > all drivers are converted to fix/set_features.
> > > 
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > ---
> > > 
> > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > [...]
> > ETHTOOL_F_WISH means that the requested features could not all be
> > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > remembered.
> 
> Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> (net: Implement SFEATURES compatibility for not updated drivers).

That's also problematic because it means we can't make any use of the
'available' masks from ETHTOOL_GFEATURES.

The patch I sent is actually tested with a modified ethtool.  The
fallback works.  I don't think you've tested whether any of your
proposals can actually practically be used by ethtool.

Ben.
=?ISO-8859-2?Q?Micha=B3_Miros=B3aw?= - May 16, 2011, 3:01 p.m.
W dniu 16 maja 2011 16:53 użytkownik Ben Hutchings
<bhutchings@solarflare.com> napisał:
> On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
>> On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
>> > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
>> > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
>> > > all drivers are converted to fix/set_features.
>> > >
>> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> > > ---
>> > >
>> > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
>> > [...]
>> > ETHTOOL_F_WISH means that the requested features could not all be
>> > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
>> > remembered.
>>
>> Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
>> (net: Implement SFEATURES compatibility for not updated drivers).
> That's also problematic because it means we can't make any use of the
> 'available' masks from ETHTOOL_GFEATURES.
>
> The patch I sent is actually tested with a modified ethtool.  The
> fallback works.  I don't think you've tested whether any of your
> proposals can actually practically be used by ethtool.

You haven't posted the updates to ethtool to netdev. I just work blind here.

For now I use userspace version I sent along some of the first series
of the conversion patches. It actually makes it easier to use if the
compatibility handling stays in kernel (doesn't care about
ETHTOOL_F_COMPAT, BTW).

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 16, 2011, 6:09 p.m.
You guys really need to sort this out properly.

Please resubmit whatever final solution is agreed upon.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław - May 16, 2011, 8:51 p.m.
On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > all drivers are converted to fix/set_features.
> > > > 
> > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > ---
> > > > 
> > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > [...]
> > > ETHTOOL_F_WISH means that the requested features could not all be
> > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > remembered.
> > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > (net: Implement SFEATURES compatibility for not updated drivers).
> That's also problematic because it means we can't make any use of the
> 'available' masks from ETHTOOL_GFEATURES.
> 
> The patch I sent is actually tested with a modified ethtool.  The
> fallback works.  I don't think you've tested whether any of your
> proposals can actually practically be used by ethtool.

While reading your patches I noted some differences in the way we see
the new [GS]FEATURES ops.

First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
feature names become an ABI instead. That's what ETH_SS_FEATURES string
set is for, and that's what comments in kernel's <linux/ethtool.h>
include say.

dev->features are exposed directly by kernel only in two ways:
 1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
    in headers for userspace, this should be treated like a debugging
    facility and not an ABI
 2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
    and RX hashing) that are renamed to ETH_FLAG_* - only those constants
    are in the ABI and only in relation with ETHTOOL_[GS]FLAGS

Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel? The
assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
but there is an conversion layer in kernel that allows old binaries to
work correctly in the common case. (-EOPNOTSUPP is still returned for
drivers which can't change particular feature. The difference is seen
only in that disabling and enabling e.g. checksumming won't disable other
dependent features in the result.)

Right now we already agree that NETIF_F_COMPAT should go.

I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
This might be made even more useful by adding simple wildcard matching.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 16, 2011, 9:08 p.m.
On Mon, 2011-05-16 at 22:51 +0200, Michał Mirosław wrote:
> On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> > On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > > all drivers are converted to fix/set_features.
> > > > > 
> > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > > ---
> > > > > 
> > > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > > [...]
> > > > ETHTOOL_F_WISH means that the requested features could not all be
> > > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > > remembered.
> > > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > > (net: Implement SFEATURES compatibility for not updated drivers).
> > That's also problematic because it means we can't make any use of the
> > 'available' masks from ETHTOOL_GFEATURES.
> > 
> > The patch I sent is actually tested with a modified ethtool.  The
> > fallback works.  I don't think you've tested whether any of your
> > proposals can actually practically be used by ethtool.
> 
> While reading your patches I noted some differences in the way we see
> the new [GS]FEATURES ops.
> 
> First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
> feature names become an ABI instead. That's what ETH_SS_FEATURES string
> set is for, and that's what comments in kernel's <linux/ethtool.h>
> include say.

We've been through this before.  I can't use those names in ethtool
because they aren't the same as ethtool used previously.  I could make
it map strings to strings, but I don't see the point.

> dev->features are exposed directly by kernel only in two ways:
>  1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
>     in headers for userspace, this should be treated like a debugging
>     facility and not an ABI
>  2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
>     and RX hashing) that are renamed to ETH_FLAG_* - only those constants
>     are in the ABI and only in relation with ETHTOOL_[GS]FLAGS
> 
> Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
> that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel?

We must not.

> The
> assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
> but there is an conversion layer in kernel that allows old binaries to
> work correctly in the common case. (-EOPNOTSUPP is still returned for
> drivers which can't change particular feature. The difference is seen
> only in that disabling and enabling e.g. checksumming won't disable other
> dependent features in the result.)
> 
> Right now we already agree that NETIF_F_COMPAT should go.
> 
> I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
> keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
> This might be made even more useful by adding simple wildcard matching.

I've explained before that I do not want to add new options to do
(mostly) the same thing.  Users should have not have to use a different
command depending on the kernel version.

Ben.
Michał Mirosław - May 16, 2011, 9:50 p.m.
On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 22:51 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> > > On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > > > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > > > all drivers are converted to fix/set_features.
> > > > > > 
> > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > > > ---
> > > > > > 
> > > > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > > > [...]
> > > > > ETHTOOL_F_WISH means that the requested features could not all be
> > > > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > > > remembered.
> > > > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > > > (net: Implement SFEATURES compatibility for not updated drivers).
> > > That's also problematic because it means we can't make any use of the
> > > 'available' masks from ETHTOOL_GFEATURES.
> > > 
> > > The patch I sent is actually tested with a modified ethtool.  The
> > > fallback works.  I don't think you've tested whether any of your
> > > proposals can actually practically be used by ethtool.
> > 
> > While reading your patches I noted some differences in the way we see
> > the new [GS]FEATURES ops.
> > 
> > First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
> > feature names become an ABI instead. That's what ETH_SS_FEATURES string
> > set is for, and that's what comments in kernel's <linux/ethtool.h>
> > include say.
> 
> We've been through this before.  I can't use those names in ethtool
> because they aren't the same as ethtool used previously.  I could make
> it map strings to strings, but I don't see the point.
> 
> > dev->features are exposed directly by kernel only in two ways:
> >  1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
> >     in headers for userspace, this should be treated like a debugging
> >     facility and not an ABI
> >  2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
> >     and RX hashing) that are renamed to ETH_FLAG_* - only those constants
> >     are in the ABI and only in relation with ETHTOOL_[GS]FLAGS
> > 
> > Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
> > that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel?
> We must not.

So what's the point in reimplementing old options via ETHTOOL_SFEATURES?

> > The
> > assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
> > but there is an conversion layer in kernel that allows old binaries to
> > work correctly in the common case. (-EOPNOTSUPP is still returned for
> > drivers which can't change particular feature. The difference is seen
> > only in that disabling and enabling e.g. checksumming won't disable other
> > dependent features in the result.)
> > 
> > Right now we already agree that NETIF_F_COMPAT should go.
> > 
> > I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
> > keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
> > This might be made even more useful by adding simple wildcard matching.
> I've explained before that I do not want to add new options to do
> (mostly) the same thing.  Users should have not have to use a different
> command depending on the kernel version.

We can avoid new option by checking feature-strings for unrecognised
arguments to -K. This way, we will have the old options which work
regardless of kernel version ('tx', 'rx', 'sg', etc.) and new options
which need recent kernel anyway (separated 'tx-checksum-*', 'loopback',
others coming in for 2.6.40). Also, this way fallbacks in userspace
are avoided.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 16, 2011, 10:09 p.m.
On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
> > On Mon, 2011-05-16 at 22:51 +0200, Michał Mirosław wrote:
> > > On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> > > > On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > > > > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > > > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > > > > all drivers are converted to fix/set_features.
> > > > > > > 
> > > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > > > > ---
> > > > > > > 
> > > > > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > > > > [...]
> > > > > > ETHTOOL_F_WISH means that the requested features could not all be
> > > > > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > > > > remembered.
> > > > > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > > > > (net: Implement SFEATURES compatibility for not updated drivers).
> > > > That's also problematic because it means we can't make any use of the
> > > > 'available' masks from ETHTOOL_GFEATURES.
> > > > 
> > > > The patch I sent is actually tested with a modified ethtool.  The
> > > > fallback works.  I don't think you've tested whether any of your
> > > > proposals can actually practically be used by ethtool.
> > > 
> > > While reading your patches I noted some differences in the way we see
> > > the new [GS]FEATURES ops.
> > > 
> > > First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
> > > feature names become an ABI instead. That's what ETH_SS_FEATURES string
> > > set is for, and that's what comments in kernel's <linux/ethtool.h>
> > > include say.
> > 
> > We've been through this before.  I can't use those names in ethtool
> > because they aren't the same as ethtool used previously.  I could make
> > it map strings to strings, but I don't see the point.
> > 
> > > dev->features are exposed directly by kernel only in two ways:
> > >  1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
> > >     in headers for userspace, this should be treated like a debugging
> > >     facility and not an ABI
> > >  2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
> > >     and RX hashing) that are renamed to ETH_FLAG_* - only those constants
> > >     are in the ABI and only in relation with ETHTOOL_[GS]FLAGS
> > > 
> > > Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
> > > that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel?
> > We must not.
> 
> So what's the point in reimplementing old options via ETHTOOL_SFEATURES?

Where, in ethtool?  The benefits include:
- Kernel remembers all the features the user wants on, even if the
combination is impossible.  Turning TX checksumming off and on no longer
forces TSO off.
- ethtool can distinguish and report whether a feature is unsupported or
its dependencies are not met.

> > > The
> > > assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
> > > but there is an conversion layer in kernel that allows old binaries to
> > > work correctly in the common case. (-EOPNOTSUPP is still returned for
> > > drivers which can't change particular feature. The difference is seen
> > > only in that disabling and enabling e.g. checksumming won't disable other
> > > dependent features in the result.)
> > > 
> > > Right now we already agree that NETIF_F_COMPAT should go.
> > > 
> > > I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
> > > keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
> > > This might be made even more useful by adding simple wildcard matching.
> > I've explained before that I do not want to add new options to do
> > (mostly) the same thing.  Users should have not have to use a different
> > command depending on the kernel version.
> 
> We can avoid new option by checking feature-strings for unrecognised
> arguments to -K. This way, we will have the old options which work
> regardless of kernel version ('tx', 'rx', 'sg', etc.) and new options
> which need recent kernel anyway (separated 'tx-checksum-*', 'loopback',
> others coming in for 2.6.40).

This is just too subtle a distinction.  It will mostly confuse users.

> Also, this way fallbacks in userspace are avoided.

No, ethtool will be supporting kernels <2.6.40 for many years yet.

Ben.
Michał Mirosław - May 17, 2011, 8:45 a.m.
On Mon, May 16, 2011 at 11:09:35PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
> > > On Mon, 2011-05-16 at 22:51 +0200, Michał Mirosław wrote:
> > > > On Mon, May 16, 2011 at 03:53:17PM +0100, Ben Hutchings wrote:
> > > > > On Mon, 2011-05-16 at 16:23 +0200, Michał Mirosław wrote:
> > > > > > On Mon, May 16, 2011 at 02:37:46PM +0100, Ben Hutchings wrote:
> > > > > > > On Mon, 2011-05-16 at 15:28 +0200, Michał Mirosław wrote:
> > > > > > > > Remove NETIF_F_COMPAT since it's redundant and will be unused after
> > > > > > > > all drivers are converted to fix/set_features.
> > > > > > > > 
> > > > > > > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > > > > > ---
> > > > > > > > 
> > > > > > > > For net as we don't want to have ETHTOOL_F_COMPAT hit stable release.
> > > > > > > [...]
> > > > > > > ETHTOOL_F_WISH means that the requested features could not all be
> > > > > > > enabled, *but are remembered*.  ETHTOOL_F_COMPAT means they were not
> > > > > > > remembered.
> > > > > > Hmm. So, lets just revert 39fc0ce5710c53bad14aaba1a789eec810c556f9
> > > > > > (net: Implement SFEATURES compatibility for not updated drivers).
> > > > > That's also problematic because it means we can't make any use of the
> > > > > 'available' masks from ETHTOOL_GFEATURES.
> > > > > 
> > > > > The patch I sent is actually tested with a modified ethtool.  The
> > > > > fallback works.  I don't think you've tested whether any of your
> > > > > proposals can actually practically be used by ethtool.
> > > > 
> > > > While reading your patches I noted some differences in the way we see
> > > > the new [GS]FEATURES ops.
> > > > 
> > > > First, you make NETIF_F_* flags part of the ethtool ABI. In my approach
> > > > feature names become an ABI instead. That's what ETH_SS_FEATURES string
> > > > set is for, and that's what comments in kernel's <linux/ethtool.h>
> > > > include say.
> > > 
> > > We've been through this before.  I can't use those names in ethtool
> > > because they aren't the same as ethtool used previously.  I could make
> > > it map strings to strings, but I don't see the point.
> > > 
> > > > dev->features are exposed directly by kernel only in two ways:
> > > >  1. /sys/class/net/*/features - since NETIF_F_* flags are not exported
> > > >     in headers for userspace, this should be treated like a debugging
> > > >     facility and not an ABI
> > > >  2. ETHTOOL_[GS]FLAGS - these export 5 flags (LRO, VLAN offload, NTuple,
> > > >     and RX hashing) that are renamed to ETH_FLAG_* - only those constants
> > > >     are in the ABI and only in relation with ETHTOOL_[GS]FLAGS
> > > > 
> > > > Second, you reimplement 'ethtool -K' using ETHTOOL_SFEATURES. Does this mean
> > > > that we want to get rid of ETHTOOL_[GS]{FLAGS,SG,...} from kernel?
> > > We must not.
> > 
> > So what's the point in reimplementing old options via ETHTOOL_SFEATURES?
> 
> Where, in ethtool?  The benefits include:
> - Kernel remembers all the features the user wants on, even if the
> combination is impossible.  Turning TX checksumming off and on no longer
> forces TSO off.

This is what you get by using old ethtool on new kernel. And only for
converted drivers, whether using SFEATURES or old calls.

> - ethtool can distinguish and report whether a feature is unsupported or
> its dependencies are not met.

In this case, when feature is unsupported at all, you still get -EOPNOTSUPP.
If you get no error from old call but after readback (via GSG, etc.) the
feature is still disabled - it means that there are some unmet dependencies.
This is the same information you get from [GS]FEATURES calls.

> > > > The
> > > > assumptions in those calls are a bit different from ETHTOOL_[GS]FEATURES
> > > > but there is an conversion layer in kernel that allows old binaries to
> > > > work correctly in the common case. (-EOPNOTSUPP is still returned for
> > > > drivers which can't change particular feature. The difference is seen
> > > > only in that disabling and enabling e.g. checksumming won't disable other
> > > > dependent features in the result.)
> > > > 
> > > > Right now we already agree that NETIF_F_COMPAT should go.
> > > > 
> > > > I'll send my idea of the ethtool code using ETHTOOL_[GS]FEATURES and
> > > > keeping NETIF_F_* flags internal to the kernel. It adds new modes (-w/-W).
> > > > This might be made even more useful by adding simple wildcard matching.
> > > I've explained before that I do not want to add new options to do
> > > (mostly) the same thing.  Users should have not have to use a different
> > > command depending on the kernel version.
> > 
> > We can avoid new option by checking feature-strings for unrecognised
> > arguments to -K. This way, we will have the old options which work
> > regardless of kernel version ('tx', 'rx', 'sg', etc.) and new options
> > which need recent kernel anyway (separated 'tx-checksum-*', 'loopback',
> > others coming in for 2.6.40).
> This is just too subtle a distinction.  It will mostly confuse users.

We should just document the difference. I expect users who don't care about
new features to not read docs. So 'tx' will still mean 'all TX checksumming'
for them, and they will expect it to turn all TX checksumming
offloads driver supports. If the set changes (like: even in earlier kernels,
some drivers add NETIF_F_SCTP_CSUM to this set) you'll need to update
ethtool userspace. That won't happen if you keep using old calls for
old options as the change will be contained in kernel-side wrapper.

> > Also, this way fallbacks in userspace are avoided.
> No, ethtool will be supporting kernels <2.6.40 for many years yet.

Sure it will. I meant no fallbacks for old options (because they aren't
needed for the tool to work correctly) and no fallback for new options
(as that is not supported in old kernels anyway).

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 18, 2011, 7:02 p.m.
On Mon, 2011-05-16 at 23:09 +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> > On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
[...]
> > > I've explained before that I do not want to add new options to do
> > > (mostly) the same thing.  Users should have not have to use a different
> > > command depending on the kernel version.
> > 
> > We can avoid new option by checking feature-strings for unrecognised
> > arguments to -K. This way, we will have the old options which work
> > regardless of kernel version ('tx', 'rx', 'sg', etc.) and new options
> > which need recent kernel anyway (separated 'tx-checksum-*', 'loopback',
> > others coming in for 2.6.40).
> 
> This is just too subtle a distinction.  It will mostly confuse users.
[...]

Sorry, I think I misunderstood you here.  I agree that new feature names
that do not correspond exactly to existing keywords should be supported
as keywords after the -K option.  I think those that do (e.g.
"tx-udp-fragmentation" vs "ufo") should not be, as adding a
kernel-version-dependent *alias* would be confusing.

I also want users to benefit from your improvements (as I explained
above) even when they use the old names, if they are using a new kernel
version.  That is why I want ethtool to try using ETHTOOL_SFEATURES
first, and why the fallback in the kernel is problematic.

Ben.
Michał Mirosław - May 19, 2011, 9:18 a.m.
On Wed, May 18, 2011 at 08:02:59PM +0100, Ben Hutchings wrote:
> On Mon, 2011-05-16 at 23:09 +0100, Ben Hutchings wrote:
> > On Mon, 2011-05-16 at 23:50 +0200, Michał Mirosław wrote:
> > > On Mon, May 16, 2011 at 10:08:59PM +0100, Ben Hutchings wrote:
> > > > I've explained before that I do not want to add new options to do
> > > > (mostly) the same thing.  Users should have not have to use a different
> > > > command depending on the kernel version.
> > > We can avoid new option by checking feature-strings for unrecognised
> > > arguments to -K. This way, we will have the old options which work
> > > regardless of kernel version ('tx', 'rx', 'sg', etc.) and new options
> > > which need recent kernel anyway (separated 'tx-checksum-*', 'loopback',
> > > others coming in for 2.6.40).
> > This is just too subtle a distinction.  It will mostly confuse users.
> Sorry, I think I misunderstood you here.  I agree that new feature names
> that do not correspond exactly to existing keywords should be supported
> as keywords after the -K option.  I think those that do (e.g.
> "tx-udp-fragmentation" vs "ufo") should not be, as adding a
> kernel-version-dependent *alias* would be confusing.

The alias can be marked as such in the documentation. Shouldn't it be
that hard for a user to read the manpage to know what the new options
are for when he sees them. I don't like the idea of translating strings,
either, because if e.g. ufo becomes split in the feature to ufo4+ufo6
or new checksum offloads are implemented, it will break.

> I also want users to benefit from your improvements (as I explained
> above) even when they use the old names, if they are using a new kernel
> version.  That is why I want ethtool to try using ETHTOOL_SFEATURES
> first, and why the fallback in the kernel is problematic.

Which benefits do you want to have? If checking what other features
changed with selected one, it's easily done by rereading the state -
possibly with GFEATURES.

I'll cook another PoC patch over those I sent to show the idea.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław - May 19, 2011, 10:03 a.m.
On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> You guys really need to sort this out properly.
> Please resubmit whatever final solution is agreed upon.

I noticed that v2.6.39 was tagged today. We should definitely remove
NETIF_F_COMPAT so it won't bite us in the future. The other patch that
fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
in - if we decide that the SFEATURES compatibility should be removed
it won't matter.

Ben, do you agree?

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław - May 24, 2011, 9:14 a.m.
On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > You guys really need to sort this out properly.
> > Please resubmit whatever final solution is agreed upon.
> I noticed that v2.6.39 was tagged today. We should definitely remove
> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> in - if we decide that the SFEATURES compatibility should be removed
> it won't matter.
> 
> Ben, do you agree?

Ping?

http://patchwork.ozlabs.org/patch/95552/
(this is a bugfix, so should go to stable)

http://patchwork.ozlabs.org/patch/95753/
(removes ETHTOOL_F_COMPAT; this we need to decide on)

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - May 24, 2011, 7:39 p.m.
From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Date: Tue, 24 May 2011 11:14:37 +0200

> On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
>> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
>> > You guys really need to sort this out properly.
>> > Please resubmit whatever final solution is agreed upon.
>> I noticed that v2.6.39 was tagged today. We should definitely remove
>> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
>> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
>> in - if we decide that the SFEATURES compatibility should be removed
>> it won't matter.
>> 
>> Ben, do you agree?
> 
> Ping?
> 
> http://patchwork.ozlabs.org/patch/95552/
> (this is a bugfix, so should go to stable)
> 
> http://patchwork.ozlabs.org/patch/95753/
> (removes ETHTOOL_F_COMPAT; this we need to decide on)

You and Ben are still arguing over details.

I want fresh versions of these patches (yes, both of them) once
all of the issues are resolved.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław - May 24, 2011, 9:59 p.m.
On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Date: Tue, 24 May 2011 11:14:37 +0200
> 
> > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> >> > You guys really need to sort this out properly.
> >> > Please resubmit whatever final solution is agreed upon.
> >> I noticed that v2.6.39 was tagged today. We should definitely remove
> >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> >> in - if we decide that the SFEATURES compatibility should be removed
> >> it won't matter.
> >> 
> >> Ben, do you agree?
> > Ping?
> > http://patchwork.ozlabs.org/patch/95552/
> > (this is a bugfix, so should go to stable)
> > 
> > http://patchwork.ozlabs.org/patch/95753/
> > (removes ETHTOOL_F_COMPAT; this we need to decide on)
> You and Ben are still arguing over details.
> 
> I want fresh versions of these patches (yes, both of them) once
> all of the issues are resolved.

We could just wait for 2.6.40 and pretend this code part never existed. ;-)

I'll rebase the first patch tomorrow. Without it the compatibility in
ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 27, 2011, 2:13 p.m.
On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:
> On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Date: Tue, 24 May 2011 11:14:37 +0200
> > 
> > > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> > >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > >> > You guys really need to sort this out properly.
> > >> > Please resubmit whatever final solution is agreed upon.
> > >> I noticed that v2.6.39 was tagged today. We should definitely remove
> > >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> > >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> > >> in - if we decide that the SFEATURES compatibility should be removed
> > >> it won't matter.
> > >> 
> > >> Ben, do you agree?
> > > Ping?
> > > http://patchwork.ozlabs.org/patch/95552/
> > > (this is a bugfix, so should go to stable)
> > > 
> > > http://patchwork.ozlabs.org/patch/95753/
> > > (removes ETHTOOL_F_COMPAT; this we need to decide on)
> > You and Ben are still arguing over details.
> > 
> > I want fresh versions of these patches (yes, both of them) once
> > all of the issues are resolved.
> 
> We could just wait for 2.6.40 and pretend this code part never existed. ;-)

I think I will make ethtool check for a minimum kernel version of 2.6.40
before using ETHTOOL_{G,S}FEATURES.

> I'll rebase the first patch tomorrow. Without it the compatibility in
> ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.

This is an improvement, but I still think the fallback is fundamentally
broken - there's no good way for userland to tell what (if anything)
went wrong when the return value has ETHTOOL_F_COMPAT set.

Ben.
Michał Mirosław - May 27, 2011, 3:28 p.m.
On Fri, May 27, 2011 at 03:13:46PM +0100, Ben Hutchings wrote:
> On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:
> > On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > Date: Tue, 24 May 2011 11:14:37 +0200
> > > > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> > > >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > > >> > You guys really need to sort this out properly.
> > > >> > Please resubmit whatever final solution is agreed upon.
> > > >> I noticed that v2.6.39 was tagged today. We should definitely remove
> > > >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> > > >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> > > >> in - if we decide that the SFEATURES compatibility should be removed
> > > >> it won't matter.
[...]
> > We could just wait for 2.6.40 and pretend this code part never existed. ;-)
> I think I will make ethtool check for a minimum kernel version of 2.6.40
> before using ETHTOOL_{G,S}FEATURES.

> > I'll rebase the first patch tomorrow. Without it the compatibility in
> > ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.
> This is an improvement, but I still think the fallback is fundamentally
> broken - there's no good way for userland to tell what (if anything)
> went wrong when the return value has ETHTOOL_F_COMPAT set.

The same situation happens with ETHTOOL_F_WISH (userspace needs to reread
the features to find out what happened) and with old ETHTOOL_S{TSO,SG,...}
(those return success if any of the features in the group changes and also
posibly disable other features when one is disabled). This wasn't really
checked before.

Ben, I think I commented on your proposal of the userspace part, but I might
have missed some of your arguments about mine. Let's sum those up:

Your version:
   - reimplements ETHTOOL_Sxx via ETHTOOL_SFEATURES in userspace for kernels
     supporting the latter (note: ETHTOOL_S{SG,...} are not ever going away)
   - causes NETIF_F_* to be an ABI
   - does not support new features

My version:
   - implements only new features via ETHTOOL_SFEATURES (old calls are still used)
   - makes feature names an ABI (for scripts actually, not the tool)
   - supports any new features kernel reports without code changes

Both versions are rough at the edges, but working. Both assume that no
behaviour changes are to be made for old '-K' options.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 27, 2011, 3:45 p.m.
On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> On Fri, May 27, 2011 at 03:13:46PM +0100, Ben Hutchings wrote:
> > On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:
> > > On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > Date: Tue, 24 May 2011 11:14:37 +0200
> > > > > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> > > > >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > > > >> > You guys really need to sort this out properly.
> > > > >> > Please resubmit whatever final solution is agreed upon.
> > > > >> I noticed that v2.6.39 was tagged today. We should definitely remove
> > > > >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> > > > >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> > > > >> in - if we decide that the SFEATURES compatibility should be removed
> > > > >> it won't matter.
> [...]
> > > We could just wait for 2.6.40 and pretend this code part never existed. ;-)
> > I think I will make ethtool check for a minimum kernel version of 2.6.40
> > before using ETHTOOL_{G,S}FEATURES.
> 
> > > I'll rebase the first patch tomorrow. Without it the compatibility in
> > > ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.
> > This is an improvement, but I still think the fallback is fundamentally
> > broken - there's no good way for userland to tell what (if anything)
> > went wrong when the return value has ETHTOOL_F_COMPAT set.
> 
> The same situation happens with ETHTOOL_F_WISH (userspace needs to reread
> the features to find out what happened) and with old ETHTOOL_S{TSO,SG,...}
> (those return success if any of the features in the group changes and also
> posibly disable other features when one is disabled). This wasn't really
> checked before.
> 
> Ben, I think I commented on your proposal of the userspace part, but I might
> have missed some of your arguments about mine. Let's sum those up:
> 
> Your version:
>    - reimplements ETHTOOL_Sxx via ETHTOOL_SFEATURES in userspace for kernels
>      supporting the latter

No, it implements 'ethtool -K' using ETHTOOL_SFEATURES.  Maybe you
consider the ethtool utility to be a thin wrapper over the ethtool API,
but that is not my intent.

>      (note: ETHTOOL_S{SG,...} are not ever going away)
>    - causes NETIF_F_* to be an ABI

If feature flag numbers are not stable then what is the point of
/sys/class/net/<name>/features?  Also, I'm not aware that features have
ever been renumbered in the past.

I think ethtool should maintain a feature bitmask rather than the
separate flags it currently does, and I previously attempted this using
a private set of flags.  Shortly afterward that, you proposed to
introduce the new features interfaces, and it seemed to me to make sense
to use the net device feature flags in ethtool.

David, do you think feature flag numbers should be considered a
userspace (i.e. stable) ABI or not?

>    - does not support new features

Not immediately.  I intend to do that afterward.

> My version:
>    - implements only new features via ETHTOOL_SFEATURES (old calls are still used)
>    - makes feature names an ABI (for scripts actually, not the tool)
>    - supports any new features kernel reports without code changes

Right.  I definitely should incorporate your code for looking up
features by string.

> Both versions are rough at the edges, but working. Both assume that no
> behaviour changes are to be made for old '-K' options.

No, my changes are intended to enhance the old options.

Ben.
Michał Mirosław - May 27, 2011, 4:34 p.m.
On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
> On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> > On Fri, May 27, 2011 at 03:13:46PM +0100, Ben Hutchings wrote:
> > > On Tue, 2011-05-24 at 23:59 +0200, Michał Mirosław wrote:
> > > > On Tue, May 24, 2011 at 03:39:30PM -0400, David Miller wrote:
> > > > > From: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > > > > Date: Tue, 24 May 2011 11:14:37 +0200
> > > > > > On Thu, May 19, 2011 at 12:03:31PM +0200, Michał Mirosław wrote:
> > > > > >> On Mon, May 16, 2011 at 02:09:58PM -0400, David Miller wrote:
> > > > > >> > You guys really need to sort this out properly.
> > > > > >> > Please resubmit whatever final solution is agreed upon.
> > > > > >> I noticed that v2.6.39 was tagged today. We should definitely remove
> > > > > >> NETIF_F_COMPAT so it won't bite us in the future. The other patch that
> > > > > >> fixes ethtool_ops->set_flags compatibility is a bugfix, so it should go
> > > > > >> in - if we decide that the SFEATURES compatibility should be removed
> > > > > >> it won't matter.
> > [...]
> > > > We could just wait for 2.6.40 and pretend this code part never existed. ;-)
> > > I think I will make ethtool check for a minimum kernel version of 2.6.40
> > > before using ETHTOOL_{G,S}FEATURES.
> > 
> > > > I'll rebase the first patch tomorrow. Without it the compatibility in
> > > > ETHTOOL_SFEATURES for non-converted drivers is busted wrt set_flags.
> > > This is an improvement, but I still think the fallback is fundamentally
> > > broken - there's no good way for userland to tell what (if anything)
> > > went wrong when the return value has ETHTOOL_F_COMPAT set.
> > 
> > The same situation happens with ETHTOOL_F_WISH (userspace needs to reread
> > the features to find out what happened) and with old ETHTOOL_S{TSO,SG,...}
> > (those return success if any of the features in the group changes and also
> > posibly disable other features when one is disabled). This wasn't really
> > checked before.
> > 
> > Ben, I think I commented on your proposal of the userspace part, but I might
> > have missed some of your arguments about mine. Let's sum those up:
> > 
> > Your version:
> >    - reimplements ETHTOOL_Sxx via ETHTOOL_SFEATURES in userspace for kernels
> >      supporting the latter
> No, it implements 'ethtool -K' using ETHTOOL_SFEATURES.  Maybe you
> consider the ethtool utility to be a thin wrapper over the ethtool API,
> but that is not my intent.

You're right. I assumed just that. -K and other modes of operation look
like they are adapting the ETHTOOL_* calls for human consumption.
There's some additional code for analysing register and other dumps,
but I see it as just merging two tools for convenience.

> >      (note: ETHTOOL_S{SG,...} are not ever going away)
> >    - causes NETIF_F_* to be an ABI
> If feature flag numbers are not stable then what is the point of
> /sys/class/net/<name>/features?  Also, I'm not aware that features have
> ever been renumbered in the past.

Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
is a debugging aid. What is it used for besides that?

[...]
> > Both versions are rough at the edges, but working. Both assume that no
> > behaviour changes are to be made for old '-K' options.
> No, my changes are intended to enhance the old options.

Kernel side already enhances them to not forget other features
'wanted' state. What other enhancements are on your mind?

BTW, I just recalled that ETHTOOL_F_WISH is set only as an information
about bits in features[].valid masks. So zero return does not mean, that
other features (not in .valid masks) haven't changed. This means that
userspace needs to reread features after any SFEATURES call, not just
those returning non-zero.

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - May 27, 2011, 11:25 p.m.
On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
> > On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
[...]
> > >      (note: ETHTOOL_S{SG,...} are not ever going away)
> > >    - causes NETIF_F_* to be an ABI
> > If feature flag numbers are not stable then what is the point of
> > /sys/class/net/<name>/features?  Also, I'm not aware that features have
> > ever been renumbered in the past.
> 
> Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
> is a debugging aid. What is it used for besides that?

xen-api <https://github.com/xen-org/xen-api> uses it in
scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
used for a particularly good reason...

> [...]
> > > Both versions are rough at the edges, but working. Both assume that no
> > > behaviour changes are to be made for old '-K' options.
> > No, my changes are intended to enhance the old options.
> 
> Kernel side already enhances them to not forget other features
> 'wanted' state. What other enhancements are on your mind?

So it does; for some reason I didn't realise that.  Then I suppose there
isn't a benefit from using ETHTOOL_SFEATURES for existing feature flags.

> BTW, I just recalled that ETHTOOL_F_WISH is set only as an information
> about bits in features[].valid masks. So zero return does not mean, that
> other features (not in .valid masks) haven't changed.

That was already true.

> This means that
> userspace needs to reread features after any SFEATURES call, not just
> those returning non-zero.

Not necessarily, though of course it is helpful to report consequential
feature changes.

Ben.
Michał Mirosław - May 28, 2011, 7:35 a.m.
On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
> On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> > On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
> > > On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> [...]
> > > >      (note: ETHTOOL_S{SG,...} are not ever going away)
> > > >    - causes NETIF_F_* to be an ABI
> > > If feature flag numbers are not stable then what is the point of
> > > /sys/class/net/<name>/features?  Also, I'm not aware that features have
> > > ever been renumbered in the past.
> > Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
> > is a debugging aid. What is it used for besides that?
> xen-api <https://github.com/xen-org/xen-api> uses it in
> scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
> used for a particularly good reason...

Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().

Best Regards,
Michał Mirosław

[added Cc: xen-devel]
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell - May 28, 2011, 10:07 a.m.
On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> > > On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
> > > > On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> > [...]
> > > > >      (note: ETHTOOL_S{SG,...} are not ever going away)
> > > > >    - causes NETIF_F_* to be an ABI
> > > > If feature flag numbers are not stable then what is the point of
> > > > /sys/class/net/<name>/features?  Also, I'm not aware that features have
> > > > ever been renumbered in the past.
> > > Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
> > > is a debugging aid. What is it used for besides that?
> > xen-api <https://github.com/xen-org/xen-api> uses it in
> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
> > used for a particularly good reason...
> 
> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().
> 
> Best Regards,
> Michał Mirosław
> 
> [added Cc: xen-devel]

added Cc: xen-api list and dev@openvswitch as well.

Complete thread is at
http://thread.gmane.org/gmane.linux.network/195552/focus=197019

Ian.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Gross - May 28, 2011, 5:31 p.m.
2011/5/28 Ian Campbell <Ian.Campbell@citrix.com>:
> On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
>> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
>> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
>> > > On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
>> > > > On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
>> > [...]
>> > > > >      (note: ETHTOOL_S{SG,...} are not ever going away)
>> > > > >    - causes NETIF_F_* to be an ABI
>> > > > If feature flag numbers are not stable then what is the point of
>> > > > /sys/class/net/<name>/features?  Also, I'm not aware that features have
>> > > > ever been renumbered in the past.
>> > > Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
>> > > is a debugging aid. What is it used for besides that?
>> > xen-api <https://github.com/xen-org/xen-api> uses it in
>> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
>> > used for a particularly good reason...
>>
>> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().

ETHTOOL_GFLAGS didn't expose the vlan acceleration flags until 2.6.37,
which is why /sys/class/net was used instead.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław - May 29, 2011, 9:38 a.m.
On Sat, May 28, 2011 at 10:31:03AM -0700, Jesse Gross wrote:
> 2011/5/28 Ian Campbell <Ian.Campbell@citrix.com>:
> > On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
> >> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
> >> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
> >> > > On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
> >> > > > On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
> >> > [...]
> >> > > > >      (note: ETHTOOL_S{SG,...} are not ever going away)
> >> > > > >    - causes NETIF_F_* to be an ABI
> >> > > > If feature flag numbers are not stable then what is the point of
> >> > > > /sys/class/net/<name>/features?  Also, I'm not aware that features have
> >> > > > ever been renumbered in the past.
> >> > > Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
> >> > > is a debugging aid. What is it used for besides that?
> >> > xen-api <https://github.com/xen-org/xen-api> uses it in
> >> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
> >> > used for a particularly good reason...
> >> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().
> ETHTOOL_GFLAGS didn't expose the vlan acceleration flags until 2.6.37,
> which is why /sys/class/net was used instead.

https://github.com/xen-org/xen-api/commit/78b8078e6ae3cf48179859bed6350bb326987546

The commit using it was introduced after 2.6.37 kernel was released and uses
undocumented acccess path to the bits in question. What is the kernel patch
this commit is referring to?

Best Regards,
Michał Mirosław
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jesse Gross - May 31, 2011, 6:43 p.m.
2011/5/29 Michał Mirosław <mirq-linux@rere.qmqm.pl>:
> On Sat, May 28, 2011 at 10:31:03AM -0700, Jesse Gross wrote:
>> 2011/5/28 Ian Campbell <Ian.Campbell@citrix.com>:
>> > On Sat, 2011-05-28 at 08:35 +0100, Michał Mirosław wrote:
>> >> On Sat, May 28, 2011 at 12:25:55AM +0100, Ben Hutchings wrote:
>> >> > On Fri, 2011-05-27 at 18:34 +0200, Michał Mirosław wrote:
>> >> > > On Fri, May 27, 2011 at 04:45:50PM +0100, Ben Hutchings wrote:
>> >> > > > On Fri, 2011-05-27 at 17:28 +0200, Michał Mirosław wrote:
>> >> > [...]
>> >> > > > >      (note: ETHTOOL_S{SG,...} are not ever going away)
>> >> > > > >    - causes NETIF_F_* to be an ABI
>> >> > > > If feature flag numbers are not stable then what is the point of
>> >> > > > /sys/class/net/<name>/features?  Also, I'm not aware that features have
>> >> > > > ever been renumbered in the past.
>> >> > > Since no NETIF_F_* were exported earlier, I assume /sys/class/net/*/features
>> >> > > is a debugging aid. What is it used for besides that?
>> >> > xen-api <https://github.com/xen-org/xen-api> uses it in
>> >> > scripts/InterfaceReconfigureVswitch.py.  Though it doesn't seem to be
>> >> > used for a particularly good reason...
>> >> Look like it should use ETHTOOL_GFLAGS instead for netdev_has_vlan_accel().
>> ETHTOOL_GFLAGS didn't expose the vlan acceleration flags until 2.6.37,
>> which is why /sys/class/net was used instead.
>
> https://github.com/xen-org/xen-api/commit/78b8078e6ae3cf48179859bed6350bb326987546
>
> The commit using it was introduced after 2.6.37 kernel was released

Well people do use kernels other than the most recently released one...

> and uses
> undocumented acccess path to the bits in question. What is the kernel patch
> this commit is referring to?

It's a temporary workaround to deal with the fact that many drivers
that support vlan acceleration do not properly handle vlan packets if
the corresponding group isn't configured on them.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index dc80d82..5fb916c 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -625,9 +625,6 @@  struct ethtool_sfeatures {
  *      Probably there are other device-specific constraints on some features
  *      in the set. When %ETHTOOL_F_UNSUPPORTED is set, .valid is considered
  *      here as though ignored bits were cleared.
- *   %ETHTOOL_F_COMPAT - some or all changes requested were made by calling
- *      compatibility functions. Requested offload state cannot be properly
- *      managed by kernel.
  *
  * Meaning of bits in the masks are obtained by %ETHTOOL_GSSET_INFO (number of
  * bits in the arrays - always multiple of 32) and %ETHTOOL_GSTRINGS commands
@@ -637,12 +634,10 @@  struct ethtool_sfeatures {
 enum ethtool_sfeatures_retval_bits {
 	ETHTOOL_F_UNSUPPORTED__BIT,
 	ETHTOOL_F_WISH__BIT,
-	ETHTOOL_F_COMPAT__BIT,
 };
 
 #define ETHTOOL_F_UNSUPPORTED   (1 << ETHTOOL_F_UNSUPPORTED__BIT)
 #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
-#define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
 
 #ifdef __KERNEL__
 
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index 74ead9e..dc07569 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -308,7 +308,7 @@  static int ethtool_set_features(struct net_device *dev, void __user *useraddr)
 		return -EINVAL;
 
 	if (ethtool_set_features_compat(dev, features))
-		ret |= ETHTOOL_F_COMPAT;
+		ret |= ETHTOOL_F_WISH;
 
 	if (features[0].valid & ~dev->hw_features) {
 		features[0].valid &= dev->hw_features;