Skip to contentSkip to conversation

TSO and checksum fixes for i40e

PATCH Superseded Alexander Duyck 5 participants 10 messages Jan 15, 2016 – Jan 20, 2016

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover TSO and checksum fixes for i40e

This patch set is meant to improve the performance and reliability of i40e when it comes to performing TSO and Tx checksum offloads related to tunnels.

I have tested it with a number of combinations of v4 over v6 and v6 over v4 for VXLANs. With this patch set I resolved a number of issues and I am now able to perform TSO for any of them as long as the outer UDP checksum is 0. It should also now be supported if the outer checksum is enabled in the case of the XL722, though I cannot test it.

I was able to verify IPv6 TSO is working correctly, however it seems like if I use VXLAN over IPv6 I am not seeing any receive checksum offloads. I'm still trying to work out that bit but as far as I can tell I haven't configured anything incorrectly as the receiving end is getting the frames in the proper format.

I'll be out on vacation for the next several days so if anything comes up I will address it on Tuesday.

Thanks.

-Alex

---

Alexander Duyck (3):
i40e: Refactor TSO to resolve unwanted behaviors
i40e: Refactor Tx checksum offload
i40e: Fix build warnings when geneve is not enabled

 drivers/net/ethernet/intel/i40e/i40e_main.c |   24 +-
 drivers/net/ethernet/intel/i40e/i40e_txrx.c |  284 ++++++++++++++++-----------
 drivers/net/ethernet/intel/i40e/i40e_txrx.h |    1 
 3 files changed, 178 insertions(+), 131 deletions(-)

--

#1 lore raw

PATCH 1/3 i40e: Refactor TSO to resolve unwanted behaviors

This patch is meant to be a general clean-up of tso. There were a number of spots where redundant actions were being taken in regards to the TSO function and the checksum function, or in some cases frames were technically being corrupted with checksums that were never requested.

So first of this patch goes through and takes all the spots that were writing the outer IP header checksum to zero and moves them all into the TSO function. (This includes the spot in IPv6 where this was occurring).

The second big change I made was to rework the outer checksum functionality so that it would be used when requested via GSO instead of just inserting it on all of the headers. This way we should be able to control if it is enabled or not via the "udpcsum" flag when creating a VXLANn tunnel. The functionality will likely not be needed for non-segmented frames as we will be able to just compute the checksum via local checksum offload.

I also dropped some useless flags from the hw_enc_features. Specifically SCTP offload and RX_CSUM don't have any impact on the actual offloads provided. Specifically a tunnel doesn't advertise SCTP support, mainly because it is a CRC offload and there is no support for doing it via a software offload. The RX_CSUM feature doesn't provide any value add due to the fact that it doesn't really do anything for the tunnel. It either receives checksum offloaded frames or not. If it chooses to ignore them is entirely up to it.

---
 drivers/net/ethernet/intel/i40e/i40e_main.c |   16 ++---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c |   93 ++++++++++++++++-----------
 drivers/net/ethernet/intel/i40e/i40e_txrx.h |    1 
 3 files changed, 62 insertions(+), 48 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +8 -8
index 2cfdd85f50fb..c21ad853d72f 100644
@@ -7473,8 +7473,6 @@ static int i40e_alloc_rings(struct i40e_vsi *vsi)
 		tx_ring->dcb_tc = 0;
 		if (vsi->back->flags & I40E_FLAG_WB_ON_ITR_CAPABLE)
 			tx_ring->flags = I40E_TXR_FLAGS_WB_ON_ITR;
-		if (vsi->back->flags & I40E_FLAG_OUTER_UDP_CSUM_CAPABLE)
-			tx_ring->flags |= I40E_TXR_FLAGS_OUTER_UDP_CSUM;
 		vsi->tx_rings[i] = tx_ring;
 
 		rx_ring = &tx_ring[1];
@@ -9018,12 +9016,12 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 	np = netdev_priv(netdev);
 	np->vsi = vsi;
 
-	netdev->hw_enc_features |= NETIF_F_IP_CSUM	  |
-				   NETIF_F_RXCSUM	  |
-				   NETIF_F_SCTP_CRC	  |
-				   NETIF_F_GSO_UDP_TUNNEL |
-				   NETIF_F_GSO_GRE	  |
-				   NETIF_F_TSO		  |
+	netdev->hw_enc_features |= NETIF_F_IP_CSUM	       |
+				   NETIF_F_TSO		       |
+				   NETIF_F_TSO_ECN	       |
+				   NETIF_F_GSO_GRE	       |
+				   NETIF_F_GSO_UDP_TUNNEL      |
+				   NETIF_F_GSO_UDP_TUNNEL_CSUM |
 				   0;
 
 	netdev->features = NETIF_F_SG		       |
