| Message ID | 20110516132807.1A89F13A6A@rere.qmqm.pl |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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.
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
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
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
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.
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
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.
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
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.
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
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
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
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
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
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.
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
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.
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
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.
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
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
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
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
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
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;
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