[PATCH net 0/3] vlan: Offload fixes for Q-in-Q vlans

[PATCH net 0/3] vlan: Offload fixes for Q-in-Q vlans

[Vladislav Yasevich]

Since the following commit:

commit 8cb65d00086bfba22bac87ff18b751432fc74003
Author: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Date:   Fri Mar 27 14:31:12 2015 +0900

    net: Move check for multiple vlans to drivers

offloads were broken in Q-in-Q vlans. The biggest problem was
that tcp checksum offload was enabled and used for all hardware,
while most of the supported HW (mostly old HW and some newer ones)
don't support it.  As a result, tcp connections could not be
established when using Q-in-Q vlans.

This short series corrects this by disabling checksum offload
support on packets sent through Q-in-Q vlans.  It is up to individual
drivers to enable it properly through ndo_features_check if they
have some support for Q-in-Q vlans.


Vladislav Yasevich (3):
  vlan: Fix Q-in-Q vlan support.
  be2net: Fix offload features for Q-in-Q packets
  virtio-net: enable TSO/checksum offloads for Q-in-Q vlans

 drivers/net/ethernet/emulex/benet/be_main.c | 4 +++-
 drivers/net/virtio_net.c                    | 1 +
 include/linux/if_vlan.h                     | 1 -
 3 files changed, 4 insertions(+), 2 deletions(-)

CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
-- 
2.7.4

[PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vladislav Yasevich]

It appears that since commit 8cb65d000, Q-in-Q vlans have been
broken.  The series that commit is part of enabled TSO and checksum
offloading on Q-in-Q vlans.  However, most HW we support can't handle
it.  To work around the issue, the above commit added a function that
turns off offloads on Q-in-Q devices, but it left the checksum offload.
That will cause issues with most older devices that supprort very basic
checksum offload capabilities as well as some newer devices (we've
reproduced te problem with both be2net and bnx).

To solve this for everyone, turn off checksum offloading feature
by default when sending Q-in-Q traffic.  Devices that are proven to
work can provided a corrected ndo_features_check implemetation.

Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 include/linux/if_vlan.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 8d5fcd6..ae537f0 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const struct sk_buff *skb,
 						     NETIF_F_SG |
 						     NETIF_F_HIGHDMA |
 						     NETIF_F_FRAGLIST |
-						     NETIF_F_HW_CSUM |
 						     NETIF_F_HW_VLAN_CTAG_TX |
 						     NETIF_F_HW_VLAN_STAG_TX);
 
-- 
2.7.4

[PATCH net 2/3] be2net: Fix offload features for Q-in-Q packets

[Vladislav Yasevich]

At least some of the be2net cards do not seem to be capabled
of performing checksum offload computions on Q-in-Q packets.
In these case, the recevied checksum on the remote is invalid
and TCP syn packets are dropped.

This patch adds a call to check disbled acceleration features
on Q-in-Q tagged traffic.

