Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT return

[Ben Hutchings <bhutchings@solarflare.com>]

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.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.