app/testpmd: fix TX checksum calculation for tunnel

-----Original Message-----
From: dev <[email protected]> On Behalf Of Gregory Etelson
Sent: Monday, July 19, 2021 11:33 AM

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.
If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

Reviewed-by Dmitry Kozlyuk <[email protected]>

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

app/test-pmd/csumonly.c index renamed from app/test-pmd/csumonly.c +0 -0

089936587b..a658cd5389 100644

app/test-pmd/csumonly.c +4 -3

@@ -480,17 +480,18 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 
 	if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV4)) {
 		ipv4_hdr = l3_hdr;
-		ipv4_hdr->hdr_checksum = 0;
 
 		ol_flags |= PKT_TX_IPV4;
 		if (info->l4_proto == IPPROTO_TCP && tso_segsz) {
 			ol_flags |= PKT_TX_IP_CKSUM;
 		} else {
-			if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
+			if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) {
 				ol_flags |= PKT_TX_IP_CKSUM;
-			else
+			} else if (ipv4_hdr->hdr_checksum) {
+				ipv4_hdr->hdr_checksum = 0;
 				ipv4_hdr->hdr_checksum =
 					rte_ipv4_cksum(ipv4_hdr);
+			}
 		}
 	} else if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV6))

app/test-pmd/csumonly.c +3 -3

 		ol_flags |= PKT_TX_IPV6;

@@ -501,10 +502,10 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 		udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info-
 >l3_len);
 		/* do not recalculate udp cksum if it was 0 */
 		if (udp_hdr->dgram_cksum != 0) {
-			udp_hdr->dgram_cksum = 0;
-			if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM)
+			if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
 				ol_flags |= PKT_TX_UDP_CKSUM;
-			else {
+			} else if (udp_hdr->dgram_cksum) {
+				udp_hdr->dgram_cksum = 0;
 				udp_hdr->dgram_cksum =

get_udptcp_checksum(l3_hdr,
udp_hdr,

app/test-pmd/csumonly.c +3 -3

 						info->ethertype);

@@ -514,12 +515,12 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 			ol_flags |= PKT_TX_UDP_SEG;
 	} else if (info->l4_proto == IPPROTO_TCP) {
 		tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info-
 >l3_len);
-		tcp_hdr->cksum = 0;
 		if (tso_segsz)
 			ol_flags |= PKT_TX_TCP_SEG;
-		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM)
+		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
 			ol_flags |= PKT_TX_TCP_CKSUM;
-		else {
+		} else if (tcp_hdr->cksum) {
+			tcp_hdr->cksum = 0;
 			tcp_hdr->cksum =

app/test-pmd/csumonly.c +2 -2

 				get_udptcp_checksum(l3_hdr, tcp_hdr,
 					info->ethertype);

@@ -529,13 +530,13 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 	} else if (info->l4_proto == IPPROTO_SCTP) {
 		sctp_hdr = (struct rte_sctp_hdr *)
 			((char *)l3_hdr + info->l3_len);
-		sctp_hdr->cksum = 0;
 		/* sctp payload must be a multiple of 4 to be
 		 * offloaded */
 		if ((tx_offloads & DEV_TX_OFFLOAD_SCTP_CKSUM) &&
 			((ipv4_hdr->total_length & 0x3) == 0)) {
 			ol_flags |= PKT_TX_SCTP_CKSUM;
-		} else {
+		} else if (sctp_hdr->cksum) {
+			sctp_hdr->cksum = 0;
 			/* XXX implement CRC32c, example available in
 			 * RFC3309 */

}
--
2.31.1

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result. If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

app/test-pmd/csumonly.c +2 -0

+				ipv4_hdr->hdr_checksum = 0;
 				ipv4_hdr->hdr_checksum =
 					rte_ipv4_cksum(ipv4_hdr);
+			}

Please we need more reviews for this patch.

19/07/2021 10:33, Gregory Etelson:

Quoted 8 lines

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.
If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner

checksums.

Quoted 8 lines

If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

nit: no blank line between Fixes and Cc lines please

Quoted 5 lines

Signed-off-by Gregory Etelson <[email protected]>

Reviewed-by Dmitry Kozlyuk <[email protected]>

---
+ } else if (ipv4_hdr->hdr_checksum) {

Please do an explicit comparison with 0 here as it cannot be considered as a boolean test.

Quoted 4 lines

app/test-pmd/csumonly.c +2 -0

+                             ipv4_hdr->hdr_checksum = 0;
                              ipv4_hdr->hdr_checksum =
                                      rte_ipv4_cksum(ipv4_hdr);
+                     }