CC: Sathya Perla <sathya.perla@broadcom.com>
CC: Ajit Khaparde <ajit.khaparde@broadcom.com
CC: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
CC: Somnath Kotur <somnath.kotur@broadcom.com>
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/ethernet/emulex/benet/be_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
index f3a09ab..4eee18c 100644
--- a/drivers/net/ethernet/emulex/benet/be_main.c
+++ b/drivers/net/ethernet/emulex/benet/be_main.c
@@ -5078,9 +5078,11 @@ static netdev_features_t be_features_check(struct sk_buff *skb,
 	struct be_adapter *adapter = netdev_priv(dev);
 	u8 l4_hdr = 0;
 
-	/* The code below restricts offload features for some tunneled packets.
+	/* The code below restricts offload features for some tunneled and
+	 * Q-in-Q packets.
 	 * Offload features for normal (non tunnel) packets are unchanged.
 	 */
+	features = vlan_features_check(skb, features);
 	if (!skb->encapsulation ||
 	    !(adapter->flags & BE_FLAGS_VXLAN_OFFLOADS))
 		return features;
-- 
2.7.4

[PATCH net 3/3] virtio-net: enable TSO/checksum offloads for Q-in-Q vlans

[Vladislav Yasevich]

Since virtio does not provide it's own ndo_features_check handler,
TSO, and now checksum offload, are disabled for stacked vlans.
Re-enable the support and let the host take care of it.  This
restores/improves Guest-to-Guest performance over Q-in-Q vlans.

CC: "Michael S. Tsirkin" <mst@redhat.com>
CC: Jason Wang <jasowang@redhat.com>
CC: virtualization@lists.linux-foundation.org
Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 drivers/net/virtio_net.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8324a5e..341fb96 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2028,6 +2028,7 @@ static const struct net_device_ops virtnet_netdev = {
 	.ndo_poll_controller = virtnet_netpoll,
 #endif
 	.ndo_xdp		= virtnet_xdp,
+	.ndo_features_check	= passthru_features_check,
 };
 
 static void virtnet_config_changed_work(struct work_struct *work)
-- 
2.7.4

Re: [PATCH net 3/3] virtio-net: enable TSO/checksum offloads for Q-in-Q vlans

[Michael S. Tsirkin]

On Thu, May 18, 2017 at 09:31:05AM -0400, Vladislav Yasevich wrote:
> Since virtio does not provide it's own ndo_features_check handler,
> TSO, and now checksum offload, are disabled for stacked vlans.
> Re-enable the support and let the host take care of it.  This
> restores/improves Guest-to-Guest performance over Q-in-Q vlans.
> 
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: virtualization@lists.linux-foundation.org
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  drivers/net/virtio_net.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8324a5e..341fb96 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2028,6 +2028,7 @@ static const struct net_device_ops virtnet_netdev = {
>  	.ndo_poll_controller = virtnet_netpoll,
>  #endif
>  	.ndo_xdp		= virtnet_xdp,
> +	.ndo_features_check	= passthru_features_check,
>  };
>  
>  static void virtnet_config_changed_work(struct work_struct *work)
> -- 
> 2.7.4

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 2017/05/18 22:31, Vladislav Yasevich wrote:
> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

The patch looks ok, but why do you think 8cb65d000 is wrong?
The same check was there before my patch set.

kernel v4.0:
> netdev_features_t netif_skb_features(struct sk_buff *skb)
...
> 	if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD))
> 		features = netdev_intersect_features(features,
> 						     NETIF_F_SG |
> 						     NETIF_F_HIGHDMA |
> 						     NETIF_F_FRAGLIST |
> 						     NETIF_F_GEN_CSUM |
> 						     NETIF_F_HW_VLAN_CTAG_TX |
> 						     NETIF_F_HW_VLAN_STAG_TX);

The commit just moved the check into another function.


Toshiaki Makita

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 2017/05/18 22:31, Vladislav Yasevich wrote:
> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>  include/linux/if_vlan.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 8d5fcd6..ae537f0 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const struct sk_buff *skb,
>  						     NETIF_F_SG |
>  						     NETIF_F_HIGHDMA |
>  						     NETIF_F_FRAGLIST |
> -						     NETIF_F_HW_CSUM |
>  						     NETIF_F_HW_VLAN_CTAG_TX |
>  						     NETIF_F_HW_VLAN_STAG_TX);
>  

I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
is IP_CSUM and IPV6_CSUM.
So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
i.e. change intersection into bitwise AND?

The intersection was introduced in db115037bb57 ("net: fix checksum
features handling in netif_skb_features()"), but I guess for this
particular check the intersection was not needed.

-- 
Toshiaki Makita

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vlad Yasevich]

On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
> On 2017/05/18 22:31, Vladislav Yasevich wrote:
>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>> broken.  The series that commit is part of enabled TSO and checksum
>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>> it.  To work around the issue, the above commit added a function that
>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>> That will cause issues with most older devices that supprort very basic
>> checksum offload capabilities as well as some newer devices (we've
>> reproduced te problem with both be2net and bnx).
>>
>> To solve this for everyone, turn off checksum offloading feature
>> by default when sending Q-in-Q traffic.  Devices that are proven to
>> work can provided a corrected ndo_features_check implemetation.
>>
>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> ---
>>  include/linux/if_vlan.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>> index 8d5fcd6..ae537f0 100644
>> --- a/include/linux/if_vlan.h
>> +++ b/include/linux/if_vlan.h
>> @@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const struct sk_buff *skb,
>>  						     NETIF_F_SG |
>>  						     NETIF_F_HIGHDMA |
>>  						     NETIF_F_FRAGLIST |
>> -						     NETIF_F_HW_CSUM |
>>  						     NETIF_F_HW_VLAN_CTAG_TX |
>>  						     NETIF_F_HW_VLAN_STAG_TX);
>>  
> 
> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
> is IP_CSUM and IPV6_CSUM.
> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
> i.e. change intersection into bitwise AND?
> 

