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 B8B23432B4; Mon, 6 Nov 2023 05:14:00 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 97630402DD; Mon, 6 Nov 2023 05:14:00 +0100 (CET) Received: from szxga08-in.huawei.com (szxga08-in.huawei.com [45.249.212.255]) by mails.dpdk.org (Postfix) with ESMTP id 7D3F3402B5 for ; Mon, 6 Nov 2023 05:13:58 +0100 (CET) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.57]) by szxga08-in.huawei.com (SkyGuard) with ESMTP id 4SNyZG6PhZz1P7kR; Mon, 6 Nov 2023 12:10:46 +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; Mon, 6 Nov 2023 12:13:53 +0800 Message-ID: <5746ebed-ff7d-1c3c-4abc-5de45ea15d10@huawei.com> Date: Mon, 6 Nov 2023 12:13:53 +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> <2eff5f84-d929-2bf9-a9cd-f1f603eb2090@huawei.com> From: "lihuisong (C)" In-Reply-To: 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 在 2023/11/3 18:42, Ferruh Yigit 写道: > On 11/3/2023 9:09 AM, lihuisong (C) wrote: >> 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. >> > Lets say 'RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO' is requested, but > 'RTE_ETH_TX_OFFLOAD_TCP_TSO' is not requested, should we still set the > 'RTE_MBUF_F_TX_TCP_SEG' flag? Yes, RTE_MBUF_F_TX_TCP_SEG flag still should be set for tunnel tso. Driver compute pseudo header checksum and fill hw descriptor based on this flag. > > I am not really clear how to handle tunnel TSO offloads, but considering > previous implementation was only relying on 'tso_segsz', continue with > all TSO offload will be similar to previous implementation, so OK to > have it. Yes > > And with same logic, should we add 'all_tunnel_tso' check to the UDP case? I didn't see some offloads about UDP_TSO for tunnel packet. And testpmd just support a command (please see cmd_tunnel_tso_set_parsed) to set these tunnel tso offloads this patch mentioned. > > > And agree that setting other tunnel related mbuf flags is out of scope > for this patch, but probably that part is missing in this code, and only What specific thing is missing in this code? > a few drivers support these flags anyway. > >>> 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; >>> . > .