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 920AB44140; Mon, 3 Jun 2024 03:38:29 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 18488402B7; Mon, 3 Jun 2024 03:38:29 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 7B6AD4029C for ; Mon, 3 Jun 2024 03:38:26 +0200 (CEST) Received: from mail.maildlp.com (unknown [172.19.163.174]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4Vsx913J9xzxQPw; Mon, 3 Jun 2024 09:34:29 +0800 (CST) Received: from kwepemf500004.china.huawei.com (unknown [7.202.181.242]) by mail.maildlp.com (Postfix) with ESMTPS id 9A398140428; Mon, 3 Jun 2024 09:38:23 +0800 (CST) Received: from [10.67.121.175] (10.67.121.175) by kwepemf500004.china.huawei.com (7.202.181.242) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1544.11; Mon, 3 Jun 2024 09:38:20 +0800 Message-ID: Date: Mon, 3 Jun 2024 09:38:19 +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 To: Ferruh Yigit , huangdengdui , , "John W. Linville" , Ciara Loftus , Shepard Siegel , Ed Czeck , John Miller , Igor Russkikh , Steven Webster , Matt Peters , Selwin Sebastian , Julien Aube , Ajit Khaparde , Somnath Kotur , Chas Williams , "Min Hu (Connor)" , Nithin Dabilpuram , Kiran Kumar K , Sunil Kumar Kori , Satha Rao , Harman Kalra , Yuying Zhang , Rahul Lakkireddy , Hemant Agrawal , Sachin Saxena , Shai Brandes , Evgeny Schemeilin , Ron Beider , Amit Bernstein , Wajeeh Atrash , Gagandeep Singh , Apeksha Gupta , John Daley , Hyong Youb Kim , Gaetan Rivet , Jeroen de Borst , Rushil Gupta , Joshua Washington , Ziyang Xuan , Xiaoyun Wang , Guoyang Zhou , Jie Hai , Yisen Zhuang , Jingjing Wu , Andrew Boyer , Rosen Xu , Long Li , Jakub Grajciar , Matan Azrad , Viacheslav Ovsiienko , Dariusz Sosnowski , Ori Kam , Suanming Mou , Zyta Szpak , Liron Himi , Martin Spinler , Chaoyong He , Jiawen Wu , Tetsuya Mukawa , Vamsi Attunuru , Devendra Singh Rawat , Alok Prasad , Bruce Richardson , Andrew Rybchenko , Cristian Dumitrescu , Stephen Hemminger , Jerin Jacob , Maciej Czekaj , Jian Wang , Maxime Coquelin , Chenbo Xia , Jochen Behrens 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> From: Jie Hai In-Reply-To: <541e9d04-bed3-47b0-8b11-746f9047e1a0@amd.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.121.175] X-ClientProxiedBy: dggems703-chm.china.huawei.com (10.3.19.180) To kwepemf500004.china.huawei.com (7.202.181.242) 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 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. > Hi, all maintainers, We need your opinions on whether pkt_len and data_len should contain CRC len. The KEEP CRC feature is related. As if it is enabled, most drivers will substract CRC len from pkt_len and data_len. which means users cannot read the CRC data through the DPDK framework interface. Among the drivers that support keeping CRC, only the bnxt, cfpl, idpf, qede and sfc get the pkt_len and data_len from the descriptor and not subtract CRC len by drivers. I don't know if the length of these drivers includes the CRC len or not, please confirm that, thanks. Back to the current patch. Hi, Ferruh. Obviously, if we need to give users access to the CRC data, we'll have to modify the defination in ethdev and usage in most drivers. I don't think this change will be backported. Am I wrong? But this patch for hns3 bugfix, need to be backported. That's why we can separate this patch from the confirmation of the meaning of pkt_len and data_len. So can this patch merge first? Thanks, Jie Hai > >>> >>>> 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. > > .