It wasn't really a problem before accelerations got enabled on q-in-q
vlans.

> The intersection was introduced in db115037bb57 ("net: fix checksum
> features handling in netif_skb_features()"), but I guess for this
> particular check the intersection was not needed.
> 

So, to put it another way, leave the intersection with HW_CSUM in the mask,
and then do:

  return features & ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);

This might work, but it assumes that everyone who announce HW_CSUM can
do q-in-q vlans.  It's been a bit of a pain tracking this down and I'd rather
fix it for everyone and let individual driver authors verify that Q-in-Q works
correctly with HW checksum.  However, I am willing to do the above if
that's what people want.

-vlad

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 2017/05/19 16:09, Vlad Yasevich wrote:
> On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
>> On 2017/05/18 22:31, Vladislav Yasevich wrote:
>>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>>> broken.  The series that commit is part of enabled TSO and checksum
>>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>>> it.  To work around the issue, the above commit added a function that
>>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>>> That will cause issues with most older devices that supprort very basic
>>> checksum offload capabilities as well as some newer devices (we've
>>> reproduced te problem with both be2net and bnx).
>>>
>>> To solve this for everyone, turn off checksum offloading feature
>>> by default when sending Q-in-Q traffic.  Devices that are proven to
>>> work can provided a corrected ndo_features_check implemetation.
>>>
>>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>> ---
>>>  include/linux/if_vlan.h | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>>> index 8d5fcd6..ae537f0 100644
>>> --- a/include/linux/if_vlan.h
>>> +++ b/include/linux/if_vlan.h
>>> @@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const struct sk_buff *skb,
>>>  						     NETIF_F_SG |
>>>  						     NETIF_F_HIGHDMA |
>>>  						     NETIF_F_FRAGLIST |
>>> -						     NETIF_F_HW_CSUM |
>>>  						     NETIF_F_HW_VLAN_CTAG_TX |
>>>  						     NETIF_F_HW_VLAN_STAG_TX);
>>>  
>>
>> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
>> is IP_CSUM and IPV6_CSUM.
>> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
>> i.e. change intersection into bitwise AND?
>>
> 
> It wasn't really a problem before accelerations got enabled on q-in-q
> vlans.

Right for stacked vlan device.
But I think the check was there for packets from guests forwarded by
bridge to vlan device so it was a problem before 8cb65d000.