@@ -9045,6 +9043,8 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 
 	if (!(pf->flags & I40E_FLAG_MFP_ENABLED))
 		netdev->features |= NETIF_F_NTUPLE;
+	if (pf->flags & I40E_FLAG_OUTER_UDP_CSUM_CAPABLE)
+		netdev->features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
 
 	/* copy netdev features into list of user selectable features */
 	netdev->hw_features |= netdev->features;
drivers/net/ethernet/intel/i40e/i40e_txrx.c +54 -39

137 lines not on this page · raw message

drivers/net/ethernet/intel/i40e/i40e_txrx.h +0 -1
index 1d167b646193..fc77a271aba8 100644
@@ -275,7 +275,6 @@ struct i40e_ring {
 
 	u16 flags;
 #define I40E_TXR_FLAGS_WB_ON_ITR	BIT(0)
-#define I40E_TXR_FLAGS_OUTER_UDP_CSUM	BIT(1)
 #define I40E_TXR_FLAGS_LAST_XMIT_MORE_SET BIT(2)
 
 	/* stats structs */
#2 lore raw

PATCH 1/3 i40e: Refactor TSO to resolve unwanted behaviors

First, thanks Alex!

Series looks pretty good, I'm applying it and doing some testing and our testers will get to it soon.

One issue I noted below.

Quoted 147 lines · On Thu, 14 Jan 2016 20:41:56 -0800 Alexander Duyck <[email protected]> wrote:

This patch is meant to be a general clean-up of tso. There were a number of spots where redundant actions were being taken in regards to the TSO function and the checksum function, or in some cases frames were technically being corrupted with checksums that were never requested.

So first of this patch goes through and takes all the spots that were writing the outer IP header checksum to zero and moves them all into the TSO function. (This includes the spot in IPv6 where this was occurring).

The second big change I made was to rework the outer checksum functionality so that it would be used when requested via GSO instead of just inserting it on all of the headers. This way we should be able to control if it is enabled or not via the "udpcsum" flag when creating a VXLANn tunnel. The functionality will likely not be needed for non-segmented frames as we will be able to just compute the checksum via local checksum offload.

I also dropped some useless flags from the hw_enc_features. Specifically SCTP offload and RX_CSUM don't have any impact on the actual offloads provided. Specifically a tunnel doesn't advertise SCTP support, mainly because it is a CRC offload and there is no support for doing it via a software offload. The RX_CSUM feature doesn't provide any value add due to the fact that it doesn't really do anything for the tunnel. It either receives checksum offloaded frames or not. If it chooses to ignore them is entirely up to it.

---
 drivers/net/ethernet/intel/i40e/i40e_main.c |   16 ++---
 drivers/net/ethernet/intel/i40e/i40e_txrx.c |   93 ++++++++++++++++-----------
 drivers/net/ethernet/intel/i40e/i40e_txrx.h |    1 
 3 files changed, 62 insertions(+), 48 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +8 -8
index 2cfdd85f50fb..c21ad853d72f 100644
@@ -7473,8 +7473,6 @@ static int i40e_alloc_rings(struct i40e_vsi *vsi)
 		tx_ring->dcb_tc = 0;
 		if (vsi->back->flags & I40E_FLAG_WB_ON_ITR_CAPABLE)
 			tx_ring->flags = I40E_TXR_FLAGS_WB_ON_ITR;
-		if (vsi->back->flags & I40E_FLAG_OUTER_UDP_CSUM_CAPABLE)
-			tx_ring->flags |= I40E_TXR_FLAGS_OUTER_UDP_CSUM;
 		vsi->tx_rings[i] = tx_ring;
 
 		rx_ring = &tx_ring[1];
@@ -9018,12 +9016,12 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 	np = netdev_priv(netdev);
 	np->vsi = vsi;
 
-	netdev->hw_enc_features |= NETIF_F_IP_CSUM	  |
-				   NETIF_F_RXCSUM	  |
-				   NETIF_F_SCTP_CRC	  |
-				   NETIF_F_GSO_UDP_TUNNEL |
-				   NETIF_F_GSO_GRE	  |
-				   NETIF_F_TSO		  |
+	netdev->hw_enc_features |= NETIF_F_IP_CSUM	       |
+				   NETIF_F_TSO		       |
+				   NETIF_F_TSO_ECN	       |
+				   NETIF_F_GSO_GRE	       |
+				   NETIF_F_GSO_UDP_TUNNEL      |
+				   NETIF_F_GSO_UDP_TUNNEL_CSUM |
 				   0;
 
 	netdev->features = NETIF_F_SG		       |
@@ -9045,6 +9043,8 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 
 	if (!(pf->flags & I40E_FLAG_MFP_ENABLED))
 		netdev->features |= NETIF_F_NTUPLE;
+	if (pf->flags & I40E_FLAG_OUTER_UDP_CSUM_CAPABLE)
+		netdev->features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
 
 	/* copy netdev features into list of user selectable features */
 	netdev->hw_features |= netdev->features;
drivers/net/ethernet/intel/i40e/i40e_txrx.c +34 -23
index ca5f0881a5bf..ac2fa7b02e94 100644
@@ -2214,11 +2214,18 @@ out:
 static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb,
 		    u8 *hdr_len, u64 *cd_type_cmd_tso_mss)
 {
-	u32 cd_cmd, cd_tso_len, cd_mss;
-	struct ipv6hdr *ipv6h;
-	struct tcphdr *tcph;
-	struct iphdr *iph;
-	u32 l4len;
+	u64 cd_cmd, cd_tso_len, cd_mss;
+	union {
+		struct iphdr *v4;
+		struct ipv6hdr *v6;
+		unsigned char *hdr;
+	} ip;
+	union {
+		struct tcphdr *tcp;
+		struct udphdr *udp;
+		unsigned char *hdr;
+	} l4;
+	u32 paylen, l4_offset;
 	int err;
 
 	if (skb->ip_summed != CHECKSUM_PARTIAL)
@@ -2231,35 +2238,51 @@ static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb,
 	if (err < 0)
 		return err;
 
-	iph = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
-	ipv6h = skb->encapsulation ? inner_ipv6_hdr(skb) : ipv6_hdr(skb);
-
-	if (iph->version == 4) {
-		tcph = skb->encapsulation ? inner_tcp_hdr(skb) : tcp_hdr(skb);
-		iph->tot_len = 0;
-		iph->check = 0;
-		tcph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
-						 0, IPPROTO_TCP, 0);
-	} else if (ipv6h->version == 6) {
-		tcph = skb->encapsulation ? inner_tcp_hdr(skb) : tcp_hdr(skb);
-		ipv6h->payload_len = 0;
-		tcph->check = ~csum_ipv6_magic(&ipv6h->saddr, &ipv6h->daddr,
-					       0, IPPROTO_TCP, 0);
+	ip.hdr = skb_network_header(skb);
+	l4.hdr = skb_transport_header(skb);
+
+	if (skb->encapsulation) {
+		if (ip.v4->version == 4) {
+			ip.v4->check = 0;
+			ip.v4->tot_len = 0;
+		} else {
+			ip.v6->payload_len = 0;
+		}
+
+		/* if we end up needing to do any changes such as update
+		 * the pseudo header checksum on the outer header we
+		 * should do it here.
+		 */
+
+		ip.hdr = skb_inner_network_header(skb);
+		l4.hdr = skb_inner_transport_header(skb);
 	}
 
-	l4len = skb->encapsulation ? inner_tcp_hdrlen(skb) : tcp_hdrlen(skb);
-	*hdr_len = (skb->encapsulation
-		    ? (skb_inner_transport_header(skb) - skb->data)
-		    : skb_transport_offset(skb)) + l4len;
+	/* initialize IP header fields */
+	if (ip.v4->version == 4) {
+		ip.v4->check = 0;
+		ip.v4->tot_len = 0;

drivers/net/ethernet/intel/i40e/i40e_txrx.c static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb, · line 2274

Quoted 10 lines
drivers/net/ethernet/intel/i40e/i40e_txrx.c +10 -0
+	} else {
+		ip.v6->payload_len = 0;
+	}
+
+	/* determine offset of transport header */
+	l4_offset = l4.hdr - skb->data;
+
+	/* remove payload length from checksum */
+	paylen = ntohs(1) * (u16)~(skb->len - l4_offset);
+	l4.tcp->check = ~csum_fold((__force __wsum)paylen + l4.tcp->check);

introduces sparse errors
CHECK /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c
/home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:37: warning: restricted __wsum degrades to integer /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:67: warning: restricted __sum16 degrades to integer /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:59: warning: incorrect type in argument 1 (different base types)
/home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:59: expected restricted __wsum [usertype] sum
/home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:59: got unsigned int

Quoted 79 lines
drivers/net/ethernet/intel/i40e/i40e_txrx.c +10 -16
+
+	/* compute length of segmentation header */
+	*hdr_len = (l4.tcp->doff * 4) + l4_offset;
 
 	/* find the field values */
 	cd_cmd = I40E_TX_CTX_DESC_TSO;
 	cd_tso_len = skb->len - *hdr_len;
 	cd_mss = skb_shinfo(skb)->gso_size;
-	*cd_type_cmd_tso_mss |= ((u64)cd_cmd << I40E_TXD_CTX_QW1_CMD_SHIFT) |
-				((u64)cd_tso_len <<
-				 I40E_TXD_CTX_QW1_TSO_LEN_SHIFT) |
-				((u64)cd_mss << I40E_TXD_CTX_QW1_MSS_SHIFT);
+	*cd_type_cmd_tso_mss |= (cd_cmd << I40E_TXD_CTX_QW1_CMD_SHIFT) |
+				(cd_tso_len << I40E_TXD_CTX_QW1_TSO_LEN_SHIFT) |
+				(cd_mss << I40E_TXD_CTX_QW1_MSS_SHIFT);
 	return 1;
 }
 
