| Message ID | 51d2fd4cf6855c2285ca9bcd8d267abedcbc599c.1308596963.git.mirq-linux@rere.qmqm.pl |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: > This is the only place where NETIF_F_* feature flags are exposed > to userspace. Except sysfs. > After this patch feature flags may be changed/reordered freely. [...] Really, what do you think was the point of exposing features through sysfs if they are going to be changed? Oh, but they have been changed already: v2.5.70 net-sysfs added v2.6.15 +#define NETIF_F_UFO 8192 /* Can offload UDP Large Send*/ v2.6.18 -#define NETIF_F_TSO 2048 /* Can offload TCP/IP segmentation */ -#define NETIF_F_UFO 8192 /* Can offload UDP Large Send*/ +#define NETIF_F_GSO 2048 /* Enable software GSO. */ +#define NETIF_F_GSO_SHIFT 16 +#define NETIF_F_TSO (SKB_GSO_TCPV4 << NETIF_F_GSO_SHIFT) +#define NETIF_F_UFO (SKB_GSO_UDP << NETIF_F_GSO_SHIFT) +#define NETIF_F_GSO_ROBUST (SKB_GSO_DODGY << NETIF_F_GSO_SHIFT) +#define NETIF_F_TSO_ECN (SKB_GSO_TCP_ECN << NETIF_F_GSO_SHIFT) +#define NETIF_F_TSO6 (SKB_GSO_TCPV6 << NETIF_F_GSO_SHIFT) v2.6.23 +#define NETIF_F_IPV6_CSUM 16 /* Can checksum TCP/UDP over IPV6 */ +#define NETIF_F_MULTI_QUEUE 16384 /* Has multiple TX/RX queues */ v2.6.24 +#define NETIF_F_NETNS_LOCAL 8192 /* Does not change network namespaces */ v2.6.27 -#define NETIF_F_MULTI_QUEUE 16384 /* Has multiple TX/RX queues */ v2.6.29 +#define NETIF_F_GRO 16384 /* Generic receive offload */ (I've omitted changes that use previously unused bits.) If we're going to keep changing features (maybe even more often) then we have to do something about this sysfs attribute. Maybe get rid of it (as it seems not to be widely used, thankfully). Maybe fix it to use the same feature values as today, but no new features. But certainly don't pretend that feature flags are not exposed. Ben.
From: Ben Hutchings <bhutchings@solarflare.com> Date: Mon, 20 Jun 2011 21:11:21 +0100 > If we're going to keep changing features (maybe even more often) then we > have to do something about this sysfs attribute. Maybe get rid of it > (as it seems not to be widely used, thankfully). Maybe fix it to use > the same feature values as today, but no new features. But certainly > don't pretend that feature flags are not exposed. I'd say we just kill it off, our hands are tied too tightly otherwise. And this ties in to our efforts to expand netdev->features to a larger type. -- 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, 20 Jun 2011 21:11:21 +0100 Ben Hutchings <bhutchings@solarflare.com> wrote: > On Mon, 2011-06-20 at 21:14 +0200, Michał Mirosław wrote: > > This is the only place where NETIF_F_* feature flags are exposed > > to userspace. > > Except sysfs. > > > After this patch feature flags may be changed/reordered freely. > [...] > > Really, what do you think was the point of exposing features through > sysfs if they are going to be changed? > > Oh, but they have been changed already: > > v2.5.70 > net-sysfs added > > v2.6.15 > +#define NETIF_F_UFO 8192 /* Can offload UDP Large Send*/ > > v2.6.18 > -#define NETIF_F_TSO 2048 /* Can offload TCP/IP segmentation */ > -#define NETIF_F_UFO 8192 /* Can offload UDP Large Send*/ > +#define NETIF_F_GSO 2048 /* Enable software GSO. */ > +#define NETIF_F_GSO_SHIFT 16 > +#define NETIF_F_TSO (SKB_GSO_TCPV4 << NETIF_F_GSO_SHIFT) > +#define NETIF_F_UFO (SKB_GSO_UDP << NETIF_F_GSO_SHIFT) > +#define NETIF_F_GSO_ROBUST (SKB_GSO_DODGY << NETIF_F_GSO_SHIFT) > +#define NETIF_F_TSO_ECN (SKB_GSO_TCP_ECN << NETIF_F_GSO_SHIFT) > +#define NETIF_F_TSO6 (SKB_GSO_TCPV6 << NETIF_F_GSO_SHIFT) > > v2.6.23 > +#define NETIF_F_IPV6_CSUM 16 /* Can checksum TCP/UDP over IPV6 */ > +#define NETIF_F_MULTI_QUEUE 16384 /* Has multiple TX/RX queues */ > > v2.6.24 > +#define NETIF_F_NETNS_LOCAL 8192 /* Does not change network namespaces */ > > v2.6.27 > -#define NETIF_F_MULTI_QUEUE 16384 /* Has multiple TX/RX queues */ > > v2.6.29 > +#define NETIF_F_GRO 16384 /* Generic receive offload */ > > (I've omitted changes that use previously unused bits.) > > If we're going to keep changing features (maybe even more often) then we > have to do something about this sysfs attribute. Maybe get rid of it > (as it seems not to be widely used, thankfully). Maybe fix it to use > the same feature values as today, but no new features. But certainly > don't pretend that feature flags are not exposed. > > Ben. > I have no problem with dropping or changing the sysfs feature output. It is useful to have a way to check if device supports something. Sysfs predates the feature support bits of ethtool. As long as ethtool supports it then I am fine with that. It would be nice to have a non-grotty way of doing something like: if ethtool eth0 --supports gro then ... fi -- 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: Stephen Hemminger <shemminger@vyatta.com> Date: Mon, 20 Jun 2011 13:51:09 -0700 > I have no problem with dropping or changing the sysfs feature output. > It is useful to have a way to check if device supports something. These days that's not even where we store the "capabilities" of the device. That happens in ->hw_features now. -- 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-06-20 at 13:52 -0700, David Miller wrote: > From: Stephen Hemminger <shemminger@vyatta.com> > Date: Mon, 20 Jun 2011 13:51:09 -0700 > > > I have no problem with dropping or changing the sysfs feature output. > > It is useful to have a way to check if device supports something. > > These days that's not even where we store the "capabilities" of the > device. > > That happens in ->hw_features now. In fact there never used to be any way for user-space to find out which capabilities were *supported*, other than to try enabling them. Once we work out how to deal with kernel-named features in ethtool it shouldn't be too hard to add options to (quietly) test whether a given feature is supported or enabled. Having done that, we could think about a fallback to sysfs and knowledge of the bit definitions for older kernel versions. Ben.
diff --git a/net/core/ethtool.c b/net/core/ethtool.c index 58d98c3..9214e54 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -240,32 +240,45 @@ static int ethtool_set_one_feature(struct net_device *dev, return 0; } -/* the following list of flags are the same as their associated - * NETIF_F_xxx values in include/linux/netdevice.h - */ -static const u32 flags_dup_features = - (ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | ETH_FLAG_NTUPLE | - ETH_FLAG_RXHASH); +#define ETH_ALL_FLAGS (ETH_FLAG_LRO | ETH_FLAG_RXVLAN | ETH_FLAG_TXVLAN | \ + ETH_FLAG_NTUPLE | ETH_FLAG_RXHASH) +#define ETH_ALL_FEATURES (NETIF_F_LRO | NETIF_F_HW_VLAN_RX | \ + NETIF_F_HW_VLAN_TX | NETIF_F_NTUPLE | NETIF_F_RXHASH) static u32 __ethtool_get_flags(struct net_device *dev) { - return dev->features & flags_dup_features; + u32 flags = 0; + + if (dev->features & NETIF_F_LRO) flags |= ETH_FLAG_LRO; + if (dev->features & NETIF_F_HW_VLAN_RX) flags |= ETH_FLAG_RXVLAN; + if (dev->features & NETIF_F_HW_VLAN_TX) flags |= ETH_FLAG_TXVLAN; + if (dev->features & NETIF_F_NTUPLE) flags |= ETH_FLAG_NTUPLE; + if (dev->features & NETIF_F_RXHASH) flags |= ETH_FLAG_RXHASH; + + return flags; } static int __ethtool_set_flags(struct net_device *dev, u32 data) { + u32 features = 0; u32 changed; - if (data & ~flags_dup_features) + if (data & ~ETH_ALL_FLAGS) return -EINVAL; + if (data & ETH_FLAG_LRO) features |= NETIF_F_LRO; + if (data & ETH_FLAG_RXVLAN) features |= NETIF_F_HW_VLAN_RX; + if (data & ETH_FLAG_TXVLAN) features |= NETIF_F_HW_VLAN_TX; + if (data & ETH_FLAG_NTUPLE) features |= NETIF_F_NTUPLE; + if (data & ETH_FLAG_RXHASH) features |= NETIF_F_RXHASH; + /* allow changing only bits set in hw_features */ - changed = (data ^ dev->features) & flags_dup_features; + changed = (features ^ dev->features) & ETH_ALL_FEATURES; if (changed & ~dev->hw_features) return (changed & dev->hw_features) ? -EINVAL : -EOPNOTSUPP; dev->wanted_features = - (dev->wanted_features & ~changed) | (data & dev->hw_features); + (dev->wanted_features & ~changed) | (features & dev->hw_features); __netdev_update_features(dev);
This is the only place where NETIF_F_* feature flags are exposed to userspace. After this patch feature flags may be changed/reordered freely. Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl> --- net/core/ethtool.c | 33 +++++++++++++++++++++++---------- 1 files changed, 23 insertions(+), 10 deletions(-)