>> The intersection was introduced in db115037bb57 ("net: fix checksum
>> features handling in netif_skb_features()"), but I guess for this
>> particular check the intersection was not needed.
>>
> 
> So, to put it another way, leave the intersection with HW_CSUM in the mask,
> and then do:
> 
>   return features & ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
> 
> This might work, but it assumes that everyone who announce HW_CSUM can
> do q-in-q vlans.  It's been a bit of a pain tracking this down and I'd rather
> fix it for everyone and let individual driver authors verify that Q-in-Q works
> correctly with HW checksum.  However, I am willing to do the above if
> that's what people want.

At least HW_CSUM in the check was introduced intentionally.
https://www.spinics.net/lists/netdev/msg152016.html

And I think HW_CSUM should work with any packets.
You know, include/linux/skbuff.h says
>  *	NETIF_F_HW_CSUM	- The driver (or its device) is able to compute one
>  *			  IP (one's complement) checksum for any combination
>  *			  of protocols or protocol layering.

We should be able to safely enable it.

...But you are so worried about layer2 protocol handling of HW_CSUM
devices, I'm ok with disabling it for now.

-- 
Toshiaki Makita

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vlad Yasevich]

On 05/19/2017 04:16 AM, Toshiaki Makita wrote:
> On 2017/05/19 16:09, Vlad Yasevich wrote:
>> On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
>>> On 2017/05/18 22:31, Vladislav Yasevich wrote:
>>>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>>>> broken.  The series that commit is part of enabled TSO and checksum
>>>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>>>> it.  To work around the issue, the above commit added a function that
>>>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>>>> That will cause issues with most older devices that supprort very basic
>>>> checksum offload capabilities as well as some newer devices (we've
>>>> reproduced te problem with both be2net and bnx).
>>>>
>>>> To solve this for everyone, turn off checksum offloading feature
>>>> by default when sending Q-in-Q traffic.  Devices that are proven to
>>>> work can provided a corrected ndo_features_check implemetation.
>>>>
>>>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>>>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>> ---
>>>>  include/linux/if_vlan.h | 1 -
>>>>  1 file changed, 1 deletion(-)
>>>>
>>>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>>>> index 8d5fcd6..ae537f0 100644
>>>> --- a/include/linux/if_vlan.h
>>>> +++ b/include/linux/if_vlan.h
>>>> @@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const struct sk_buff *skb,
>>>>  						     NETIF_F_SG |
>>>>  						     NETIF_F_HIGHDMA |
>>>>  						     NETIF_F_FRAGLIST |
>>>> -						     NETIF_F_HW_CSUM |
>>>>  						     NETIF_F_HW_VLAN_CTAG_TX |
>>>>  						     NETIF_F_HW_VLAN_STAG_TX);
>>>>  
>>>
>>> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
>>> is IP_CSUM and IPV6_CSUM.
>>> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
>>> i.e. change intersection into bitwise AND?
>>>
>>
>> It wasn't really a problem before accelerations got enabled on q-in-q
>> vlans.
> 
> Right for stacked vlan device.
> But I think the check was there for packets from guests forwarded by
> bridge to vlan device so it was a problem before 8cb65d000.

Not really, since stacked vlans in guests wouldn't have accelerations on.
Haven't really tried a new guest on old hosts.  It might be an issue there...

