| Message ID | 20101021221015.22906.3516.stgit@jf-dev1-dcblab |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Thu, 2010-10-21 at 15:10 -0700, John Fastabend wrote: > Toggling the vlan tx|rx hw offloads needs to set the hard_header_len > as well otherwise we end up using LL_RESERVED_SPACE incorrectly. > This results in pskb_expand_head() being used unnecessarily. > > This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN > flag and set the header length. [...] Note that not every driver that implements the set_flags operation calls back to ethtool_op_set_flags(). Ben.
On Fri, Oct 22, 2010 at 6:00 AM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Thu, 2010-10-21 at 15:10 -0700, John Fastabend wrote: >> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len >> as well otherwise we end up using LL_RESERVED_SPACE incorrectly. >> This results in pskb_expand_head() being used unnecessarily. >> >> This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN >> flag and set the header length. > [...] > > Note that not every driver that implements the set_flags operation calls > back to ethtool_op_set_flags(). Currently all of the drivers that support toggling this using ethtool call into ethtool_op_set_flags. Even if they don't, things will continue to work correctly, albeit with a performance hit, so it's not a catastrophe. This does assume that drivers which support offloading will start with it enabled. If they don't and just use the non-vlan header length then this will drop the header length down even further when offloading is enabled. All current drivers that support toggling do start with offloading enabled, so maybe it's not that big a deal. Another issue is that cards that don't support vlan offloading at all probably won't take the header into account, so they'll get hit every time. When we are using vlan devices we also manually add the vlan header length but it doesn't update if we change the underlying device. It seems a little redundant to have to do it in both places. I like that this is generic and independent of vlan devices. Hopefully we can figure out these corner cases (or maybe decide that they're not important or this is strictly an improvement). -- 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 10/25/2010 3:45 PM, Jesse Gross wrote: > On Fri, Oct 22, 2010 at 6:00 AM, Ben Hutchings > <bhutchings@solarflare.com> wrote: >> On Thu, 2010-10-21 at 15:10 -0700, John Fastabend wrote: >>> Toggling the vlan tx|rx hw offloads needs to set the hard_header_len >>> as well otherwise we end up using LL_RESERVED_SPACE incorrectly. >>> This results in pskb_expand_head() being used unnecessarily. >>> >>> This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN >>> flag and set the header length. >> [...] >> >> Note that not every driver that implements the set_flags operation calls >> back to ethtool_op_set_flags(). > > Currently all of the drivers that support toggling this using ethtool > call into ethtool_op_set_flags. Even if they don't, things will > continue to work correctly, albeit with a performance hit, so it's not > a catastrophe. > > This does assume that drivers which support offloading will start with > it enabled. If they don't and just use the non-vlan header length > then this will drop the header length down even further when > offloading is enabled. All current drivers that support toggling do > start with offloading enabled, so maybe it's not that big a deal. > > Another issue is that cards that don't support vlan offloading at all > probably won't take the header into account, so they'll get hit every > time. > The lower layer driver should not include the vlan tag in hard_header_len because pkts pushed to the real net device will not add the vlan tag. The vlan device however should increment/dec the len value depending on if the underlying net device is offloading the vlan tagging. > When we are using vlan devices we also manually add the vlan header > length but it doesn't update if we change the underlying device. It > seems a little redundant to have to do it in both places. Right, I think doing this in vlan_transfer_features() should work. > > I like that this is generic and independent of vlan devices. > Hopefully we can figure out these corner cases (or maybe decide that > they're not important or this is strictly an improvement). I'll post an update. Thanks for the comments. -- John > -- > 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 -- 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/net/core/ethtool.c b/net/core/ethtool.c index 956a9f4..4f7fe26 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -21,6 +21,7 @@ #include <linux/uaccess.h> #include <linux/vmalloc.h> #include <linux/slab.h> +#include <linux/if_vlan.h> /* * Some useful ethtool_ops methods that're device independent. @@ -151,6 +152,14 @@ int ethtool_op_set_flags(struct net_device *dev, u32 data, u32 supported) if (data & ~supported) return -EINVAL; + /* is ETH_FLAGS_TXVLAN being toggled */ + if ((dev->features & ETH_FLAG_TXVLAN) ^ (data & ETH_FLAG_TXVLAN)) { + if (data & ETH_FLAG_TXVLAN) + dev->hard_header_len -= VLAN_HLEN; + else + dev->hard_header_len += VLAN_HLEN; + } + dev->features = ((dev->features & ~flags_dup_features) | (data & flags_dup_features)); return 0;
Toggling the vlan tx|rx hw offloads needs to set the hard_header_len as well otherwise we end up using LL_RESERVED_SPACE incorrectly. This results in pskb_expand_head() being used unnecessarily. This add a check in ethtool_op_set_flags to catch the ETH_FLAG_TXVLAN flag and set the header length. Signed-off-by: John Fastabend <john.r.fastabend@intel.com> --- net/core/ethtool.c | 9 +++++++++ 1 files changed, 9 insertions(+), 0 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