| Message ID | 20130711000015.13134.17801.stgit@ahduyck-hc1.jf.intel.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2013-07-10 at 17:05 -0700, Alexander Duyck wrote: > This change makes it so that the GRE and VXLAN tunnels can make use of Tx > checksum offload support provided by some drivers via the hw_enc_features. > Without this fix enabling GSO means sacrificing Tx checksum offload and > this actually leads to a performance regression as shown below: > > Utilization > Send > Throughput local GSO > 10^6bits/s % S state > 6276.51 8.39 enabled > 7123.52 8.42 disabled While testing your patch, I discovered TSO support is completely broken on GRE, using bnx2x testbed. Oh well. It seems Nicira guys do not test a lot their patches. -- 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 Wed, 2013-07-10 at 17:42 -0700, Eric Dumazet wrote: > On Wed, 2013-07-10 at 17:05 -0700, Alexander Duyck wrote: > > This change makes it so that the GRE and VXLAN tunnels can make use of Tx > > checksum offload support provided by some drivers via the hw_enc_features. > > Without this fix enabling GSO means sacrificing Tx checksum offload and > > this actually leads to a performance regression as shown below: > > > > Utilization > > Send > > Throughput local GSO > > 10^6bits/s % S state > > 6276.51 8.39 enabled > > 7123.52 8.42 disabled > > While testing your patch, I discovered TSO support is completely broken > on GRE, using bnx2x testbed. > > Oh well. > > It seems Nicira guys do not test a lot their patches. > Receiver receives corrupted frames : IpExtInCsumErrors is increasing The outer checksum is not correct. Maybe Dmitry has an idea of what is going on ? 18:36:06.085164 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], proto: GRE (47), length: 1500, bad cksum 3fad (->40ad)!) 10.246.17.83 > 10.246.17.84: GREv0, Flags [none], length 1480 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], proto: TCP (6), length: 1476) 7.7.8.83.52523 > 7.7.8.84.48165: ., cksum 0x88b9 (correct), 1:1425(1424) ack 1 win 449 <nop,nop,timestamp 1901210 83620016> 18:36:06.085165 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], proto: GRE (47), length: 1500, bad cksum 3fad (->40ad)!) 10.246.17.83 > 10.246.17.84: GREv0, Flags [none], length 1480 IP (tos 0x0, ttl 64, id 48052, offset 0, flags [DF], proto: TCP (6), length: 1476) 7.7.8.83.52523 > 7.7.8.84.48165: ., cksum 0x8329 (correct), 1425:2849(1424) ack 1 win 449 <nop,nop,timestamp 1901210 83620016> 18:36:06.085166 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], proto: GRE (47), length: 1500, bad cksum 3fad (->40ad)!) 10.246.17.83 > 10.246.17.84: GREv0, Flags [none], length 1480 IP (tos 0x0, ttl 64, id 48053, offset 0, flags [DF], proto: TCP (6), length: 1476) 7.7.8.83.52523 > 7.7.8.84.48165: ., cksum 0x7d99 (correct), 2849:4273(1424) ack 1 win 449 <nop,nop,timestamp 1901210 83620016> -- 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 Thu, 11 Jul 2013 at 00:42 GMT, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Wed, 2013-07-10 at 17:05 -0700, Alexander Duyck wrote: >> This change makes it so that the GRE and VXLAN tunnels can make use of Tx >> checksum offload support provided by some drivers via the hw_enc_features. >> Without this fix enabling GSO means sacrificing Tx checksum offload and >> this actually leads to a performance regression as shown below: >> >> Utilization >> Send >> Throughput local GSO >> 10^6bits/s % S state >> 6276.51 8.39 enabled >> 7123.52 8.42 disabled > > While testing your patch, I discovered TSO support is completely broken > on GRE, using bnx2x testbed. > In my test, GRE works fine, but gretap not. -- 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 07/10/2013 06:43 PM, Eric Dumazet wrote: > On Wed, 2013-07-10 at 17:42 -0700, Eric Dumazet wrote: >> On Wed, 2013-07-10 at 17:05 -0700, Alexander Duyck wrote: >>> This change makes it so that the GRE and VXLAN tunnels can make use of Tx >>> checksum offload support provided by some drivers via the hw_enc_features. >>> Without this fix enabling GSO means sacrificing Tx checksum offload and >>> this actually leads to a performance regression as shown below: >>> >>> Utilization >>> Send >>> Throughput local GSO >>> 10^6bits/s % S state >>> 6276.51 8.39 enabled >>> 7123.52 8.42 disabled >> While testing your patch, I discovered TSO support is completely broken >> on GRE, using bnx2x testbed. >> >> Oh well. >> >> It seems Nicira guys do not test a lot their patches. >> I think part of the problem is that there aren't really many devices that support any hardware offloads at this point. Thus, in the case of GSO, it wasn't really easy to notice the issue if you don't have a device capable of doing the offloads. I may look at seeing if we can push a patch I had for enabling Tx encapsulated checksum offload for ixgbe and maybe igb as at least that way we have a common platform to do some testing with. > Receiver receives corrupted frames : IpExtInCsumErrors is increasing > > The outer checksum is not correct. > > Maybe Dmitry has an idea of what is going on ? > > 18:36:06.085164 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], > proto: GRE (47), length: 1500, bad cksum 3fad (->40ad)!) 10.246.17.83 > > 10.246.17.84: GREv0, Flags [none], length 1480 > IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], proto: TCP (6), > length: 1476) 7.7.8.83.52523 > 7.7.8.84.48165: ., cksum 0x88b9 > (correct), 1:1425(1424) ack 1 win 449 <nop,nop,timestamp 1901210 > 83620016> > > 18:36:06.085165 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], > proto: GRE (47), length: 1500, bad cksum 3fad (->40ad)!) 10.246.17.83 > > 10.246.17.84: GREv0, Flags [none], length 1480 > IP (tos 0x0, ttl 64, id 48052, offset 0, flags [DF], proto: TCP (6), > length: 1476) 7.7.8.83.52523 > 7.7.8.84.48165: ., cksum 0x8329 > (correct), 1425:2849(1424) ack 1 win 449 <nop,nop,timestamp 1901210 > 83620016> > > 18:36:06.085166 IP (tos 0x0, ttl 64, id 48051, offset 0, flags [DF], > proto: GRE (47), length: 1500, bad cksum 3fad (->40ad)!) 10.246.17.83 > > 10.246.17.84: GREv0, Flags [none], length 1480 > IP (tos 0x0, ttl 64, id 48053, offset 0, flags [DF], proto: TCP (6), > length: 1476) 7.7.8.83.52523 > 7.7.8.84.48165: ., cksum 0x7d99 > (correct), 2849:4273(1424) ack 1 win 449 <nop,nop,timestamp 1901210 > 83620016> It was my understanding that the code for enabling TSO for encapsulated frames was good as Joseph has been working on it and testing it for i40e. I'm assuming if you turn off TSO then the GSO and Tx checksum are working correctly? I just want to make sure the issue you are seeing isn't in any way related to my patch. Thanks, Alex -- 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 Wed, 2013-07-10 at 19:30 -0700, Alexander Duyck wrote: > > It was my understanding that the code for enabling TSO for encapsulated > frames was good as Joseph has been working on it and testing it for > i40e. I'm assuming if you turn off TSO then the GSO and Tx checksum are > working correctly? I just want to make sure the issue you are seeing > isn't in any way related to my patch. Disabling TSO is solving the issue, and your patch solves the csum performance issue. I would like to fully understand the problems before acknowledging your patch, if you do not mind ;) -- 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 Wed, Jul 10, 2013 at 5:42 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Wed, 2013-07-10 at 17:05 -0700, Alexander Duyck wrote: >> This change makes it so that the GRE and VXLAN tunnels can make use of Tx >> checksum offload support provided by some drivers via the hw_enc_features. >> Without this fix enabling GSO means sacrificing Tx checksum offload and >> this actually leads to a performance regression as shown below: >> >> Utilization >> Send >> Throughput local GSO >> 10^6bits/s % S state >> 6276.51 8.39 enabled >> 7123.52 8.42 disabled > > While testing your patch, I discovered TSO support is completely broken > on GRE, using bnx2x testbed. > > Oh well. > > It seems Nicira guys do not test a lot their patches. > > Nice speculation ;) I did some digging and found following commit causing problem with GRE TSO on bnx setup. c957d09ffda417f6c8e3d1f10e2b05228607d6d7 is the first bad commit commit c957d09ffda417f6c8e3d1f10e2b05228607d6d7 Author: Yuval Mintz <yuvalmin@broadcom.com> Date: Tue Jun 25 08:50:11 2013 +0300 bnx2x: Remove sparse and coccinelle warnings This patch solves several sparse issues as well as an unneeded semicolon found via coccinelle. Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com> Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com> Signed-off-by: Ariel Elior <ariele@broadcom.com> Signed-off-by: Eilon Greenstein <eilong@broadcom.com> Signed-off-by: David S. Miller <davem@davemloft.net> -- 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: Pravin Shelar <pshelar@nicira.com> Date: Thu, 11 Jul 2013 00:45:19 -0700 > I did some digging and found following commit causing problem with GRE > TSO on bnx setup. > > c957d09ffda417f6c8e3d1f10e2b05228607d6d7 is the first bad commit > commit c957d09ffda417f6c8e3d1f10e2b05228607d6d7 > Author: Yuval Mintz <yuvalmin@broadcom.com> > Date: Tue Jun 25 08:50:11 2013 +0300 > > bnx2x: Remove sparse and coccinelle warnings Yeah it's this change: @@ -3543,9 +3543,12 @@ static void bnx2x_update_pbds_gso_enc(struct sk_buff *skb, /* outer IP header info */ if (xmit_type & XMIT_CSUM_V4) { struct iphdr *iph = ip_hdr(skb); + u16 csum = (__force u16)(~iph->check) - + (__force u16)iph->tot_len - + (__force u16)iph->frag_off; + pbd2->fw_ip_csum_wo_len_flags_frag = - bswab16(csum_fold((~iph->check) - - iph->tot_len - iph->frag_off)); + bswab16(csum_fold((__force __wsum)csum)); Way too aggressive with the u16 casting. These two expressions are not equivalent: csum_fold((~iph->check) - iph->tot_len - iph->frag_off) csum_fold((__wsum) ((u16)(~iph->check) - (u16)iph->tot_len - (u16)iph->frag_off)) In the first case, the entire expression is computed as 32-bit, in the second the ~iph->check is truncated to 16-bits before tot_len and frag_off are subtracted from it. -- 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
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] On Behalf Of David Miller > Sent: Thursday, July 11, 2013 11:14 AM > To: pshelar@nicira.com > Cc: eric.dumazet@gmail.com; alexander.h.duyck@intel.com; > netdev@vger.kernel.org; stephen@networkplumber.org; > joseph.gasparakis@intel.com; jesse@nicira.com > Subject: Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx > checksum offload > > From: Pravin Shelar <pshelar@nicira.com> > Date: Thu, 11 Jul 2013 00:45:19 -0700 > > > I did some digging and found following commit causing problem with GRE > > TSO on bnx setup. > > > > c957d09ffda417f6c8e3d1f10e2b05228607d6d7 is the first bad commit > > commit c957d09ffda417f6c8e3d1f10e2b05228607d6d7 > > Author: Yuval Mintz <yuvalmin@broadcom.com> > > Date: Tue Jun 25 08:50:11 2013 +0300 > > > > bnx2x: Remove sparse and coccinelle warnings > > Yeah it's this change: > > @@ -3543,9 +3543,12 @@ static void bnx2x_update_pbds_gso_enc(struct > sk_buff *skb, > /* outer IP header info */ > if (xmit_type & XMIT_CSUM_V4) { > struct iphdr *iph = ip_hdr(skb); > + u16 csum = (__force u16)(~iph->check) - > + (__force u16)iph->tot_len - > + (__force u16)iph->frag_off; > + > pbd2->fw_ip_csum_wo_len_flags_frag = > - bswab16(csum_fold((~iph->check) - > - iph->tot_len - iph->frag_off)); > + bswab16(csum_fold((__force __wsum)csum)); > > Way too aggressive with the u16 casting. These two expressions are > not equivalent: > > csum_fold((~iph->check) - iph->tot_len - iph->frag_off) > > csum_fold((__wsum) ((u16)(~iph->check) - (u16)iph->tot_len - > (u16)iph->frag_off)) > > In the first case, the entire expression is computed as 32-bit, in > the second the ~iph->check is truncated to 16-bits before tot_len > and frag_off are subtracted from it. I will provide the fix soon ... > -- > 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 -- 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 Thu, 2013-07-11 at 00:45 -0700, Pravin Shelar wrote: > Nice speculation ;) Indeed ;) Sorry Pravin ! > > I did some digging and found following commit causing problem with GRE > TSO on bnx setup. Thanks for taking care of this problem. -- 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 07/11/2013 06:51 AM, Eric Dumazet wrote: > On Thu, 2013-07-11 at 00:45 -0700, Pravin Shelar wrote: > >> Nice speculation ;) > Indeed ;) Sorry Pravin ! > >> I did some digging and found following commit causing problem with GRE >> TSO on bnx setup. > Thanks for taking care of this problem. > > I may have found another issue, but I don't think it is related to my patch. Specifically it seems like there is some MTU calculation somewhere that is is off by 14 bytes when using gretap. I'm just setting up an endpoint and using it back to back with the following commands: ip link add tun0 type gretap remote 192.168.11.129 local 192.168.11.1 ifconfig tun0 192.168.100.1/24 When I try to ping with a size of 1421 or more, it is being dropped and being counted as a tx error for the tunnel. It looks like 3.9 doesn't have this issue. I'll try to figure out the root cause before the end of today. Thanks, Alex -- 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: Alexander Duyck <alexander.h.duyck@intel.com> Date: Wed, 10 Jul 2013 17:05:06 -0700 > This change makes it so that the GRE and VXLAN tunnels can make use of Tx > checksum offload support provided by some drivers via the hw_enc_features. > Without this fix enabling GSO means sacrificing Tx checksum offload and > this actually leads to a performance regression as shown below: > > Utilization > Send > Throughput local GSO > 10^6bits/s % S state > 6276.51 8.39 enabled > 7123.52 8.42 disabled > > To resolve this it was necessary to address two items. First > netif_skb_features needed to be updated so that it would correctly handle > the Trans Ether Bridging protocol without impacting the need to check for > Q-in-Q tagging. To do this it was necessary to update harmonize_features > so that it used skb_network_protocol instead of just using the outer > protocol. > > Second it was necessary to update the GRE and UDP tunnel segmentation > offloads so that they would reset the encapsulation bit and inner header > offsets after the offload was complete. > > As a result of this change I have seen the following results on a interface > with Tx checksum enabled for encapsulated frames: > > Utilization > Send > Throughput local GSO > 10^6bits/s % S state > 7123.52 8.42 disabled > 8321.75 5.43 enabled > > v2: Instead of replacing refrence to skb->protocol with > skb_network_protocol just replace the protocol reference in > harmonize_features to allow for double VLAN tag checks. > > Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> Applied, thanks Alexander. -- 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 560dafd..d398b60 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -2481,10 +2481,10 @@ static int dev_gso_segment(struct sk_buff *skb, netdev_features_t features) } static netdev_features_t harmonize_features(struct sk_buff *skb, - __be16 protocol, netdev_features_t features) + netdev_features_t features) { if (skb->ip_summed != CHECKSUM_NONE && - !can_checksum_protocol(features, protocol)) { + !can_checksum_protocol(features, skb_network_protocol(skb))) { features &= ~NETIF_F_ALL_CSUM; } else if (illegal_highdma(skb->dev, skb)) { features &= ~NETIF_F_SG; @@ -2505,20 +2505,18 @@ netdev_features_t netif_skb_features(struct sk_buff *skb) struct vlan_ethhdr *veh = (struct vlan_ethhdr *)skb->data; protocol = veh->h_vlan_encapsulated_proto; } else if (!vlan_tx_tag_present(skb)) { - return harmonize_features(skb, protocol, features); + return harmonize_features(skb, features); } features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_STAG_TX); - if (protocol != htons(ETH_P_8021Q) && protocol != htons(ETH_P_8021AD)) { - return harmonize_features(skb, protocol, features); - } else { + if (protocol == htons(ETH_P_8021Q) || protocol == htons(ETH_P_8021AD)) 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; - return harmonize_features(skb, protocol, features); - } + + return harmonize_features(skb, features); } EXPORT_SYMBOL(netif_skb_features); diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c index 775d5b5..55e6bfb 100644 --- a/net/ipv4/gre_offload.c +++ b/net/ipv4/gre_offload.c @@ -100,6 +100,9 @@ static struct sk_buff *gre_gso_segment(struct sk_buff *skb, } __skb_push(skb, tnl_hlen - ghl); + skb_reset_inner_headers(skb); + skb->encapsulation = 1; + skb_reset_mac_header(skb); skb_set_network_header(skb, mac_len); skb->mac_len = mac_len; diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 6b270e5..d399dd5 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -2323,6 +2323,9 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb, struct udphdr *uh; int udp_offset = outer_hlen - tnl_hlen; + skb_reset_inner_headers(skb); + skb->encapsulation = 1; + skb->mac_len = mac_len; skb_push(skb, outer_hlen); @@ -2345,7 +2348,6 @@ struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb, uh->check = CSUM_MANGLED_0; } - skb->ip_summed = CHECKSUM_NONE; skb->protocol = protocol; } while ((skb = skb->next)); out:
This change makes it so that the GRE and VXLAN tunnels can make use of Tx checksum offload support provided by some drivers via the hw_enc_features. Without this fix enabling GSO means sacrificing Tx checksum offload and this actually leads to a performance regression as shown below: Utilization Send Throughput local GSO 10^6bits/s % S state 6276.51 8.39 enabled 7123.52 8.42 disabled To resolve this it was necessary to address two items. First netif_skb_features needed to be updated so that it would correctly handle the Trans Ether Bridging protocol without impacting the need to check for Q-in-Q tagging. To do this it was necessary to update harmonize_features so that it used skb_network_protocol instead of just using the outer protocol. Second it was necessary to update the GRE and UDP tunnel segmentation offloads so that they would reset the encapsulation bit and inner header offsets after the offload was complete. As a result of this change I have seen the following results on a interface with Tx checksum enabled for encapsulated frames: Utilization Send Throughput local GSO 10^6bits/s % S state 7123.52 8.42 disabled 8321.75 5.43 enabled v2: Instead of replacing refrence to skb->protocol with skb_network_protocol just replace the protocol reference in harmonize_features to allow for double VLAN tag checks. Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> --- net/core/dev.c | 14 ++++++-------- net/ipv4/gre_offload.c | 3 +++ net/ipv4/udp.c | 4 +++- 3 files changed, 12 insertions(+), 9 deletions(-) -- 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