> 
>>> The intersection was introduced in db115037bb57 ("net: fix checksum
>>> features handling in netif_skb_features()"), but I guess for this
>>> particular check the intersection was not needed.
>>>
>>
>> So, to put it another way, leave the intersection with HW_CSUM in the mask,
>> and then do:
>>
>>   return features & ~(NETIF_F_IP_CSUM|NETIF_F_IPV6_CSUM);
>>
>> This might work, but it assumes that everyone who announce HW_CSUM can
>> do q-in-q vlans.  It's been a bit of a pain tracking this down and I'd rather
>> fix it for everyone and let individual driver authors verify that Q-in-Q works
>> correctly with HW checksum.  However, I am willing to do the above if
>> that's what people want.
> 
> At least HW_CSUM in the check was introduced intentionally.
> https://www.spinics.net/lists/netdev/msg152016.html
> 
> And I think HW_CSUM should work with any packets.
> You know, include/linux/skbuff.h says
>>  *	NETIF_F_HW_CSUM	- The driver (or its device) is able to compute one
>>  *			  IP (one's complement) checksum for any combination
>>  *			  of protocols or protocol layering.
> 
> We should be able to safely enable it.
> 
> ...But you are so worried about layer2 protocol handling of HW_CSUM
> devices, I'm ok with disabling it for now.
> 

It's a concern after running across this issue.  Granted, the few devices
we've seen this bug on use IP/IPV6 checksum features.  I am hoping someone
else might weigh in here.

-vlad

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Toshiaki Makita]

On 17/05/19 (金) 18:53, Vlad Yasevich wrote:
> On 05/19/2017 04:16 AM, Toshiaki Makita wrote:
>> On 2017/05/19 16:09, Vlad Yasevich wrote:
>>> On 05/18/2017 10:13 PM, Toshiaki Makita wrote:
>>>> On 2017/05/18 22:31, Vladislav Yasevich wrote:
>>>>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>>>>> broken.  The series that commit is part of enabled TSO and checksum
>>>>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>>>>> it.  To work around the issue, the above commit added a function that
>>>>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>>>>> That will cause issues with most older devices that supprort very basic
>>>>> checksum offload capabilities as well as some newer devices (we've
>>>>> reproduced te problem with both be2net and bnx).
>>>>>
>>>>> To solve this for everyone, turn off checksum offloading feature
>>>>> by default when sending Q-in-Q traffic.  Devices that are proven to
>>>>> work can provided a corrected ndo_features_check implemetation.
>>>>>
>>>>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>>>>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>>> ---
>>>>>  include/linux/if_vlan.h | 1 -
>>>>>  1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
>>>>> index 8d5fcd6..ae537f0 100644
>>>>> --- a/include/linux/if_vlan.h
>>>>> +++ b/include/linux/if_vlan.h
>>>>> @@ -619,7 +619,6 @@ static inline netdev_features_t vlan_features_check(const struct sk_buff *skb,
>>>>>  						     NETIF_F_SG |
>>>>>  						     NETIF_F_HIGHDMA |
>>>>>  						     NETIF_F_FRAGLIST |
>>>>> -						     NETIF_F_HW_CSUM |
>>>>>  						     NETIF_F_HW_VLAN_CTAG_TX |
>>>>>  						     NETIF_F_HW_VLAN_STAG_TX);
>>>>>
>>>>
>>>> I guess HW_CSUM theoretically can handle Q-in-Q packets and the problem
>>>> is IP_CSUM and IPV6_CSUM.
>>>> So wouldn't it be better to leave HW_CSUM and drop IP_CSUM/IPV6_CSUM,
>>>> i.e. change intersection into bitwise AND?
>>>>
>>>
>>> It wasn't really a problem before accelerations got enabled on q-in-q
>>> vlans.
>>
>> Right for stacked vlan device.
>> But I think the check was there for packets from guests forwarded by
>> bridge to vlan device so it was a problem before 8cb65d000.
>
> Not really, since stacked vlans in guests wouldn't have accelerations on.
> Haven't really tried a new guest on old hosts.  It might be an issue there...

It's real. I'm now remembering that I came across a similar issue before 
introducing 8cb65d000. The situation was that bridge (vlan_filtering) 
adds a vlan tag to a frame which is already tagged by guests, or by a 
vlan device on the top of the bridge (Note that virtio and bridge have 
HW_CSUM in vlan_features). I addressed the problem in drivers side since 
all the IP/IPV6_CSUM drivers I encountered the issue on are able to 
notify devices of IP header offset. Now I checked be2net driver's code 
and realized it doesn't provide IP offset so it makes sense to drop 
IP/IPV6_CSUM by default. Anyway, kernels before 8cb65d000 have that 
problem, not only after 8cb65d000.

Toshiaki Makita

Re: [PATCH net 3/3] virtio-net: enable TSO/checksum offloads for Q-in-Q vlans

[Jason Wang]

On 2017年05月18日 21:31, Vladislav Yasevich wrote:
> Since virtio does not provide it's own ndo_features_check handler,
> TSO, and now checksum offload, are disabled for stacked vlans.
> Re-enable the support and let the host take care of it.  This
> restores/improves Guest-to-Guest performance over Q-in-Q vlans.
>
> CC: "Michael S. Tsirkin" <mst@redhat.com>
> CC: Jason Wang <jasowang@redhat.com>
> CC: virtualization@lists.linux-foundation.org
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> ---
>   drivers/net/virtio_net.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8324a5e..341fb96 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2028,6 +2028,7 @@ static const struct net_device_ops virtnet_netdev = {
>   	.ndo_poll_controller = virtnet_netpoll,
>   #endif
>   	.ndo_xdp		= virtnet_xdp,
> +	.ndo_features_check	= passthru_features_check,
>   };
>   
>   static void virtnet_config_changed_work(struct work_struct *work)

Acked-by: Jason Wang <jasowang@redhat.com>

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[David Miller]

From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Thu, 18 May 2017 09:31:03 -0400

> It appears that since commit 8cb65d000, Q-in-Q vlans have been
> broken.  The series that commit is part of enabled TSO and checksum
> offloading on Q-in-Q vlans.  However, most HW we support can't handle
> it.  To work around the issue, the above commit added a function that
> turns off offloads on Q-in-Q devices, but it left the checksum offload.
> That will cause issues with most older devices that supprort very basic
> checksum offload capabilities as well as some newer devices (we've
> reproduced te problem with both be2net and bnx).
> 
> To solve this for everyone, turn off checksum offloading feature
> by default when sending Q-in-Q traffic.  Devices that are proven to
> work can provided a corrected ndo_features_check implemetation.
> 
> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

This is a tough one.  I can certainly sympathize with your frustration
trying to track this down.

Clearing NETIF_F_HW_CSUM completely is the most conservative change.

However, for all the (perhaps many) cards upon which the checksumming
does work properly in Q-in-Q situations, this change could be
introducing non-trivial performance regressions.

So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
on balance, the best way forward.

Thanks.

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Vlad Yasevich]

