[PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Alexander Duyck]

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

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:

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Eric Dumazet]

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.

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Eric Dumazet]

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>

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Cong Wang]

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.

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Alexander Duyck]

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

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Eric Dumazet]

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

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Pravin Shelar]

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>

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[David Miller]

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.

RE: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Dmitry Kravkov]

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

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Eric Dumazet]

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.

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[Alexander Duyck]

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

Re: [PATCH net v2] gso: Update tunnel segmentation to support Tx checksum offload

[David Miller]

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.