@@ -2351,15 +2374,12 @@ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 		if (*tx_flags & I40E_TX_FLAGS_IPV4) {
 			if (*tx_flags & I40E_TX_FLAGS_TSO) {
 				*cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV4;
-				ip_hdr(skb)->check = 0;
 			} else {
 				*cd_tunneling |=
 					 I40E_TX_CTX_EXT_IP_IPV4_NO_CSUM;
 			}
 		} else if (*tx_flags & I40E_TX_FLAGS_IPV6) {
 			*cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV6;
-			if (*tx_flags & I40E_TX_FLAGS_TSO)
-				ip_hdr(skb)->check = 0;
 		}
 
 		/* Now set the ctx descriptor fields */
@@ -2373,15 +2393,11 @@ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 			*tx_flags &= ~I40E_TX_FLAGS_IPV4;
 			*tx_flags |= I40E_TX_FLAGS_IPV6;
 		}
-		if ((tx_ring->flags & I40E_TXR_FLAGS_OUTER_UDP_CSUM) &&
-		    (l4_tunnel == I40E_TXD_CTX_UDP_TUNNELING)        &&
-		    (*cd_tunneling & I40E_TXD_CTX_QW0_EXT_IP_MASK)) {
-			oudph->check = ~csum_tcpudp_magic(oiph->saddr,
-					oiph->daddr,
-					(skb->len - skb_transport_offset(skb)),
-					IPPROTO_UDP, 0);
+		/* indicate if we need to offload outer UDP header */
+		if ((*tx_flags & I40E_TX_FLAGS_TSO) &&
+		    (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM))
 			*cd_tunneling |= I40E_TXD_CTX_QW0_L4T_CS_MASK;
-		}
+
 	} else {
 		network_hdr_len = skb_network_header_len(skb);
 		this_ip_hdr = ip_hdr(skb);
@@ -2397,7 +2413,6 @@ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
 		 */
 		if (*tx_flags & I40E_TX_FLAGS_TSO) {
 			*td_cmd |= I40E_TX_DESC_CMD_IIPT_IPV4_CSUM;
-			this_ip_hdr->check = 0;
 		} else {
 			*td_cmd |= I40E_TX_DESC_CMD_IIPT_IPV4;
 		}
drivers/net/ethernet/intel/i40e/i40e_txrx.h +0 -1
index 1d167b646193..fc77a271aba8 100644
@@ -275,7 +275,6 @@ struct i40e_ring {
 
 	u16 flags;
 #define I40E_TXR_FLAGS_WB_ON_ITR	BIT(0)
-#define I40E_TXR_FLAGS_OUTER_UDP_CSUM	BIT(1)
 #define I40E_TXR_FLAGS_LAST_XMIT_MORE_SET BIT(2)
 
 	/* stats structs */

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

#3 lore raw

PATCH 1/3 i40e: Refactor TSO to resolve unwanted behaviors

I'll probably submit a v2 with the cheksum adjustment bits split out into a separate function since there are multiple drivers that need to make similar adjustments to checksums. That way I only have to have one function doing all the casting between bitwise types.

-Alex

Sent from my iPad

Quoted 267 lines · Jesse Brandeburg

First, thanks Alex!

Series looks pretty good, I'm applying it and doing some testing and our testers will get to it soon.

One issue I noted below.

Quoted 147 lines · On Thu, 14 Jan 2016 20:41:56 -0800 Alexander Duyck <[email protected]> wrote:

This patch is meant to be a general clean-up of tso. There were a number of spots where redundant actions were being taken in regards to the TSO function and the checksum function, or in some cases frames were technically being corrupted with checksums that were never requested.

So first of this patch goes through and takes all the spots that were writing the outer IP header checksum to zero and moves them all into the TSO function. (This includes the spot in IPv6 where this was occurring).

The second big change I made was to rework the outer checksum functionality so that it would be used when requested via GSO instead of just inserting it on all of the headers. This way we should be able to control if it is enabled or not via the "udpcsum" flag when creating a VXLANn tunnel. The functionality will likely not be needed for non-segmented frames as we will be able to just compute the checksum via local checksum offload.

I also dropped some useless flags from the hw_enc_features. Specifically SCTP offload and RX_CSUM don't have any impact on the actual offloads provided. Specifically a tunnel doesn't advertise SCTP support, mainly because it is a CRC offload and there is no support for doing it via a software offload. The RX_CSUM feature doesn't provide any value add due to the fact that it doesn't really do anything for the tunnel. It either receives checksum offloaded frames or not. If it chooses to ignore them is entirely up to it.

---

drivers/net/ethernet/intel/i40e/i40e_main.c |   16 ++---
drivers/net/ethernet/intel/i40e/i40e_txrx.c |   93 ++++++++++++++++-----------
drivers/net/ethernet/intel/i40e/i40e_txrx.h |    1 
3 files changed, 62 insertions(+), 48 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +8 -8
index 2cfdd85f50fb..c21ad853d72f 100644
@@ -7473,8 +7473,6 @@ static int i40e_alloc_rings(struct i40e_vsi *vsi)
       tx_ring->dcb_tc = 0;
       if (vsi->back->flags & I40E_FLAG_WB_ON_ITR_CAPABLE)
           tx_ring->flags = I40E_TXR_FLAGS_WB_ON_ITR;
-        if (vsi->back->flags & I40E_FLAG_OUTER_UDP_CSUM_CAPABLE)
-            tx_ring->flags |= I40E_TXR_FLAGS_OUTER_UDP_CSUM;
       vsi->tx_rings[i] = tx_ring;
 
       rx_ring = &tx_ring[1];
@@ -9018,12 +9016,12 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
   np = netdev_priv(netdev);
   np->vsi = vsi;
 
-    netdev->hw_enc_features |= NETIF_F_IP_CSUM      |
-                   NETIF_F_RXCSUM      |
-                   NETIF_F_SCTP_CRC      |
-                   NETIF_F_GSO_UDP_TUNNEL |
-                   NETIF_F_GSO_GRE      |
-                   NETIF_F_TSO          |
+    netdev->hw_enc_features |= NETIF_F_IP_CSUM           |
+                   NETIF_F_TSO               |
+                   NETIF_F_TSO_ECN           |
+                   NETIF_F_GSO_GRE           |
+                   NETIF_F_GSO_UDP_TUNNEL      |
+                   NETIF_F_GSO_UDP_TUNNEL_CSUM |
                  0;
 
   netdev->features = NETIF_F_SG               |
@@ -9045,6 +9043,8 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 
   if (!(pf->flags & I40E_FLAG_MFP_ENABLED))
       netdev->features |= NETIF_F_NTUPLE;
+    if (pf->flags & I40E_FLAG_OUTER_UDP_CSUM_CAPABLE)
+        netdev->features |= NETIF_F_GSO_UDP_TUNNEL_CSUM;
 
   /* copy netdev features into list of user selectable features */
   netdev->hw_features |= netdev->features;
drivers/net/ethernet/intel/i40e/i40e_txrx.c +34 -23
index ca5f0881a5bf..ac2fa7b02e94 100644
@@ -2214,11 +2214,18 @@ out:
 static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb,
           u8 *hdr_len, u64 *cd_type_cmd_tso_mss)
 {
-    u32 cd_cmd, cd_tso_len, cd_mss;
-    struct ipv6hdr *ipv6h;
-    struct tcphdr *tcph;
-    struct iphdr *iph;
-    u32 l4len;
+    u64 cd_cmd, cd_tso_len, cd_mss;
+    union {
+        struct iphdr *v4;
+        struct ipv6hdr *v6;
+        unsigned char *hdr;
+    } ip;
+    union {
+        struct tcphdr *tcp;
+        struct udphdr *udp;
+        unsigned char *hdr;
+    } l4;
+    u32 paylen, l4_offset;
   int err;
 
   if (skb->ip_summed != CHECKSUM_PARTIAL)
@@ -2231,35 +2238,51 @@ static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb,
   if (err < 0)
       return err;
 
-    iph = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
-    ipv6h = skb->encapsulation ? inner_ipv6_hdr(skb) : ipv6_hdr(skb);
-
-    if (iph->version == 4) {
-        tcph = skb->encapsulation ? inner_tcp_hdr(skb) : tcp_hdr(skb);
-        iph->tot_len = 0;
-        iph->check = 0;
-        tcph->check = ~csum_tcpudp_magic(iph->saddr, iph->daddr,
-                         0, IPPROTO_TCP, 0);
-    } else if (ipv6h->version == 6) {
-        tcph = skb->encapsulation ? inner_tcp_hdr(skb) : tcp_hdr(skb);
-        ipv6h->payload_len = 0;
-        tcph->check = ~csum_ipv6_magic(&ipv6h->saddr, &ipv6h->daddr,
-                           0, IPPROTO_TCP, 0);
+    ip.hdr = skb_network_header(skb);
+    l4.hdr = skb_transport_header(skb);
+
+    if (skb->encapsulation) {
+        if (ip.v4->version == 4) {
+            ip.v4->check = 0;
+            ip.v4->tot_len = 0;
+        } else {
+            ip.v6->payload_len = 0;
+        }
+
+        /* if we end up needing to do any changes such as update
+         * the pseudo header checksum on the outer header we
+         * should do it here.
+         */
+
+        ip.hdr = skb_inner_network_header(skb);
+        l4.hdr = skb_inner_transport_header(skb);
   }
 
-    l4len = skb->encapsulation ? inner_tcp_hdrlen(skb) : tcp_hdrlen(skb);
-    *hdr_len = (skb->encapsulation
-            ? (skb_inner_transport_header(skb) - skb->data)
-            : skb_transport_offset(skb)) + l4len;
+    /* initialize IP header fields */
+    if (ip.v4->version == 4) {
+        ip.v4->check = 0;
+        ip.v4->tot_len = 0;

drivers/net/ethernet/intel/i40e/i40e_txrx.c static int i40e_tso(struct i40e_ring *tx_ring, struct sk_buff *skb, · line 2274

Quoted 10 lines
drivers/net/ethernet/intel/i40e/i40e_txrx.c +10 -0
+    } else {
+        ip.v6->payload_len = 0;
+    }
+
+    /* determine offset of transport header */
+    l4_offset = l4.hdr - skb->data;
+
+    /* remove payload length from checksum */
+    paylen = ntohs(1) * (u16)~(skb->len - l4_offset);
+    l4.tcp->check = ~csum_fold((__force __wsum)paylen + l4.tcp->check);

introduces sparse errors
CHECK /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c
/home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2520:18: warning: cast to restricted __be16 /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:37: warning: restricted __wsum degrades to integer /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:67: warning: restricted __sum16 degrades to integer /home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:59: warning: incorrect type in argument 1 (different base types)
/home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:59: expected restricted __wsum [usertype] sum
/home/jbrandeb/git/lad/i40e_linux_master/src/i40e/i40e_txrx.c:2521:59: got unsigned int

Quoted 75 lines
drivers/net/ethernet/intel/i40e/i40e_txrx.c +10 -16
+
+    /* compute length of segmentation header */
+    *hdr_len = (l4.tcp->doff * 4) + l4_offset;
 
   /* find the field values */
   cd_cmd = I40E_TX_CTX_DESC_TSO;
   cd_tso_len = skb->len - *hdr_len;
   cd_mss = skb_shinfo(skb)->gso_size;
-    *cd_type_cmd_tso_mss |= ((u64)cd_cmd << I40E_TXD_CTX_QW1_CMD_SHIFT) |
-                ((u64)cd_tso_len <<
-                 I40E_TXD_CTX_QW1_TSO_LEN_SHIFT) |
-                ((u64)cd_mss << I40E_TXD_CTX_QW1_MSS_SHIFT);
+    *cd_type_cmd_tso_mss |= (cd_cmd << I40E_TXD_CTX_QW1_CMD_SHIFT) |
+                (cd_tso_len << I40E_TXD_CTX_QW1_TSO_LEN_SHIFT) |
+                (cd_mss << I40E_TXD_CTX_QW1_MSS_SHIFT);
   return 1;
 }
 
@@ -2351,15 +2374,12 @@ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
       if (*tx_flags & I40E_TX_FLAGS_IPV4) {
           if (*tx_flags & I40E_TX_FLAGS_TSO) {
               *cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV4;
-                ip_hdr(skb)->check = 0;
           } else {
               *cd_tunneling |=
                    I40E_TX_CTX_EXT_IP_IPV4_NO_CSUM;
           }
       } else if (*tx_flags & I40E_TX_FLAGS_IPV6) {
           *cd_tunneling |= I40E_TX_CTX_EXT_IP_IPV6;
-            if (*tx_flags & I40E_TX_FLAGS_TSO)
-                ip_hdr(skb)->check = 0;
       }
 
       /* Now set the ctx descriptor fields */
@@ -2373,15 +2393,11 @@ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
           *tx_flags &= ~I40E_TX_FLAGS_IPV4;
           *tx_flags |= I40E_TX_FLAGS_IPV6;
       }
-        if ((tx_ring->flags & I40E_TXR_FLAGS_OUTER_UDP_CSUM) &&
-            (l4_tunnel == I40E_TXD_CTX_UDP_TUNNELING)        &&
-            (*cd_tunneling & I40E_TXD_CTX_QW0_EXT_IP_MASK)) {
-            oudph->check = ~csum_tcpudp_magic(oiph->saddr,
-                    oiph->daddr,
-                    (skb->len - skb_transport_offset(skb)),
-                    IPPROTO_UDP, 0);
+        /* indicate if we need to offload outer UDP header */
+        if ((*tx_flags & I40E_TX_FLAGS_TSO) &&
+            (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM))
           *cd_tunneling |= I40E_TXD_CTX_QW0_L4T_CS_MASK;
-        }
+
   } else {
       network_hdr_len = skb_network_header_len(skb);
       this_ip_hdr = ip_hdr(skb);
@@ -2397,7 +2413,6 @@ static void i40e_tx_enable_csum(struct sk_buff *skb, u32 *tx_flags,
        */
       if (*tx_flags & I40E_TX_FLAGS_TSO) {
           *td_cmd |= I40E_TX_DESC_CMD_IIPT_IPV4_CSUM;
-            this_ip_hdr->check = 0;
       } else {
           *td_cmd |= I40E_TX_DESC_CMD_IIPT_IPV4;
       }
drivers/net/ethernet/intel/i40e/i40e_txrx.h +0 -1
index 1d167b646193..fc77a271aba8 100644
@@ -275,7 +275,6 @@ struct i40e_ring {
 
   u16 flags;
 #define I40E_TXR_FLAGS_WB_ON_ITR    BIT(0)
-#define I40E_TXR_FLAGS_OUTER_UDP_CSUM    BIT(1)
 #define I40E_TXR_FLAGS_LAST_XMIT_MORE_SET BIT(2)
 
   /* stats structs */
Quoted 4 lines

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan

#4 lore raw

PATCH 2/3 i40e: Refactor Tx checksum offload

In reply to Alexander Duyck

This patch is meant to clean up a number of bugs in the Tx checksum offload for the i40e driver. The main issues all tend to be in the area of IPv6.

1.  IPv6 tunnel offloads not advertised
2.  IPv6 extension headers not skipped
3.  Use of iphdr(skb)->protocol in checksum path for IPv6
4.  Tx flags not reset when going from V6 to V4 for offloads

Specifically this driver was not offloading IPv6 under a number of circumstances, and in some cases such as IPv4 over an IPv6 based VXLAN tunnel it would fail outright to send any valid data.

---
 drivers/net/ethernet/intel/i40e/i40e_main.c |    2 
 drivers/net/ethernet/intel/i40e/i40e_txrx.c |  193 ++++++++++++++++-----------
 2 files changed, 115 insertions(+), 80 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +2 -0
index c21ad853d72f..a15ab5fd3bd7 100644
@@ -9017,7 +9017,9 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
 	np->vsi = vsi;
 
 	netdev->hw_enc_features |= NETIF_F_IP_CSUM	       |
+				   NETIF_F_IPV6_CSUM	       |
 				   NETIF_F_TSO		       |
+				   NETIF_F_TSO6		       |
 				   NETIF_F_TSO_ECN	       |
 				   NETIF_F_GSO_GRE	       |
 				   NETIF_F_GSO_UDP_TUNNEL      |
drivers/net/ethernet/intel/i40e/i40e_txrx.c +113 -80

245 lines not on this page · raw message

#5 lore raw

PATCH 3/3 i40e: Fix build warnings when geneve is not enabled

In reply to Alexander Duyck

My default config didn't have geneve enabled. As a result I was getting warnings about the port functions being defined but not used.

---
 drivers/net/ethernet/intel/i40e/i40e_main.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)
drivers/net/ethernet/intel/i40e/i40e_main.c +2 -4
index a15ab5fd3bd7..d35e6d62f659 100644
@@ -8664,6 +8664,7 @@ static void i40e_del_vxlan_port(struct net_device *netdev,
 #endif
 }
 
+#if IS_ENABLED(CONFIG_GENEVE)
 /**
  * i40e_add_geneve_port - Get notifications about GENEVE ports that come up
  * @netdev: This physical port's netdev
@@ -8673,7 +8674,6 @@ static void i40e_del_vxlan_port(struct net_device *netdev,
 static void i40e_add_geneve_port(struct net_device *netdev,
 				 sa_family_t sa_family, __be16 port)
 {
-#if IS_ENABLED(CONFIG_GENEVE)
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_vsi *vsi = np->vsi;
 	struct i40e_pf *pf = vsi->back;
@@ -8708,7 +8708,6 @@ static void i40e_add_geneve_port(struct net_device *netdev,
 	pf->flags |= I40E_FLAG_UDP_FILTER_SYNC;
 
 	dev_info(&pf->pdev->dev, "adding geneve port %d\n", ntohs(port));
-#endif
 }
 
 /**
@@ -8720,7 +8719,6 @@ static void i40e_add_geneve_port(struct net_device *netdev,
 static void i40e_del_geneve_port(struct net_device *netdev,
 				 sa_family_t sa_family, __be16 port)
 {
-#if IS_ENABLED(CONFIG_GENEVE)
 	struct i40e_netdev_priv *np = netdev_priv(netdev);
 	struct i40e_vsi *vsi = np->vsi;
 	struct i40e_pf *pf = vsi->back;
@@ -8746,8 +8744,8 @@ static void i40e_del_geneve_port(struct net_device *netdev,
 		netdev_warn(netdev, "geneve port %d was not found, not deleting\n",
 			    ntohs(port));
 	}
-#endif
 }
+#endif
 
 static int i40e_get_phys_port_id(struct net_device *netdev,
 				 struct netdev_phys_item_id *ppid)
#6 lore raw

PATCH 3/3 i40e: Fix build warnings when geneve is not enabled

Quoted 8 lines

-----Original Message-----
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Alexander Duyck
Sent: Thursday, January 14, 2016 8:42 PM
To: intel-wired-lan at lists.osuosl.org
Subject: [Intel-wired-lan] [next PATCH 3/3] i40e: Fix build warnings when geneve is not enabled

Quoted 7 lines

My default config didn't have geneve enabled. As a result I was getting warnings about the port functions being defined but not used.

---
 drivers/net/ethernet/intel/i40e/i40e_main.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Patch code changes correctly applied,no build warnings w/ Geneve disabled

#7 lore raw

cover TSO and checksum fixes for i40e

In reply to Alexander Duyck
Quoted 9 lines · Alexander Duyck

This patch set is meant to improve the performance and reliability of i40e when it comes to performing TSO and Tx checksum offloads related to tunnels.

I have tested it with a number of combinations of v4 over v6 and v6 over v4 for VXLANs. With this patch set I resolved a number of issues and I am now able to perform TSO for any of them as long as the outer UDP checksum is 0. It should also now be supported if the outer checksum is enabled in the case of the XL722, though I cannot test it.

Quoted 9 lines

I was able to verify IPv6 TSO is working correctly, however it seems like if I use VXLAN over IPv6 I am not seeing any receive checksum offloads. I'm still trying to work out that bit but as far as I can tell I haven't configured anything incorrectly as the receiving end is getting the frames in the proper format.

I'll be out on vacation for the next several days so if anything comes up I will address it on Tuesday.

Jeff,

Between the fix from Arnd for the build issue and the sparse warnings I will be submitting a v2 of my patches. Could you please drop the existing set from the tree?

Thanks.

-Alex

#8 lore raw

cover TSO and checksum fixes for i40e

Quoted 4 lines · Alexander Duyck

Between the fix from Arnd for the build issue and the sparse warnings I will be submitting a v2 of my patches.? Could you please drop the existing set from the tree?

Yeah, I will drop it. ?Are you wanting me to pick up Arnd's fix? ?or Eric D's fix? ?or neither and take your v2 build fix?

I want to get that build fix pushed later today if possible so that I
won't get any more "fixes" for the same issue.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160120/eb58ec46/attachment.asc>

#9 lore raw

cover TSO and checksum fixes for i40e

Quoted 8 lines · Jeff Kirsher
Quoted 4 lines · Alexander Duyck

Between the fix from Arnd for the build issue and the sparse warnings I will be submitting a v2 of my patches. Could you please drop the existing set from the tree?

Yeah, I will drop it. Are you wanting me to pick up Arnd's fix? or Eric D's fix? or neither and take your v2 build fix?

I was thinking Arnd's fix, I haven't seen Eric D's. I just know that mine was only partial compared to the patch Arnd provided.

Thanks.

- Alex