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 46D1743D1E; Fri, 22 Mar 2024 07:28:33 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id CF90E42DF1; Fri, 22 Mar 2024 07:28:32 +0100 (CET) Received: from szxga05-in.huawei.com (szxga05-in.huawei.com [45.249.212.191]) by mails.dpdk.org (Postfix) with ESMTP id F3AC842DE9 for ; Fri, 22 Mar 2024 07:28:29 +0100 (CET) Received: from mail.maildlp.com (unknown [172.19.88.163]) by szxga05-in.huawei.com (SkyGuard) with ESMTP id 4V1C7Q0Sysz1GCK2; Fri, 22 Mar 2024 14:28:02 +0800 (CST) Received: from kwepemd100004.china.huawei.com (unknown [7.221.188.31]) by mail.maildlp.com (Postfix) with ESMTPS id 4B14418002D; Fri, 22 Mar 2024 14:28:27 +0800 (CST) Received: from [10.67.121.175] (10.67.121.175) by kwepemd100004.china.huawei.com (7.221.188.31) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1258.28; Fri, 22 Mar 2024 14:28:26 +0800 Message-ID: Date: Fri, 22 Mar 2024 14:28:25 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled From: Jie Hai To: Ferruh Yigit , huangdengdui , CC: , , References: <20240206011030.2007689-1-haijie1@huawei.com> <11b8feac-4a9e-4d2c-8995-ed492d684750@amd.com> <7438563e-c7b4-6e13-68bf-74ff423546af@huawei.com> <6246e1f8-dcd4-468d-a05d-2e292f6e1714@amd.com> <6c831a66-f916-48d4-68d9-4c3bcfcb4979@huawei.com> <1ec105cf-c8d2-4010-867d-30970c25a2a1@amd.com> <7e376ab0-67a0-4a3c-a528-7928077e7b56@huawei.com> <760c70e6-ca2d-4d5e-9a05-809b81d32dd3@huawei.com> <541e9d04-bed3-47b0-8b11-746f9047e1a0@amd.com> In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.175] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To kwepemd100004.china.huawei.com (7.221.188.31) 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 Kindly ping for reply. Thanks, Jie Hai On 2024/3/8 19:36, Jie Hai wrote: > On 2024/3/1 19:10, Ferruh Yigit wrote: >> On 3/1/2024 6:55 AM, huangdengdui wrote: >>> >>> >>> On 2024/2/29 17:25, Ferruh Yigit wrote: >>>> On 2/29/2024 3:58 AM, huangdengdui wrote: >>>>> >>>>> >>>>> On 2024/2/28 21:07, Ferruh Yigit wrote: >>>>>> On 2/28/2024 2:27 AM, huangdengdui wrote: >>>>>>> >>>>>>> >>>>>>> On 2024/2/27 0:43, Ferruh Yigit wrote: >>>>>>>> On 2/26/2024 3:16 AM, Jie Hai wrote: >>>>>>>>> On 2024/2/23 21:53, Ferruh Yigit wrote: >>>>>>>>>> On 2/20/2024 3:58 AM, Jie Hai wrote: >>>>>>>>>>> Hi, Ferruh, >>>>>>>>>>> >>>>>>>>>>> Thanks for your review. >>>>>>>>>>> >>>>>>>>>>> On 2024/2/7 22:15, Ferruh Yigit wrote: >>>>>>>>>>>> On 2/6/2024 1:10 AM, Jie Hai wrote: >>>>>>>>>>>>> From: Dengdui Huang >>>>>>>>>>>>> >>>>>>>>>>>>> When KEEP_CRC offload is enabled, some packets will be >>>>>>>>>>>>> truncated and >>>>>>>>>>>>> the CRC is still be stripped in following cases: >>>>>>>>>>>>> 1. For HIP08 hardware, the packet type is TCP and the length >>>>>>>>>>>>>       is less than or equal to 60B. >>>>>>>>>>>>> 2. For other hardwares, the packet type is IP and the length >>>>>>>>>>>>>       is less than or equal to 60B. >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> If a device doesn't support the offload by some packets, it >>>>>>>>>>>> can be >>>>>>>>>>>> option to disable offload for that device, instead of >>>>>>>>>>>> calculating it in >>>>>>>>>>>> software and append it. >>>>>>>>>>> >>>>>>>>>>> The KEEP CRC feature of hns3 is faulty only in the specific >>>>>>>>>>> packet >>>>>>>>>>> type and small packet(<60B) case. >>>>>>>>>>> What's more, the small ethernet packet is not common. >>>>>>>>>>> >>>>>>>>>>>> Unless you have a specific usecase, or requirement to >>>>>>>>>>>> support the >>>>>>>>>>>> offload. >>>>>>>>>>> >>>>>>>>>>> Yes, some users of hns3 are already using this feature. >>>>>>>>>>> So we cannot drop this offload >>>>>>>>>>> >>>>>>>>>>>> <...> >>>>>>>>>>>> >>>>>>>>>>>>> @@ -2492,10 +2544,16 @@ hns3_recv_pkts_simple(void *rx_queue, >>>>>>>>>>>>>                 goto pkt_err; >>>>>>>>>>>>>               rxm->packet_type = hns3_rx_calc_ptype(rxq, >>>>>>>>>>>>> l234_info, >>>>>>>>>>>>> ol_info); >>>>>>>>>>>>> - >>>>>>>>>>>>>             if (rxm->packet_type == >>>>>>>>>>>>> RTE_PTYPE_L2_ETHER_TIMESYNC) >>>>>>>>>>>>>                 rxm->ol_flags |= RTE_MBUF_F_RX_IEEE1588_PTP; >>>>>>>>>>>>>     +        if (unlikely(rxq->crc_len > 0)) { >>>>>>>>>>>>> +            if (hns3_need_recalculate_crc(rxq, rxm)) >>>>>>>>>>>>> +                hns3_recalculate_crc(rxq, rxm); >>>>>>>>>>>>> +            rxm->pkt_len -= rxq->crc_len; >>>>>>>>>>>>> +            rxm->data_len -= rxq->crc_len; >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> Removing 'crc_len' from 'mbuf->pkt_len' & 'mbuf->data_len' is >>>>>>>>>>>> practically same as stripping CRC. >>>>>>>>>>>> >>>>>>>>>>>> We don't count CRC length in the statistics, but it should be >>>>>>>>>>>> accessible >>>>>>>>>>>> in the payload by the user. >>>>>>>>>>> Our drivers are behaving exactly as you say. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> If so I missed why mbuf 'pkt_len' and 'data_len' reduced by >>>>>>>>>> 'rxq->crc_len', can you please explain what above lines does? >>>>>>>>>> >>>>>>>>>> >>>>>>>>> @@ -2470,8 +2523,7 @@ hns3_recv_pkts_simple(void *rx_queue, >>>>>>>>>           rxdp->rx.bd_base_info = 0; >>>>>>>>> >>>>>>>>>           rxm->data_off = RTE_PKTMBUF_HEADROOM; >>>>>>>>> -        rxm->pkt_len = >>>>>>>>> (uint16_t)(rte_le_to_cpu_16(rxd.rx.pkt_len)) - >>>>>>>>> -                rxq->crc_len; >>>>>>>>> +        rxm->pkt_len = rte_le_to_cpu_16(rxd.rx.pkt_len); >>>>>>>>> >>>>>>>>> In the previous code above, the 'pkt_len' is set to the length >>>>>>>>> obtained >>>>>>>>> from the BD. the length obtained from the BD already contains >>>>>>>>> CRC length. >>>>>>>>> But as you said above, the DPDK requires that the length of the >>>>>>>>> mbuf >>>>>>>>> does not contain CRC length . So we subtract 'rxq->crc_len' from >>>>>>>>> mbuf'pkt_len' and 'data_len'. This patch doesn't change the >>>>>>>>> logic, it >>>>>>>>> just moves the code around. >>>>>>>>> >>>>>>>> >>>>>>>> Nope, I am not saying mbuf length shouldn't contain CRC length, >>>>>>>> indeed >>>>>>>> it is other way around and this is our confusion. >>>>>>>> >>>>>>>> CRC length shouldn't be in the statistics, I mean in received >>>>>>>> bytes stats. >>>>>>>> Assume that received packet is 128 bytes and we know it has the >>>>>>>> CRC, >>>>>>>> Rx received bytes stat should be 124 (rx_bytes = 128 - CRC = 124) >>>>>>>> >>>>>>>> But mbuf->data_len & mbuf->pkt_len should have full frame length, >>>>>>>> including CRC. >>>>>>>> >>>>>>>> As application explicitly requested to KEEP CRC, it will know >>>>>>>> last 4 >>>>>>>> bytes are CRC. >>>>>>>> Anything after 'mbuf->data_len' in the mbuf buffer is not valid, >>>>>>>> so if >>>>>>>> you reduce 'mbuf->data_len' by CRC size, application can't know >>>>>>>> if 4 >>>>>>>> bytes after 'mbuf->data_len' is valid CRC or not. >>>>>>>> >>>>>>> I agree with you. >>>>>>> >>>>>>> But the implementation of other PMDs supported KEEP_CRC is like >>>>>>> this. >>>>>>> In addition, there are probably many users that are already using >>>>>>> it. >>>>>>> If we modify it, it may cause applications incompatible. >>>>>>> >>>>>>> what do you think? >>>>>>> >>>>>> This is documented in the ethdev [1], better to follow the >>>>>> documentation >>>>>> for all PMDs, can you please highlight the relevant driver code, >>>>>> we can >>>>>> discuss it with their maintainers. >>>>>> >>>>>> Alternatively we can document this additionally in the KEEP_CRC >>>>>> feature >>>>>> document if it helps for the applications. >>>>>> >>>>>> >>>>>> [1] >>>>>> https://git.dpdk.org/dpdk/tree/lib/ethdev/rte_ethdev.h?h=v23.11#n257 >>>>> >>>>> Currently，this documentation does not describe whether pkt_len and >>>>> data_len should contain crc_len. >>>>> >>>> >>>> I think it is clear that pkt_len and data_len should contain >>>> crc_len, we >>>> can ask for more comments. >>> This patch doesn't change the logic for hns3 PMD and the >>> implementation of >>> other PMDs supported KEEP_CRC is like hns3 PMD. Can we merge this >>> patch first? >>> >> >> If hns3 behaving against the documented behavior, I don't understand why >> you are pushing for merging this patch, instead of fixing it. >> >> >> Other drivers behavior is something else, not directly related to this >> patch, but again if you can provide references we can discuss with their >> maintainers. >> >> >>>> >>>>> Do you mean that we add this description in the KEEP_CRC feature >>>>> document >>>>> and notify all drivers that support KEEP_CRC to follow this >>>>> documentation? >>>>> >>>>> If so, can you merge this patch first? >>>>> Then we send a RFC to disscuss it with all PMDs maintainer. >>>>> >>>> >>>> Not for drivers, just a suggestion that if we should update feature >>>> documentation with above information for users. So there is no >>>> dependency to features document update. >>>> >>>> >>> Sorry I'm more confused. What should we do next? >> >> There is already API documentation about KEEP_CRC, I think that is >> already sufficient for driver developers. >> >> I am just brainstorming if updating './doc/guides/nics/features.rst' can >> help end user, but it is not an action or blocker for this patch. >> >> Next step is to update this path. >> > > > Hi, Ferruh, > > Thanks for your attention. > I have the following suggestions for the email discussion. > Please take the time to see if they make sense. > > For pkt_len and data_len, there is no clear document indicating > whether the length should include the CRC. > However, according to the usage of the driver and the APP, it > is obvious that almost all drivers do not include the CRC by default. > > The issue you raised about pkt_len/data_len supposedly containing CRC > and users not being able to get CRC has been around for a long > time, at least dating back to DPDK 18.11 when there was no hns3 > driver. And there is no clear solution to this problem for now. > > This issue is not caused by the current patch and is not related > to the problem to be solved by the current patch. > Therefore, it is recommended that the problem corresponding to the current > patch should be fixed first. > > The problem that the pkt_len/data_len should contain the CRC > and the user should access the CRC can be discussed later. > > I have the following two options: > > 1. Modify the corresponding document to specify that pkt_len/data_len > should contain CRC, and modify all related drivers. This requires the > participation and discussion of other driver developers. > > 2. Users can use rte_pktmbuf_dump() to print packet data. > The length of the packet data to be printed can be specified. > However, the API restricts that the length of the data required is > less than data_len. Therefore, users cannot obtain the CRC value. > However, the result of the rte_pktmbuf_tailroom() API can tell how > many bytes after data_len are accessible. Users can use > rte_pktmbuf_tailroom > and keep_crc offload to obtain the CRC value of packets. > >> .