-----Original Message-----
From: Gregory Etelson <[email protected]>
Sent: Tuesday, July 27, 2021 21:08
To: [email protected]
Cc: [email protected]; Ajit Khaparde <[email protected]>; Olivier Matz <[email protected]>; Andrew Rybchenko <[email protected]>; Yigit, Ferruh <[email protected]>; Thomas Monjalon <[email protected]>; [email protected]; Li, Xiaoyun <[email protected]>
Subject: [PATCH v2] app/testpmd: fix TX checksum calculation for tunnel

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.
If TX checksum is calculated by a software, existing value must be zeroed first. The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

---
v2:
remove blank line between Fixes and Cc
explicitly compare with 0 value in if ()

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

app/test-pmd/csumonly.c index renamed from app/test-pmd/csumonly.c +0 -0

0161f72175..bd5ad64a57 100644

app/test-pmd/csumonly.c +4 -3

@@ -480,17 +480,18 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 
 	if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV4)) {
 		ipv4_hdr = l3_hdr;
-		ipv4_hdr->hdr_checksum = 0;
 
 		ol_flags |= PKT_TX_IPV4;
 		if (info->l4_proto == IPPROTO_TCP && tso_segsz) {
 			ol_flags |= PKT_TX_IP_CKSUM;
 		} else {
-			if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
+			if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) {
 				ol_flags |= PKT_TX_IP_CKSUM;
-			else
+			} else if (ipv4_hdr->hdr_checksum != 0) {
+				ipv4_hdr->hdr_checksum = 0;
 				ipv4_hdr->hdr_checksum =
 					rte_ipv4_cksum(ipv4_hdr);
+			}
 		}
 	} else if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV6))

app/test-pmd/csumonly.c +0 -0

 		ol_flags |= PKT_TX_IPV6;

app/test-pmd/csumonly.c +1 -0

+				udp_hdr->dgram_cksum = 0;
 				udp_hdr->dgram_cksum =
 					get_udptcp_checksum(l3_hdr, udp_hdr,

app/test-pmd/csumonly.c +3 -3

 						info->ethertype);

@@ -514,12 +515,12 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 			ol_flags |= PKT_TX_UDP_SEG;
 	} else if (info->l4_proto == IPPROTO_TCP) {
 		tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
-		tcp_hdr->cksum = 0;
 		if (tso_segsz)
 			ol_flags |= PKT_TX_TCP_SEG;
-		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM)
+		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
 			ol_flags |= PKT_TX_TCP_CKSUM;
-		else {
+		} else if (tcp_hdr->cksum != 0) {
+			tcp_hdr->cksum = 0;
 			tcp_hdr->cksum =
 				get_udptcp_checksum(l3_hdr, tcp_hdr,

app/test-pmd/csumonly.c +2 -2

 					info->ethertype);

@@ -529,13 +530,13 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 	} else if (info->l4_proto == IPPROTO_SCTP) {
 		sctp_hdr = (struct rte_sctp_hdr *)
 			((char *)l3_hdr + info->l3_len);
-		sctp_hdr->cksum = 0;
 		/* sctp payload must be a multiple of 4 to be
 		 * offloaded */
 		if ((tx_offloads & DEV_TX_OFFLOAD_SCTP_CKSUM) &&
 			((ipv4_hdr->total_length & 0x3) == 0)) {
 			ol_flags |= PKT_TX_SCTP_CKSUM;
-		} else {
+		} else if (sctp_hdr->cksum != 0) {
+			sctp_hdr->cksum = 0;
 			/* XXX implement CRC32c, example available in
 			 * RFC3309 */

}
--
2.32.0

Quoted 9 lines

Subject: [PATCH v2] app/testpmd: fix TX checksum calculation for tunnel

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.
If TX checksum is calculated by a software, existing value must be zeroed

first.

Quoted 50 lines

The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

---
v2:
remove blank line between Fixes and Cc explicitly compare with 0
value in if ()

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

app/test-pmd/csumonly.c index renamed from app/test-pmd/csumonly.c +0 -0

0161f72175..bd5ad64a57 100644

app/test-pmd/csumonly.c +4 -3

@@ -480,17 +480,18 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 
      if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV4)) {
              ipv4_hdr = l3_hdr;
-             ipv4_hdr->hdr_checksum = 0;
 
              ol_flags |= PKT_TX_IPV4;
              if (info->l4_proto == IPPROTO_TCP && tso_segsz) {
                      ol_flags |= PKT_TX_IP_CKSUM;
              } else {
-                     if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
+                     if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) {
                              ol_flags |= PKT_TX_IP_CKSUM;
-                     else
+                     } else if (ipv4_hdr->hdr_checksum != 0) {
+                             ipv4_hdr->hdr_checksum = 0;
                              ipv4_hdr->hdr_checksum =
                                      rte_ipv4_cksum(ipv4_hdr);
+                     }
              }
      } else if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV6))

              ol_flags |= PKT_TX_IPV6; @@ -501,10 +502,10 @@
