diff mbox series

app/testpmd: fix TX checksum calculation for tunnel

Message ID 20210719083309.15428-1-getelson@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series app/testpmd: fix TX checksum calculation for tunnel |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/github-robot success github build: passed
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS

Commit Message

Gregory Etelson July 19, 2021, 8:33 a.m. UTC 
  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: stable@dpdk.org

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
Reviewed-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
 app/test-pmd/csumonly.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

Comments

Ori Kam July 21, 2021, 6:42 a.m. UTC | #1 
Hi Gregory,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> 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: stable@dpdk.org
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> Reviewed-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
>  app/test-pmd/csumonly.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index
> 089936587b..a658cd5389 100644
> --- a/app/test-pmd/csumonly.c
> +++ b/app/test-pmd/csumonly.c
> @@ -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))
>  		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,
>  						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 =
>  				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

Acked-by: Ori Kam <orika@nvidia.com>
Thanks,
Ori
Thomas Monjalon July 24, 2021, 11:37 a.m. UTC | #2 
Please we need more reviews for this patch.

19/07/2021 10:33, Gregory Etelson:
> 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: stable@dpdk.org

nit: no blank line between Fixes and Cc lines please

> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> Reviewed-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> ---
> +			} else if (ipv4_hdr->hdr_checksum) {

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

> +				ipv4_hdr->hdr_checksum = 0;
>  				ipv4_hdr->hdr_checksum =
>  					rte_ipv4_cksum(ipv4_hdr);
> +			}
Gregory Etelson July 24, 2021, 12:43 p.m. UTC | #3 
> Please we need more reviews for this patch.
> 

I'll update the patch and post a v2.

> 19/07/2021 10:33, Gregory Etelson:
> > 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: stable@dpdk.org
> 
> nit: no blank line between Fixes and Cc lines please
> 
> >
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > Reviewed-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
> > ---
> > +                     } else if (ipv4_hdr->hdr_checksum) {
> 
> Please do an explicit comparison with 0 here as it cannot be considered as a
> boolean test.
> 
> > +                             ipv4_hdr->hdr_checksum = 0;
> >                               ipv4_hdr->hdr_checksum =
> >                                       rte_ipv4_cksum(ipv4_hdr);
> > +                     }
> 
>
diff mbox series

Patch

diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c
index 089936587b..a658cd5389 100644
--- a/app/test-pmd/csumonly.c
+++ b/app/test-pmd/csumonly.c
@@ -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))
 		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,
 						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 =
 				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 */
 		}