Message ID | 1295308844.3362.135.camel@edumazet-laptop |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Tue, 2011-01-18 at 01:00 +0100, Eric Dumazet wrote: > Le lundi 17 janvier 2011 à 23:47 +0000, Ben Hutchings a écrit : > > On Tue, 2011-01-18 at 00:41 +0100, Eric Dumazet wrote: > > > Oh well, my dev machine, with bnx2 and tg3 NICs, isnt working at all > > > with current linux-2.6 tree (I need vlans on my setup) > > > > > > tg3 : vlan not working > > > > > > bnx2 : vlan + TSO not working (or very slowly, since only non GSO frames > > > are OK) > > > > > > I suspect recent commits from Jesse are the problem. > > > (bnx2_xmit() is feeded with zeroed vlan_tci skbs) > > > > > > Maybe f01a5236bd4b1401 (net offloading: Generalize > > > netif_get_vlan_features().) ? > > > > > > I dont see NETIF_F_HW_VLAN_TX being set in vlan_features in net drivers. > > > They only set this flag in dev->features > > > > > > I dont think changing all drivers to also set vlan_features makes sense. > > > > > > Is following patch the right path ? (It does solve my problem) > > > > This isn't right. NETIF_F_HW_VLAN_TX in vlan_features would mean that > > the hardware can do two levels of VLAN tag insertion, which is generally > > not true. > > > > So should we revert part of Jesse patch ? I want my vlan back ;) Yeah, that looks right. Ben. > diff --git a/net/core/dev.c b/net/core/dev.c > index 54277df..8209d93 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -2076,7 +2076,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, > features = netif_skb_features(skb); > > if (vlan_tx_tag_present(skb) && > - !(features & NETIF_F_HW_VLAN_TX)) { > + !(dev->features & NETIF_F_HW_VLAN_TX)) { > skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb)); > if (unlikely(!skb)) > goto out; > >
On Mon, Jan 17, 2011 at 4:09 PM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Tue, 2011-01-18 at 01:00 +0100, Eric Dumazet wrote: >> Le lundi 17 janvier 2011 à 23:47 +0000, Ben Hutchings a écrit : >> > On Tue, 2011-01-18 at 00:41 +0100, Eric Dumazet wrote: >> > > Oh well, my dev machine, with bnx2 and tg3 NICs, isnt working at all >> > > with current linux-2.6 tree (I need vlans on my setup) >> > > >> > > tg3 : vlan not working >> > > >> > > bnx2 : vlan + TSO not working (or very slowly, since only non GSO frames >> > > are OK) >> > > >> > > I suspect recent commits from Jesse are the problem. >> > > (bnx2_xmit() is feeded with zeroed vlan_tci skbs) >> > > >> > > Maybe f01a5236bd4b1401 (net offloading: Generalize >> > > netif_get_vlan_features().) ? >> > > >> > > I dont see NETIF_F_HW_VLAN_TX being set in vlan_features in net drivers. >> > > They only set this flag in dev->features >> > > >> > > I dont think changing all drivers to also set vlan_features makes sense. >> > > >> > > Is following patch the right path ? (It does solve my problem) >> > >> > This isn't right. NETIF_F_HW_VLAN_TX in vlan_features would mean that >> > the hardware can do two levels of VLAN tag insertion, which is generally >> > not true. >> > >> >> So should we revert part of Jesse patch ? I want my vlan back ;) > > Yeah, that looks right. I think it is better for netif_skb_features() to actually return the correct features rather than bypass it here. NETIF_F_HW_VLAN_TX doesn't depend on any other offloads, so we can always include it if it is in dev->features. Separately, this means there is a problem with bnx2 because it allows vlan insertion to be turned off, which would have the same effect. Maybe it is looking directly at skb->protocol or similar for TSO. -- 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: Jesse Gross <jesse@nicira.com> Date: Mon, 17 Jan 2011 16:13:18 -0800 > I think it is better for netif_skb_features() to actually return the > correct features rather than bypass it here. NETIF_F_HW_VLAN_TX > doesn't depend on any other offloads, so we can always include it if > it is in dev->features. > > Separately, this means there is a problem with bnx2 because it allows > vlan insertion to be turned off, which would have the same effect. > Maybe it is looking directly at skb->protocol or similar for TSO. Please, someone cons up an acceptable fix fast. 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
Le lundi 17 janvier 2011 à 22:09 -0800, David Miller a écrit : > From: Jesse Gross <jesse@nicira.com> > Date: Mon, 17 Jan 2011 16:13:18 -0800 > > > I think it is better for netif_skb_features() to actually return the > > correct features rather than bypass it here. NETIF_F_HW_VLAN_TX > > doesn't depend on any other offloads, so we can always include it if > > it is in dev->features. > > > > Separately, this means there is a problem with bnx2 because it allows > > vlan insertion to be turned off, which would have the same effect. > > Maybe it is looking directly at skb->protocol or similar for TSO. > > Please, someone cons up an acceptable fix fast. > I just woke up, and honestly dont understand why only bnx2 is affected by this problem of masking NETIF_F_HW_VLAN_TX And I dont understand all this netif_skb_features() stuff : if we want to actually test dev->features & NETIF_F_HW_VLAN_TX, and this flag doesnt depend on other offloads, why are we doing features & vlan_features. Jesse, I dont understand why you say "bnx2 allows vlan insertion to be turned off". Really. -- 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, Jan 18, 2011 at 1:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > Le lundi 17 janvier 2011 à 22:09 -0800, David Miller a écrit : >> From: Jesse Gross <jesse@nicira.com> >> Date: Mon, 17 Jan 2011 16:13:18 -0800 >> >> > I think it is better for netif_skb_features() to actually return the >> > correct features rather than bypass it here. NETIF_F_HW_VLAN_TX >> > doesn't depend on any other offloads, so we can always include it if >> > it is in dev->features. >> > >> > Separately, this means there is a problem with bnx2 because it allows >> > vlan insertion to be turned off, which would have the same effect. >> > Maybe it is looking directly at skb->protocol or similar for TSO. >> >> Please, someone cons up an acceptable fix fast. >> > > I just woke up, and honestly dont understand why only bnx2 is affected > by this problem of masking NETIF_F_HW_VLAN_TX If NETIF_F_HW_VLAN_TX is masked then the tag will be inserted in software, which is generally OK. The problem is that some drivers assume that if they can do vlan tagging in hardware it will always be used and therefore don't expect vlan tags when setting up TSO, etc. > > And I dont understand all this netif_skb_features() stuff : if we want > to actually test dev->features & NETIF_F_HW_VLAN_TX, and this flag > doesnt depend on other offloads, why are we doing features & > vlan_features. The idea is to put all of the logic in one place rather than having pieces that are really interdependent scattered around in the different offloads. So we could test dev->features directly for vlans but I would rather just have netif_skb_features() return the right values to start off with. > > Jesse, I dont understand why you say "bnx2 allows vlan insertion to be > turned off". Really. You can disable it using Ethtool, which will turn off NETIF_F_HW_VLAN_TX the same as this bug. I'm running a quick test on a patch to always allow NETIF_F_HW_VLAN_TX to be returned from netif_skb_features(). -- 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
Le mardi 18 janvier 2011 à 01:32 -0500, Jesse Gross a écrit : > On Tue, Jan 18, 2011 at 1:21 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > Le lundi 17 janvier 2011 à 22:09 -0800, David Miller a écrit : > >> From: Jesse Gross <jesse@nicira.com> > >> Date: Mon, 17 Jan 2011 16:13:18 -0800 > >> > >> > I think it is better for netif_skb_features() to actually return the > >> > correct features rather than bypass it here. NETIF_F_HW_VLAN_TX > >> > doesn't depend on any other offloads, so we can always include it if > >> > it is in dev->features. > >> > > >> > Separately, this means there is a problem with bnx2 because it allows > >> > vlan insertion to be turned off, which would have the same effect. > >> > Maybe it is looking directly at skb->protocol or similar for TSO. > >> > >> Please, someone cons up an acceptable fix fast. > >> > > > > I just woke up, and honestly dont understand why only bnx2 is affected > > by this problem of masking NETIF_F_HW_VLAN_TX > > If NETIF_F_HW_VLAN_TX is masked then the tag will be inserted in > software, which is generally OK. The problem is that some drivers > assume that if they can do vlan tagging in hardware it will always be > used and therefore don't expect vlan tags when setting up TSO, etc. > OK, but then this driver gave us the hint core network was actually always doing software vlan, tagging ;) > > > > And I dont understand all this netif_skb_features() stuff : if we want > > to actually test dev->features & NETIF_F_HW_VLAN_TX, and this flag > > doesnt depend on other offloads, why are we doing features & > > vlan_features. > > The idea is to put all of the logic in one place rather than having > pieces that are really interdependent scattered around in the > different offloads. So we could test dev->features directly for vlans > but I would rather just have netif_skb_features() return the right > values to start off with. > > > > > Jesse, I dont understand why you say "bnx2 allows vlan insertion to be > > turned off". Really. > > You can disable it using Ethtool, which will turn off > NETIF_F_HW_VLAN_TX the same as this bug. > > I'm running a quick test on a patch to always allow NETIF_F_HW_VLAN_TX > to be returned from netif_skb_features(). 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
diff --git a/net/core/dev.c b/net/core/dev.c index 54277df..8209d93 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2076,7 +2076,7 @@ int dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, features = netif_skb_features(skb); if (vlan_tx_tag_present(skb) && - !(features & NETIF_F_HW_VLAN_TX)) { + !(dev->features & NETIF_F_HW_VLAN_TX)) { skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb)); if (unlikely(!skb)) goto out;