process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info
*info,
              udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
              /* do not recalculate udp cksum if it was 0 */
              if (udp_hdr->dgram_cksum != 0) {

app/test-pmd/csumonly.c process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info, · line 507

Quoted 6 lines

app/test-pmd/csumonly.c +2 -3

-                     udp_hdr->dgram_cksum = 0;
-                     if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM)
+                     if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
                              ol_flags |= PKT_TX_UDP_CKSUM;
-                     else {
+                     } else {

else if (udp_hdr->dgram_cksum != 0) { ?

Quoted 40 lines

+                             udp_hdr->dgram_cksum = 0;
                              udp_hdr->dgram_cksum =
                                      get_udptcp_checksum(l3_hdr, udp_hdr,
                                              info->ethertype); @@
-514,12 +515,12 @@ process_inner_cksums(void *l3_hdr, const struct
testpmd_offload_info *info,
                      ol_flags |= PKT_TX_UDP_SEG;
      } else if (info->l4_proto == IPPROTO_TCP) {
              tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);

app/test-pmd/csumonly.c +4 -3

-             tcp_hdr->cksum = 0;
              if (tso_segsz)
                      ol_flags |= PKT_TX_TCP_SEG;
-             else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM)
+             else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
                      ol_flags |= PKT_TX_TCP_CKSUM;
-             else {
+             } else if (tcp_hdr->cksum != 0) {
+                     tcp_hdr->cksum = 0;
                      tcp_hdr->cksum =
                              get_udptcp_checksum(l3_hdr, tcp_hdr,
                                      info->ethertype); @@ -529,13
+530,13 @@ process_inner_cksums(void *l3_hdr, const struct

testpmd_offload_info *info,

      } else if (info->l4_proto == IPPROTO_SCTP) {
              sctp_hdr = (struct rte_sctp_hdr *)
                      ((char *)l3_hdr + info->l3_len);

app/test-pmd/csumonly.c +2 -3

-             sctp_hdr->cksum = 0;
              /* sctp payload must be a multiple of 4 to be
               * offloaded */
              if ((tx_offloads & DEV_TX_OFFLOAD_SCTP_CKSUM) &&
                      ((ipv4_hdr->total_length & 0x3) == 0)) {
                      ol_flags |= PKT_TX_SCTP_CKSUM;
-             } else {
+             } else if (sctp_hdr->cksum != 0) {
+                     sctp_hdr->cksum = 0;
                      /* XXX implement CRC32c, example available in
                       * RFC3309 */
              }
--

2.32.0

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result. If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

-----Original Message-----
From: Gregory Etelson <[email protected]>
Sent: Tuesday, July 27, 2021 21:08
To: [email protected]
Cc: [email protected]; Ajit Khaparde <[email protected]>; Olivier Matz <[email protected]>; Andrew Rybchenko <[email protected]>; Yigit, Ferruh <[email protected]>; Thomas Monjalon <[email protected]>; [email protected]; Li, Xiaoyun <[email protected]>
Subject: [PATCH v2] app/testpmd: fix TX checksum calculation for tunnel

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.
If TX checksum is calculated by a software, existing value must be zeroed first. The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

---
v2:
remove blank line between Fixes and Cc
explicitly compare with 0 value in if ()

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.

If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

---
v2:
remove blank line between Fixes and Cc
explicitly compare with 0 value in if ()

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

app/test-pmd/csumonly.c +12 -11

index 0161f72175..bd5ad64a57 100644
@@ -480,17 +480,18 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 
 	if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV4)) {
 		ipv4_hdr = l3_hdr;
-		ipv4_hdr->hdr_checksum = 0;
 
 		ol_flags |= PKT_TX_IPV4;
 		if (info->l4_proto == IPPROTO_TCP && tso_segsz) {
 			ol_flags |= PKT_TX_IP_CKSUM;
 		} else {
-			if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
+			if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) {
 				ol_flags |= PKT_TX_IP_CKSUM;
-			else
+			} else if (ipv4_hdr->hdr_checksum != 0) {
+				ipv4_hdr->hdr_checksum = 0;
 				ipv4_hdr->hdr_checksum =
 					rte_ipv4_cksum(ipv4_hdr);
+			}
 		}
 	} else if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV6))
 		ol_flags |= PKT_TX_IPV6;

@@ -501,10 +502,10 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 		udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
 		/* do not recalculate udp cksum if it was 0 */
 		if (udp_hdr->dgram_cksum != 0) {
-			udp_hdr->dgram_cksum = 0;
-			if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM)
+			if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
 				ol_flags |= PKT_TX_UDP_CKSUM;
-			else {
+			} else {
+				udp_hdr->dgram_cksum = 0;
 				udp_hdr->dgram_cksum =
 					get_udptcp_checksum(l3_hdr, udp_hdr,
 						info->ethertype);

@@ -514,12 +515,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 			ol_flags |= PKT_TX_UDP_SEG;
 	} else if (info->l4_proto == IPPROTO_TCP) {
 		tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);
-		tcp_hdr->cksum = 0;
 		if (tso_segsz)
 			ol_flags |= PKT_TX_TCP_SEG;
-		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM)
+		else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
 			ol_flags |= PKT_TX_TCP_CKSUM;
-		else {
+		} else if (tcp_hdr->cksum != 0) {
+			tcp_hdr->cksum = 0;
 			tcp_hdr->cksum =
 				get_udptcp_checksum(l3_hdr, tcp_hdr,
 					info->ethertype);

@@ -529,13 +530,13 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,
 	} else if (info->l4_proto == IPPROTO_SCTP) {
 		sctp_hdr = (struct rte_sctp_hdr *)
 			((char *)l3_hdr + info->l3_len);
-		sctp_hdr->cksum = 0;
 		/* sctp payload must be a multiple of 4 to be
 		 * offloaded */
 		if ((tx_offloads & DEV_TX_OFFLOAD_SCTP_CKSUM) &&
 			((ipv4_hdr->total_length & 0x3) == 0)) {
 			ol_flags |= PKT_TX_SCTP_CKSUM;
-		} else {
+		} else if (sctp_hdr->cksum != 0) {
+			sctp_hdr->cksum = 0;
 			/* XXX implement CRC32c, example available in
 			 * RFC3309 */
 		}

Signature · 1 line

2.32.0

Quoted 5 lines

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.

This is not always true. Actually, the checksum value is optionally set by software to the value that is expected by the hardware to offload the checksum correctly. This is done through rte_eth_tx_prepare(), which is called in csumonly test engine.

For instance, on an ixgbe NIC, it does:

rte_eth_tx_prepare()
eth_dev->tx_pkt_prepare()
ixgbe_prep_pkts()
rte_net_intel_cksum_flags_prepare()
if packet is IP, set IP checksum to 0
if packet is TCP or UDP, set L4 checksum to the phdr csum

This driver-specific rte_eth_tx_prepare() can indeed do nothing and let the hardware ignore the checksum in the packet.

Quoted 7 lines

If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Sorry, I think I don't understand the issue. Are you trying to compute the inner checksum by hardware and the outer checksum by software?

Quoted 1 line

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

I'm not sure the problem origin is this commit (however, I may have misunderstood your issue).

At the time this commit was done, it was required to set the TCP/UDP checksum to the pseudo header checksum to offload an L4 checksum. See:
https://git.dpdk.org/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=51f694dd40f5
#n107

The introduction of rte_eth_tx_prepare() API removed this need, see:
https://git.dpdk.org/dpdk/commit/?id=6b520d54ebfe

Thanks,
Olivier

Quoted 38 lines

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

---
v2:
remove blank line between Fixes and Cc explicitly compare with 0
value in if ()

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

app/test-pmd/csumonly.c index renamed from app/test-pmd/csumonly.c +0 -0

0161f72175..bd5ad64a57 100644

app/test-pmd/csumonly.c +4 -3

@@ -480,17 +480,18 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 
      if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV4)) {
              ipv4_hdr = l3_hdr;
-             ipv4_hdr->hdr_checksum = 0;
 
              ol_flags |= PKT_TX_IPV4;
              if (info->l4_proto == IPPROTO_TCP && tso_segsz) {
                      ol_flags |= PKT_TX_IP_CKSUM;
              } else {
-                     if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
+                     if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) {
                              ol_flags |= PKT_TX_IP_CKSUM;
-                     else
+                     } else if (ipv4_hdr->hdr_checksum != 0) {
+                             ipv4_hdr->hdr_checksum = 0;
                              ipv4_hdr->hdr_checksum =
                                      rte_ipv4_cksum(ipv4_hdr);
+                     }
              }
      } else if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV6))

              ol_flags |= PKT_TX_IPV6; @@ -501,10 +502,10 @@

Quoted 1 line

process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info

*info,

Quoted 4 lines

              udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
              /* do not recalculate udp cksum if it was 0 */
              if (udp_hdr->dgram_cksum != 0) {

app/test-pmd/csumonly.c +0 -1

-                     udp_hdr->dgram_cksum = 0;

app/test-pmd/csumonly.c process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,

Quoted 10 lines

app/test-pmd/csumonly.c +3 -3

-                     if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM)
+                     if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
                              ol_flags |= PKT_TX_UDP_CKSUM;
-                     else {
+                     } else {
+                             udp_hdr->dgram_cksum = 0;
                              udp_hdr->dgram_cksum =
                                      get_udptcp_checksum(l3_hdr, udp_hdr,
                                              info->ethertype); @@
-514,12 +515,12 @@ process_inner_cksums(void *l3_hdr, const struct

testpmd_offload_info *info,

Quoted 6 lines

                      ol_flags |= PKT_TX_UDP_SEG;
      } else if (info->l4_proto == IPPROTO_TCP) {
              tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);

app/test-pmd/csumonly.c +0 -1

-             tcp_hdr->cksum = 0;
              if (tso_segsz)
                      ol_flags |= PKT_TX_TCP_SEG;

app/test-pmd/csumonly.c process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,

Quoted 10 lines

app/test-pmd/csumonly.c +4 -2

-             else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM)
+             else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
                      ol_flags |= PKT_TX_TCP_CKSUM;
