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 79FC743295; Mon, 6 Nov 2023 13:29:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 66E8F4064A; Mon, 6 Nov 2023 13:29:57 +0100 (CET) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 2ABFF402EF for ; Mon, 6 Nov 2023 13:29:55 +0100 (CET) Received: from kwepemm000004.china.huawei.com (unknown [172.30.72.53]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4SP9ZY0DqXzrTwC; Mon, 6 Nov 2023 20:26:45 +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 20:29:53 +0800 Message-ID: <32a1c1ee-2c2c-9109-64b0-2a7ee64358bf@huawei.com> Date: Mon, 6 Nov 2023 20:29:52 +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> <5746ebed-ff7d-1c3c-4abc-5de45ea15d10@huawei.com> <7819d317-c0d6-457c-9c93-5b561fabd242@amd.com> From: "lihuisong (C)" In-Reply-To: <7819d317-c0d6-457c-9c93-5b561fabd242@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.59] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) 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/6 18:09, Ferruh Yigit 写道: > On 11/6/2023 4:13 AM, lihuisong (C) wrote: >> 在 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? >> > I don't mean this patch, but existing code. It doesn't set > 'RTE_MBUF_F_TX_TUNNEL_UDP' or 'RTE_MBUF_F_TX_TUNNEL_IP' mbuf flags when > relevant TSO offload requested. get it. I will check "cmd_tunnel_tso_set_parsed" after this patch and see if we need to do something for tunnel tso in testpmd. Will send v3 according to your advice ASAP. > > > >>> 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; >>>>> . >>> . > .