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 7CEFA44142; Mon, 3 Jun 2024 09:07:10 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 08A064029C; Mon, 3 Jun 2024 09:07:10 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 463654026C for ; Mon, 3 Jun 2024 09:07:08 +0200 (CEST) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru DDF7938 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1717398427; bh=B4XsHFyiXKtqTB7ltje71mhl04K8DVDXvcFyj2eFxCg=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=ag8s1ilwNVJsp8lqnRtjmazNk4b83O2EypbmZTI8UcRPVYd94rY8uhHC3cnU/KmUA gSOCOwuaB+IUWR/tCh0xnPUsdX7vXcph4LAt+bt2jry0DTDI+4vHhOGOY6DeDeyvqs zEz6BbWKU6TLJGv0/iYo8V3slboOnFj02RqB4BLY= Received: from [192.168.1.39] (unknown [188.170.73.49]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id DDF7938; Mon, 3 Jun 2024 10:07:02 +0300 (MSK) Message-ID: <4e15c607-9aef-4c33-91ff-4d6784ea6b4d@oktetlabs.ru> Date: Mon, 3 Jun 2024 10:07:01 +0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] net/hns3: fix Rx packet truncation when KEEP CRC enabled To: Stephen Hemminger , Jie Hai Cc: Ferruh Yigit , huangdengdui , dev@dpdk.org, "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 , 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 , Cristian Dumitrescu , Jerin Jacob , Maciej Czekaj , Jian Wang , Maxime Coquelin , Chenbo Xia , Jochen Behrens , lihuisong@huawei.com, fengchengwen@huawei.com, liuyonglong@huawei.com 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> <20240602193304.75716520@hermes.local> Content-Language: en-US From: Andrew Rybchenko In-Reply-To: <20240602193304.75716520@hermes.local> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 6/3/24 05:33, Stephen Hemminger wrote: > On Mon, 3 Jun 2024 09:38:19 +0800 > 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. >>> >> 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. > > IMHO the only sane thing is: > -if keep crc is enabled then pkt_len and data_len include the extra bytes for the CRC. > -if keep crc is disabled, then pkt_len and data_len match the length of the packet without the CRC. > > > Other than driver testing, never saw much point to using keep crc. +1