-             else {
+             } else if (tcp_hdr->cksum != 0) {
+                     tcp_hdr->cksum = 0;
                      tcp_hdr->cksum =
                              get_udptcp_checksum(l3_hdr, tcp_hdr,
                                      info->ethertype); @@ -529,13
+530,13 @@ process_inner_cksums(void *l3_hdr, const struct

testpmd_offload_info *info,

Quoted 18 lines

      } else if (info->l4_proto == IPPROTO_SCTP) {
              sctp_hdr = (struct rte_sctp_hdr *)
                      ((char *)l3_hdr + info->l3_len);

app/test-pmd/csumonly.c +2 -3

-             sctp_hdr->cksum = 0;
              /* sctp payload must be a multiple of 4 to be
               * offloaded */
              if ((tx_offloads & DEV_TX_OFFLOAD_SCTP_CKSUM) &&
                      ((ipv4_hdr->total_length & 0x3) == 0)) {
                      ol_flags |= PKT_TX_SCTP_CKSUM;
-             } else {
+             } else if (sctp_hdr->cksum != 0) {
+                     sctp_hdr->cksum = 0;
                      /* XXX implement CRC32c, example available in
                       * RFC3309 */
              }
--

2.32.0

Hello Oliver,

Please see my comments below

Quoted 21 lines · Gregory Etelson

Quoted 5 lines

TX checksum of a tunnelled packet can be calculated for outer headers only or for both outer and inner parts. The calculation method is determined by application.
If TX checksum calculation can be offloaded, hardware ignores existing checksum value and replaces it with an updated result.

This is not always true. Actually, the checksum value is optionally set by software to the value that is expected by the hardware to offload the checksum correctly. This is done through rte_eth_tx_prepare(), which is called in csumonly test engine.

For instance, on an ixgbe NIC, it does:

rte_eth_tx_prepare()
eth_dev->tx_pkt_prepare()
ixgbe_prep_pkts()
rte_net_intel_cksum_flags_prepare()
if packet is IP, set IP checksum to 0
if packet is TCP or UDP, set L4 checksum to the phdr csum

Quoted 3 lines

This driver-specific rte_eth_tx_prepare() can indeed do nothing and let the hardware ignore the checksum in the packet.

You are right. I'll update the patch comment in v3.

Quoted 8 lines

Quoted 7 lines

If TX checksum is calculated by a software, existing value must be zeroed first.
The testpmd checksum forwarding engine always zeroed inner checksums. If inner checksum calculation was offloaded, that header was left with 0 checksum value.
Following outer software checksum calculation produced wrong value. The patch zeroes inner IPv4 checksum only before software calculation.

Quoted 3 lines

Sorry, I think I don't understand the issue. Are you trying to compute the inner checksum by hardware and the outer checksum by software?

Correct. Inner checksum is offloaded and outer computed in software.

Consider this example:
Tunneled packet arrived at port A and being forwarded through port B. The packet arrived at port A with correct inner checksums - L3 and L4. Port B TX offloads inner L3 only.

process_inner_cksums() sets "ipv4_hdr->hdr_checksum = 0;" unconditionally.
Inner L3 checksum value will be restored by port B TX checksum offload, but when process_outer_cksums() runs software calculation on outer L4 it will use 0 and produce wrong result.

Therefore, the patch zeros inner checksum values only before actual software calculations.

Quoted 6 lines

Quoted 2 lines

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

I'm not sure the problem origin is this commit (however, I may have misunderstood your issue).

Quoted 7 lines

At the time this commit was done, it was required to set the TCP/UDP checksum to the pseudo header checksum to offload an L4 checksum. See:
https://git.dpdk.org/dpdk/tree/lib/librte_mbuf/rte_mbuf.h?id=51f694dd40f5
#n107

The introduction of rte_eth_tx_prepare() API removed this need, see:
https://git.dpdk.org/dpdk/commit/?id=6b520d54ebfe

Quoted 93 lines

Thanks,
Olivier

Quoted 38 lines

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

---
v2:
remove blank line between Fixes and Cc explicitly compare with 0
value in if ()

---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

app/test-pmd/csumonly.c index renamed from app/test-pmd/csumonly.c +0 -0

0161f72175..bd5ad64a57 100644

app/test-pmd/csumonly.c +4 -3

@@ -480,17 +480,18 @@ process_inner_cksums(void *l3_hdr, const struct
 testpmd_offload_info *info,
 
      if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV4)) {
              ipv4_hdr = l3_hdr;
-             ipv4_hdr->hdr_checksum = 0;
 
              ol_flags |= PKT_TX_IPV4;
              if (info->l4_proto == IPPROTO_TCP && tso_segsz) {
                      ol_flags |= PKT_TX_IP_CKSUM;
              } else {
-                     if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM)
+                     if (tx_offloads & DEV_TX_OFFLOAD_IPV4_CKSUM) {
                              ol_flags |= PKT_TX_IP_CKSUM;
-                     else
+                     } else if (ipv4_hdr->hdr_checksum != 0) {
+                             ipv4_hdr->hdr_checksum = 0;
                              ipv4_hdr->hdr_checksum =
                                      rte_ipv4_cksum(ipv4_hdr);
+                     }
              }
      } else if (info->ethertype == _htons(RTE_ETHER_TYPE_IPV6))

              ol_flags |= PKT_TX_IPV6; @@ -501,10 +502,10 @@

Quoted 1 line

process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info

*info,

