Message-ID: <20110517084543.GB18423@rere.qmqm.pl>
Date:	Tue, 17 May 2011 10:45:43 +0200
From:	Michał Mirosław <mirq-linux@...e.qmqm.pl>
To:	Ben Hutchings <bhutchings@...arflare.com>
Cc:	netdev@...r.kernel.org, David Miller <davem@...emloft.net>
Subject: Re: [PATCH] ethtool: ETHTOOL_SFEATURES: remove NETIF_F_COMPAT
 return

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@...e.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists