[BUG] bnx2 + vlan + TSO : doesnt work

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)

diff --git a/net/core/dev.c b/net/core/dev.c
index 54277df..5c13e55 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -5265,6 +5265,9 @@ int register_netdevice(struct net_device *dev)
 	 */
 	dev->vlan_features |= (NETIF_F_GRO | NETIF_F_HIGHDMA);
 
+	/* allow netif_skb_features() not mask this bit from dev->features */
+	dev->vlan_features |= NETIF_F_HW_VLAN_TX;
+
 	ret = call_netdevice_notifiers(NETDEV_POST_INIT, dev);
 	ret = notifier_to_errno(ret);
 	if (ret)

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.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Le lundi 17 janvier 2011 à 23:47 +0000, Ben Hutchings a écrit :

On Tue, 2011-01-18 at 00:41 +0100, Eric Dumazet wrote:

quoted

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 ;)

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 Tue, 2011-01-18 at 01:00 +0100, Eric Dumazet wrote:

Le lundi 17 janvier 2011 à 23:47 +0000, Ben Hutchings a écrit :

quoted

On Tue, 2011-01-18 at 00:41 +0100, Eric Dumazet wrote:

quoted

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.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Mon, Jan 17, 2011 at 4:09 PM, Ben Hutchings
[off-list ref] wrote:

On Tue, 2011-01-18 at 01:00 +0100, Eric Dumazet wrote:

quoted

Le lundi 17 janvier 2011 à 23:47 +0000, Ben Hutchings a écrit :

quoted

On Tue, 2011-01-18 at 00:41 +0100, Eric Dumazet wrote:

quoted

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.

From: Jesse Gross <redacted>
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.

Le lundi 17 janvier 2011 à 22:09 -0800, David Miller a écrit :

From: Jesse Gross <redacted>
Date: Mon, 17 Jan 2011 16:13:18 -0800

quoted

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.

On Tue, Jan 18, 2011 at 1:21 AM, Eric Dumazet [off-list ref] wrote:

Le lundi 17 janvier 2011 à 22:09 -0800, David Miller a écrit :

quoted

From: Jesse Gross <redacted>
Date: Mon, 17 Jan 2011 16:13:18 -0800

quoted

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().

Le mardi 18 janvier 2011 à 01:32 -0500, Jesse Gross a écrit :

On Tue, Jan 18, 2011 at 1:21 AM, Eric Dumazet [off-list ref] wrote:

quoted

Le lundi 17 janvier 2011 à 22:09 -0800, David Miller a écrit :

quoted

From: Jesse Gross <redacted>
Date: Mon, 17 Jan 2011 16:13:18 -0800

quoted

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 ;)

quoted

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.

quoted

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