[BUG] bnx2 + vlan + TSO : doesnt work

[BUG] bnx2 + vlan + TSO : doesnt work

[Eric Dumazet]

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)

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Ben Hutchings]

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.

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

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

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Eric Dumazet]

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

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;

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Ben Hutchings]

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

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

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Jesse Gross]

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.

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[David Miller]

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.

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Eric Dumazet]

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.

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Jesse Gross]

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

Re: [BUG] bnx2 + vlan + TSO : doesnt work

[Eric Dumazet]

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