From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id CB4C0A0C40; Thu, 29 Jul 2021 10:25:33 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 544B440687; Thu, 29 Jul 2021 10:25:33 +0200 (CEST) Received: from mail-wr1-f48.google.com (mail-wr1-f48.google.com [209.85.221.48]) by mails.dpdk.org (Postfix) with ESMTP id 25BE140041 for ; Thu, 29 Jul 2021 10:25:32 +0200 (CEST) Received: by mail-wr1-f48.google.com with SMTP id q3so5889986wrx.0 for ; Thu, 29 Jul 2021 01:25:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=Vr3X+fib5+VtKG0Xv8J08wqKEH9/zt5Uy7HMD2++ZSY=; b=d5xU5bkTkCU2xefYxGEyIXtjH02mutD3czLlf6+fD0jNTEF4MfSMMZvr/lMMT2AFDr uH3PjhwOp/Q4eak/zQDmKEqaagDy+PApmEz/PNQ9lsFYXLeu1MrB9TaBtk1qZuBLOI/P onVZN5lvMslqyoBNXkmKuxpSeem3S2kd8HpfYVuKQJNmApmfJ6yyqBUpw6dM01cfLMLS J5zzLavBL6E5Opy/wf0V10ZNA3cobHlorRSj46GvZmYmy9zFV+J6MSfYM7dsvTvJsBKl Fva4UVcsXr05nXP0Mr1xSURFuiMlm9k0fFWY3mhr4nube/6WMWa1TMkkg2qC7bW2Gyvf 3wtA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=Vr3X+fib5+VtKG0Xv8J08wqKEH9/zt5Uy7HMD2++ZSY=; b=jxFjV+9pm/4/t6QIT2finNp3bPa76hUg9JUsSglBsYzaDhqbv61cpWoKuXQSBtduWz go5SUdYD2jDM8SdyY1r2/7xkPPTRNEPWE0tuV04r6i3A4U3MUk4htYNyhNtv1BBEXR8P 9725DYFXpw7cJycLIaQUXxfjmf5YZZ5rTNZ7rSYI85iwu3v7yXpfndaOmRViN0NdGVP7 1+VXovzrQI+cCZiDSHl4Bdgfz/CKTUgfOo0gqCgo3YeC7Wiem/qvuL0i0/GKd9+JjGCU 0WVGhAP8LneAahtMJe71Vw40/LUfO4pMqDVTDz7qOBZTO2bD4Yo9odZX4WMUgaJq7iyE Sgdg== X-Gm-Message-State: AOAM530C+zXHmIXlRrPtLwD0d2/Vue24yBXqrDpQxeiGJxeoaSBF+z7m mMl2EieoRTmsFwzAHY/GsY9BuA== X-Google-Smtp-Source: ABdhPJwACG84WJQCzq4/1UqfYbI9zBFJ3Q6athBySQvR8KUUI7fwZkZ8klGICrsRSr6DdfZMYPMwhQ== X-Received: by 2002:adf:fc0d:: with SMTP id i13mr3346652wrr.276.1627547131743; Thu, 29 Jul 2021 01:25:31 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id l2sm2650255wry.32.2021.07.29.01.25.30 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 29 Jul 2021 01:25:31 -0700 (PDT) Date: Thu, 29 Jul 2021 10:25:30 +0200 From: Olivier Matz To: Gregory Etelson Cc: "dev@dpdk.org" , Ajit Khaparde , Andrew Rybchenko , Ferruh Yigit , NBU-Contact-Thomas Monjalon , "stable@dpdk.org" , Xiaoyun Li Message-ID: References: <20210719083309.15428-1-getelson@nvidia.com> <20210727130757.30724-1-getelson@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [dpdk-dev] [PATCH v2] app/testpmd: fix TX checksum calculation for tunnel X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Wed, Jul 28, 2021 at 04:07:51PM +0000, Gregory Etelson wrote: > Hello Oliver, > > Please see my comments below > > > On Tue, Jul 27, 2021 at 04:07:57PM +0300, Gregory Etelson wrote: > > > 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. > > > > You are right. I'll update the patch comment in v3. > > > > 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? > > > > 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)); } > 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. 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? > > > > 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 Just a reminder for this one. Thanks, Olivier > > Thanks, > > Olivier > > > > > Cc: stable@dpdk.org > > > > > > Signed-off-by: Gregory Etelson > > > --- > > > 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(-) > > > > > > diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c index > > > 0161f72175..bd5ad64a57 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 != 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 */ > > > } > > > -- > > > 2.32.0 > > >