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 05CCB43279; Fri, 3 Nov 2023 10:09:20 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C885940284; Fri, 3 Nov 2023 10:09:19 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id C34A54027F for ; Fri, 3 Nov 2023 10:09:17 +0100 (CET) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4SMFL001NyzvQYJ; Fri, 3 Nov 2023 17:09:11 +0800 (CST) Received: from [10.67.121.59] (10.67.121.59) by kwepemm000004.china.huawei.com (7.193.23.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.31; Fri, 3 Nov 2023 17:09:09 +0800 Message-ID: <2eff5f84-d929-2bf9-a9cd-f1f603eb2090@huawei.com> Date: Fri, 3 Nov 2023 17:09:09 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 Subject: Re: [PATCH v2] app/testpmd: fix UDP cksum error for UFO enable To: Ferruh Yigit , CC: , , , References: <20230728021310.15970-1-lihuisong@huawei.com> <20230802025520.8000-1-lihuisong@huawei.com> <62dbcb50-31b0-41a2-9d83-a53b01abd0a6@amd.com> From: "lihuisong (C)" In-Reply-To: <62dbcb50-31b0-41a2-9d83-a53b01abd0a6@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems704-chm.china.huawei.com (10.3.19.181) To kwepemm000004.china.huawei.com (7.193.23.18) X-CFilter-Loop: Reflected 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 Hi Ferruh, Thanks for you review. 在 2023/11/3 9:31, Ferruh Yigit 写道: > On 8/2/2023 3:55 AM, Huisong Li wrote: >> The command "tso set " is used to enable UFO, please >> see commit ce8e6e742807 ("app/testpmd: support UFO in checksum engine") >> >> The above patch configures the RTE_MBUF_F_TX_UDP_SEG to enable UFO only if >> tso_segsz is set. >> > "The above patch sets the RTE_MBUF_F_TX_UDP_SEG in mbuf ol_flags, only > by checking if 'tso_segsz' is set, but missing check if UFO offload > (RTE_ETH_TX_OFFLOAD_UDP_TSO) supported by device." Ack > > >> Then tx_prepare() may call rte_net_intel_cksum_prepare() >> to compute pseudo header checksum (because some PMDs may supports TSO). >> > Not sure what do you mean by '(because some PMDs may supports TSO)'? > > Do you mean something like following: > "RTE_MBUF_F_TX_UDP_SEG flag causes driver that supports TSO/UFO to > compute pseudo header checksum." Ack > > >> As a result, if the peer sends UDP packets, all packets with UDP checksum >> error are received for the PMDs only supported TSO. >> > "As a result, if device only supports TSO, but not UFO, UDP packet > checksum will be wrong." Ack > > >> So enabling UFO also depends on if driver has RTE_ETH_TX_OFFLOAD_UDP_TSO >> capability. Similarly, TSO also need to do like this. >> >> In addition, this patch also fixes cmd_tso_set_parsed() for UFO to make >> it better to support TSO and UFO. >> >> Fixes: ce8e6e742807 ("app/testpmd: support UFO in checksum engine") >> >> Signed-off-by: Huisong Li >> --- >> v2: add handle for tunnel TSO offload in process_inner_cksums >> >> --- >> app/test-pmd/cmdline.c | 47 +++++++++++++++++++++-------------------- >> app/test-pmd/csumonly.c | 11 ++++++++-- >> 2 files changed, 33 insertions(+), 25 deletions(-) >> >> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c >> index 0d0723f659..8be593d405 100644 >> --- a/app/test-pmd/cmdline.c >> +++ b/app/test-pmd/cmdline.c >> @@ -4906,6 +4906,7 @@ cmd_tso_set_parsed(void *parsed_result, >> { >> struct cmd_tso_set_result *res = parsed_result; >> struct rte_eth_dev_info dev_info; >> + uint64_t offloads; >> int ret; >> >> if (port_id_is_invalid(res->port_id, ENABLED_WARN)) >> @@ -4922,37 +4923,37 @@ cmd_tso_set_parsed(void *parsed_result, >> if (ret != 0) >> return; >> >> - if ((ports[res->port_id].tso_segsz != 0) && >> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) { >> - fprintf(stderr, "Error: TSO is not supported by port %d\n", >> - res->port_id); >> - return; >> + if (ports[res->port_id].tso_segsz != 0) { >> + if ((dev_info.tx_offload_capa & (RTE_ETH_TX_OFFLOAD_TCP_TSO | >> + RTE_ETH_TX_OFFLOAD_UDP_TSO)) == 0) { >> + fprintf(stderr, "Error: both TSO and UFO are not supported by port %d\n", >> + res->port_id); >> + return; >> + } >> + /* display warnings if configuration is not supported by the NIC */ >> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) >> + fprintf(stderr, "Warning: port %d doesn't support TSO\n", >> + res->port_id); >> + if ((dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) == 0) >> + fprintf(stderr, "Warning: port %d doesn't support UFO\n", >> + res->port_id); >> > Requesting TSO/UFO by setting 'tso_segsz', but device capability missing > is an error case, so OK to have first message. > > But only supporting TSO or UFO is not an error case, not sure about > logging this. But even it is logged, I think it shouldn't be to stderr > or it should say "Warning: ", a regular logging can be done. All right, will fix it in next version. > > >> } >> >> if (ports[res->port_id].tso_segsz == 0) { >> ports[res->port_id].dev_conf.txmode.offloads &= >> - ~RTE_ETH_TX_OFFLOAD_TCP_TSO; >> - printf("TSO for non-tunneled packets is disabled\n"); >> + ~(RTE_ETH_TX_OFFLOAD_TCP_TSO | RTE_ETH_TX_OFFLOAD_UDP_TSO); >> + printf("TSO and UFO for non-tunneled packets is disabled\n"); >> } else { >> - ports[res->port_id].dev_conf.txmode.offloads |= >> - RTE_ETH_TX_OFFLOAD_TCP_TSO; >> - printf("TSO segment size for non-tunneled packets is %d\n", >> + offloads = (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) ? >> + RTE_ETH_TX_OFFLOAD_TCP_TSO : 0; >> + offloads |= (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_UDP_TSO) ? >> + RTE_ETH_TX_OFFLOAD_UDP_TSO : 0; >> + ports[res->port_id].dev_conf.txmode.offloads |= offloads; >> + printf("segment size for non-tunneled packets is %d\n", >> ports[res->port_id].tso_segsz); >> } >> - cmd_config_queue_tx_offloads(&ports[res->port_id]); >> - >> - /* display warnings if configuration is not supported by the NIC */ >> - ret = eth_dev_info_get_print_err(res->port_id, &dev_info); >> - if (ret != 0) >> - return; >> - >> - if ((ports[res->port_id].tso_segsz != 0) && >> - (dev_info.tx_offload_capa & RTE_ETH_TX_OFFLOAD_TCP_TSO) == 0) { >> - fprintf(stderr, >> - "Warning: TSO enabled but not supported by port %d\n", >> - res->port_id); >> - } >> > Above is redundant check, and introduced with commit [1], I assume by > mistake. Yes, it is a redundant check indeed. This check is introduced in the first patch[1]. But the patch [2] add offload capabilities check but don't delete the old check. [1] Fixes: b51c47536a9e ("app/testpmd: support TSO in checksum forward engine") [2] Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check") > Since removing above check is not related to UFO, what do you > think to separate it to its own patch? ok, will separate it from this patch. > > [1] > Fixes: 3926dd2b6668 ("app/testpmd: enforce offload capabilities check") > > >> + cmd_config_queue_tx_offloads(&ports[res->port_id]); >> cmd_reconfig_device_queue(res->port_id, 1, 1); >> } >> >> diff --git a/app/test-pmd/csumonly.c b/app/test-pmd/csumonly.c >> index c103e54111..21210aff43 100644 >> --- a/app/test-pmd/csumonly.c >> +++ b/app/test-pmd/csumonly.c >> @@ -466,6 +466,12 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info, >> uint64_t ol_flags = 0; >> uint32_t max_pkt_len, tso_segsz = 0; >> uint16_t l4_off; >> + uint64_t all_tunnel_tso = RTE_ETH_TX_OFFLOAD_VXLAN_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_GRE_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_IPIP_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_GENEVE_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_IP_TNL_TSO | >> + RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO; >> >> /* ensure packet is large enough to require tso */ >> if (!info->is_tunnel) { >> @@ -505,7 +511,7 @@ 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) { >> - if (tso_segsz) >> + if (tso_segsz && (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_TSO)) >> ol_flags |= RTE_MBUF_F_TX_UDP_SEG; >> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_UDP_CKSUM) { >> ol_flags |= RTE_MBUF_F_TX_UDP_CKSUM; >> @@ -528,7 +534,8 @@ process_inner_cksums(void *l3_hdr, const struct testpmd_offload_info *info, >> #endif >> } else if (info->l4_proto == IPPROTO_TCP) { >> tcp_hdr = (struct rte_tcp_hdr *)((char *)l3_hdr + info->l3_len); >> - if (tso_segsz) >> + if (tso_segsz && >> + (tx_offloads & (RTE_ETH_TX_OFFLOAD_TCP_TSO | all_tunnel_tso))) >> > Should we check 'all_tunnel_tso', and why they are checked only for TCP? Yes, this patch is just for TCP_TSO and UDP_TSO. But here is necessary for tunnel_tso, or this doesn't set 'RTE_MBUF_F_TX_TCP_SEG' flag for tunnel tso. > > As far as I can see some tunnel TSO offloads should case setting > relevant mbuf flags, like RTE_MBUF_F_TX_TUNNEL_UDP or > RTE_ETH_TX_OFFLOAD_IP_TNL_TSO. > > With above check, if RTE_ETH_TX_OFFLOAD_TCP_TSO not set but only > RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO set, we still set 'RTE_MBUF_F_TX_TCP_SEG' > flag but not 'RTE_MBUF_F_TX_TUNNEL_UDP' flag. At least here didn't change the original behavior for tunnel tso. I'm not still clear how to set these flag for tunnel tso. But I can ensure that 'RTE_MBUF_F_TX_TCP_SEG' flag is must for tunnel tso. > > I assume intention is to be close to previous implementation, where only > tso_segsz checked, and cover as much as possible TSO offload requests, > but not sure if this is accurate with expected usage. we may need to do something for tunnel tso command as this patch did. I will take a look at it after this patch. > >> ol_flags |= RTE_MBUF_F_TX_TCP_SEG; >> else if (tx_offloads & RTE_ETH_TX_OFFLOAD_TCP_CKSUM) { >> ol_flags |= RTE_MBUF_F_TX_TCP_CKSUM; > .