Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.
On Mon, May 22, 2017 at 4:59 PM, David Miller wrote: > From: Vladislav Yasevich > 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 >> Signed-off-by: Vladislav Yasevich > > 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
Re: [PATCH net 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.
On 05/22/2017 07:59 PM, David Miller wrote: > From: Vladislav Yasevich > 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 >> Signed-off-by: Vladislav Yasevich > > 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.
From: Vladislav Yasevich 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 > Signed-off-by: Vladislav Yasevich 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.
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 Signed-off-by: Vladislav Yasevich --- 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 1/3] vlan: Fix tcp checksums offloads for Q-in-Q vlan.
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 Signed-off-by: Vladislav Yasevich --- 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.
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 >>> Signed-off-by: Vladislav Yasevich >>> --- >>> 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.
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 >> Signed-off-by: Vladislav Yasevich >> --- >> 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.
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 > Signed-off-by: Vladislav Yasevich > --- > 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.
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 > Signed-off-by: Vladislav Yasevich 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