On 05/22/2017 07:59 PM, David Miller wrote:
> From: Vladislav Yasevich <vyasevich@gmail.com>
> Date: Thu, 18 May 2017 09:31:03 -0400
> 
>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>> broken.  The series that commit is part of enabled TSO and checksum
>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>> it.  To work around the issue, the above commit added a function that
>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>> That will cause issues with most older devices that supprort very basic
>> checksum offload capabilities as well as some newer devices (we've
>> reproduced te problem with both be2net and bnx).
>>
>> To solve this for everyone, turn off checksum offloading feature
>> by default when sending Q-in-Q traffic.  Devices that are proven to
>> work can provided a corrected ndo_features_check implemetation.
>>
>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> 
> This is a tough one.  I can certainly sympathize with your frustration
> trying to track this down.
> 
> Clearing NETIF_F_HW_CSUM completely is the most conservative change.
> 
> However, for all the (perhaps many) cards upon which the checksumming
> does work properly in Q-in-Q situations, this change could be
> introducing non-trivial performance regressions.
> 
> So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
> on balance, the best way forward.
> 

Thanks.  I'll update and re-submit.

-vlad

> Thanks.
> 

Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.

[Alexander Duyck]

On Mon, May 22, 2017 at 4:59 PM, David Miller <davem@davemloft.net> wrote:
> From: Vladislav Yasevich <vyasevich@gmail.com>
> Date: Thu, 18 May 2017 09:31:03 -0400
>
>> It appears that since commit 8cb65d000, Q-in-Q vlans have been
>> broken.  The series that commit is part of enabled TSO and checksum
>> offloading on Q-in-Q vlans.  However, most HW we support can't handle
>> it.  To work around the issue, the above commit added a function that
>> turns off offloads on Q-in-Q devices, but it left the checksum offload.
>> That will cause issues with most older devices that supprort very basic
>> checksum offload capabilities as well as some newer devices (we've
>> reproduced te problem with both be2net and bnx).
>>
>> To solve this for everyone, turn off checksum offloading feature
>> by default when sending Q-in-Q traffic.  Devices that are proven to
>> work can provided a corrected ndo_features_check implemetation.
>>
>> Fixes: 8cb65d000 ("net: Move check for multiple vlans to drivers")
>> CC: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>
> This is a tough one.  I can certainly sympathize with your frustration
> trying to track this down.
>
> Clearing NETIF_F_HW_CSUM completely is the most conservative change.
>
> However, for all the (perhaps many) cards upon which the checksumming
> does work properly in Q-in-Q situations, this change could be
> introducing non-trivial performance regressions.
>
> So I think Toshiaki's suggestion to drop IP_CSUM and IPV6_CSUM is,
> on balance, the best way forward.
>
> Thanks.

I would have to agree. I think we can simplify all this since all we
are essentially looking at doing is dropping the
netdev_features_intersect call and instead just do an AND instead of
fuzzing for the other CSUM bits. That way we don't have to worry about
systems that might only be able to handle frames that are IPv4/v6
with one level of VLAN and should be able to support multiple levels
of mixed headers.

We probably should add a comment about HW_CSUM being the only one we
can support as well so nobody comes back later and tries to revert the
change.

- Alex