Quoted 4 lines

              udp_hdr = (struct rte_udp_hdr *)((char *)l3_hdr + info->l3_len);
              /* do not recalculate udp cksum if it was 0 */
              if (udp_hdr->dgram_cksum != 0) {

app/test-pmd/csumonly.c +0 -1

-                     udp_hdr->dgram_cksum = 0;

app/test-pmd/csumonly.c process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,

Quoted 10 lines

app/test-pmd/csumonly.c +3 -3

-                     if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM)
+                     if (tx_offloads & DEV_TX_OFFLOAD_UDP_CKSUM) {
                              ol_flags |= PKT_TX_UDP_CKSUM;
-                     else {
+                     } else {
+                             udp_hdr->dgram_cksum = 0;
                              udp_hdr->dgram_cksum =
                                      get_udptcp_checksum(l3_hdr, udp_hdr,
                                              info->ethertype); @@
-514,12 +515,12 @@ process_inner_cksums(void *l3_hdr, const struct

testpmd_offload_info *info,

Quoted 6 lines

                      ol_flags |= PKT_TX_UDP_SEG;
      } else if (info->l4_proto == IPPROTO_TCP) {
              tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len);

app/test-pmd/csumonly.c +0 -1

-             tcp_hdr->cksum = 0;
              if (tso_segsz)
                      ol_flags |= PKT_TX_TCP_SEG;

app/test-pmd/csumonly.c process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info,

Quoted 10 lines

app/test-pmd/csumonly.c +4 -2

-             else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM)
+             else if (tx_offloads & DEV_TX_OFFLOAD_TCP_CKSUM) {
                      ol_flags |= PKT_TX_TCP_CKSUM;
-             else {
+             } else if (tcp_hdr->cksum != 0) {
+                     tcp_hdr->cksum = 0;
                      tcp_hdr->cksum =
                              get_udptcp_checksum(l3_hdr, tcp_hdr,
                                      info->ethertype); @@ -529,13
+530,13 @@ process_inner_cksums(void *l3_hdr, const struct

testpmd_offload_info *info,

Quoted 18 lines

      } else if (info->l4_proto == IPPROTO_SCTP) {
              sctp_hdr = (struct rte_sctp_hdr *)
                      ((char *)l3_hdr + info->l3_len);

app/test-pmd/csumonly.c +2 -3

-             sctp_hdr->cksum = 0;
              /* sctp payload must be a multiple of 4 to be
               * offloaded */
              if ((tx_offloads & DEV_TX_OFFLOAD_SCTP_CKSUM) &&
                      ((ipv4_hdr->total_length & 0x3) == 0)) {
                      ol_flags |= PKT_TX_SCTP_CKSUM;
-             } else {
+             } else if (sctp_hdr->cksum != 0) {
+                     sctp_hdr->cksum = 0;
                      /* XXX implement CRC32c, example available in
                       * RFC3309 */
              }
--

2.32.0

Quoted 2 lines

Correct. Inner checksum is offloaded and outer computed in software.

I think this approach is not sane: the value of the outer checksum depends on the inner checksum, so it has to be calculated after. There is a comment in the code about this:

        /* Then process outer headers if any. Note that the software
         * checksum will be wrong if one of the inner checksums is
         * processed in hardware. */
        if (info.is_tunnel == 1) {
                tx_ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
                                tx_offloads,
                                !!(tx_ol_flags & PKT_TX_TCP_SEG));
        }

Quoted 6 lines

Consider this example:
Tunneled packet arrived at port A and being forwarded through port B. The packet arrived at port A with correct inner checksums - L3 and L4. Port B TX offloads inner L3 only.

process_inner_cksums() sets "ipv4_hdr->hdr_checksum = 0;"

unconditionally.

Quoted 3 lines

Inner L3 checksum value will be restored by port B TX checksum offload, but when
process_outer_cksums() runs software calculation on outer L4 it will use 0

and produce wrong result.

Quoted 1 line

Quoted 1 line

Therefore, the patch zeros inner checksum values only before actual

software calculations.

I better understand your use case, thanks.

However, with your patch, if the inner L4 checksum is wrong when it arrives on port A, I think it will result in a packet with a wrong outer L4 checksum and a correct inner L4 checksum. Is it what you expect?

I don't argue against the patch itself. What you suggest better matches the offload API than what we have today. Can you please send another version that better explains the use-case?

One more suggestion, maybe for later. Currently, the csumonly engine can be configured to do the checksum in sw or in hw. Maybe we could add a "dont- touch" option, to keep the value in the packet. Would it help for your use- case?

Hello Olivier,

[:snip:]

Quoted 7 lines

Quoted 2 lines

Correct. Inner checksum is offloaded and outer computed in software.

I think this approach is not sane: the value of the outer checksum depends on the inner checksum, so it has to be calculated after. There is a comment in the code about this:

Quoted 8 lines

        /* Then process outer headers if any. Note that the software
         * checksum will be wrong if one of the inner checksums is
         * processed in hardware. */
        if (info.is_tunnel == 1) {
                tx_ol_flags |= process_outer_cksums(outer_l3_hdr, &info,
                                tx_offloads,
                                !!(tx_ol_flags & PKT_TX_TCP_SEG));
        }

I agree. That computation order led to error in my case. What if the engine could analyze port TX offload options and select processing order according to existing configuration ?

Quoted 17 lines

Quoted 6 lines

Consider this example:
Tunneled packet arrived at port A and being forwarded through port B. The packet arrived at port A with correct inner checksums - L3 and L4. Port B TX offloads inner L3 only.

process_inner_cksums() sets "ipv4_hdr->hdr_checksum = 0;"

unconditionally.

Quoted 3 lines

Inner L3 checksum value will be restored by port B TX checksum offload, but when
process_outer_cksums() runs software calculation on outer L4 it will use 0

and produce wrong result.

Quoted 2 lines

Therefore, the patch zeros inner checksum values only before actual

software calculations.

I better understand your use case, thanks.

Quoted 4 lines

However, with your patch, if the inner L4 checksum is wrong when it arrives on port A, I think it will result in a packet with a wrong outer L4 checksum and a correct inner L4 checksum. Is it what you expect?

Wrong checksum reflects ongoing issue that must be investigated and fixed. I don't expect forwarding engine to fix wrong checksum because it can hide a real problem.

Quoted 4 lines

I don't argue against the patch itself. What you suggest better matches the offload API than what we have today. Can you please send another version that better explains the use-case?

I posted v3 with updated comment.

Quoted 5 lines

One more suggestion, maybe for later. Currently, the csumonly engine can be configured to do the checksum in sw or in hw. Maybe we could add a "dont- touch" option, to keep the value in the packet. Would it help for your use- case?

That's a very good idea.
Packets can arrive with pre-calculated correct checksums. Keeping these values can save processing time.

[:snip:]

Regards,
Gregory

csumonly engine calculates TX checksum of a tunnelled packet for outer headers only or separately for outer and inner headers. The calculation method is determined by checksum configuration options. If TX checksum calculation is separated, the inner headers are processed before outer headers.

Inner headers processing sets checksum values to 0 unconditionally. If TX configuration offloads inner checksums only, outer checksum calculation in software will read 0 instead of real values and produce wrong result.

The patch zeroes inner checksums only before software calculation.

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

Quoted 3 lines

Fixes 51f694dd40f5 ("app/testpmd: rework checksum forward engine")

Cc [email protected]

As said previously, I think the correct Fixes line is:

Fixes 6b520d54ebfe ("app/testpmd: use Tx preparation in checksum engine")

Acked-by Olivier Matz <[email protected]>

Thanks

csumonly engine calculates TX checksum of a tunnelled packet for outer headers only or separately for outer and inner headers. The calculation method is determined by checksum configuration options. If TX checksum calculation is separated, the inner headers are processed before outer headers.

Inner headers processing sets checksum values to 0 unconditionally. If TX configuration offloads inner checksums only, outer checksum calculation in software will read 0 instead of real values and produce wrong result.

The patch zeroes inner checksums only before software calculation.

Fixes 6b520d54ebfe ("app/testpmd: use Tx preparation in checksum engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

Quoted 11 lines

csumonly engine calculates TX checksum of a tunnelled packet for outer headers only or separately for outer and inner headers. The calculation method is determined by checksum configuration options. If TX checksum calculation is separated, the inner headers are processed before outer headers.

Inner headers processing sets checksum values to 0 unconditionally. If TX configuration offloads inner checksums only, outer checksum calculation in software will read 0 instead of real values and produce wrong result.

Quoted 6 lines

The patch zeroes inner checksums only before software calculation.

Fixes 6b520d54ebfe ("app/testpmd: use Tx preparation in checksum engine")

Cc [email protected]

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Olivier Matz <[email protected]>

-----Original Message-----
From: dev <[email protected]> On Behalf Of Thomas Monjalon
Sent: Friday, July 30, 2021 8:04 PM
To: Gregory Etelson <[email protected]>
Cc: [email protected]; Ajit Khaparde <[email protected]>; Andrew Rybchenko <[email protected]>; Yigit, Ferruh <[email protected]>; [email protected]; Li, Xiaoyun <[email protected]>; Tomasz Kulasek <[email protected]>; Ananyev, Konstantin <[email protected]>; Olivier Matz <[email protected]>
Subject: Re: [dpdk-dev] [PATCH v4] app/testpmd: fix TX checksum calculation for tunnel

30/07/2021 10:39, Olivier Matz:

Quoted 14 lines · Gregory Etelson

Quoted 13 lines

csumonly engine calculates TX checksum of a tunnelled packet for outer headers only or separately for outer and inner headers. The calculation method is determined by checksum configuration options. If TX checksum calculation is separated, the inner headers are processed before outer headers.

Inner headers processing sets checksum values to 0 unconditionally. If TX configuration offloads inner checksums only, outer checksum calculation in software will read 0 instead of real values and produce wrong result.

The patch zeroes inner checksums only before software calculation.

Quoted 8 lines

Quoted 5 lines

Fixes 6b520d54ebfe ("app/testpmd: use Tx preparation in checksum

engine")
Cc: [email protected]

Signed-off-by Gregory Etelson <[email protected]>

Acked-by Olivier Matz <[email protected]>

The previous acks were forgotten (it should be added manually in the patch):

Acked-by Ori Kam <[email protected]>

Acked-by Ajit Khaparde <[email protected]>

Acked-by Xiaoyun Li <[email protected]>

Applied, thanks.