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 657BDA0547; Wed, 12 Oct 2022 10:50:46 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1D76842D86; Wed, 12 Oct 2022 10:50:46 +0200 (CEST) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id E933042D6E for ; Wed, 12 Oct 2022 10:50:44 +0200 (CEST) Received: from [192.168.10.54] (unknown [37.252.90.175]) (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 agw.arknetworks.am (Postfix) with ESMTPSA id 3C9F7E002A; Wed, 12 Oct 2022 12:50:44 +0400 (+04) Message-ID: Date: Wed, 12 Oct 2022 12:49:44 +0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.2 Subject: Re: CRC offload from application's POV To: Ferruh Yigit , "Wu, Wenjun1" , "Su, Simei" Cc: Denis Pryazhennikov , "dev@dpdk.org" References: <11b33bf3-413a-6955-423a-cc47a73e2202@arknetworks.am> <103627bd-d704-84a2-9f3e-5e4a7341e6a7@amd.com> <2f221574-ac04-2931-275b-e758669e0bef@arknetworks.am> <6e1eb50c-8633-a4b1-e18a-84b525074e32@amd.com> <5e9224e3-7d38-9699-fc69-83d4dd9ab3ae@amd.com> <71cf8172-1000-9e6b-e7f3-9e4850b6615c@amd.com> Content-Language: en-US From: Viacheslav Galaktionov In-Reply-To: <71cf8172-1000-9e6b-e7f3-9e4850b6615c@amd.com> 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 10/12/22 12:21, Ferruh Yigit wrote: > On 10/12/2022 9:18 AM, Wu, Wenjun1 wrote: >> >> >>> -----Original Message----- >>> From: Ferruh Yigit >>> Sent: Wednesday, October 12, 2022 4:07 PM >>> To: Wu, Wenjun1 ; Viacheslav Galaktionov >>> ; Su, Simei >>> Cc: Denis Pryazhennikov ; >>> dev@dpdk.org >>> Subject: Re: CRC offload from application's POV >>> >>> On 10/12/2022 3:29 AM, Wu, Wenjun1 wrote: >>>> >>>> >>>>> -----Original Message----- >>>>> From: Ferruh Yigit >>>>> Sent: Tuesday, October 11, 2022 9:46 PM >>>>> To: Viacheslav Galaktionov ; >>>>> Su, Simei ; Wu, Wenjun1 >>>>> Cc: Denis Pryazhennikov ; >>>>> dev@dpdk.org >>>>> Subject: Re: CRC offload from application's POV >>>>> >>>>> On 10/11/2022 12:54 PM, Viacheslav Galaktionov wrote: >>>>>> On 10/11/22 15:36, Ferruh Yigit wrote: >>>>>>> On 10/11/2022 11:48 AM, Viacheslav Galaktionov wrote: >>>>>>>> Hi! >>>>>>>> >>>>>>>> We're looking to implement CRC offload in our driver and we're >>>>>>>> having difficulties understanding what the feature changes from >>>>>>>> the application's point of view. If we enable the KEEP_CRC >>>>>>>> offload, then the NIC is supposed to preserve the CRC in the >>>>>>>> packet, that much is clear. But we checked other drivers and it >>>>>>>> seems common for PMDs to remove the CRC from the final mbufs. >>>>>>>> Why is that? >>>>>>>> >>>>>>>> We couldn't find any place where the CRC would be stored after >>>>>>>> removal, so it looks like the application doesn't have access to >>>>>>>> this piece of data. And if so, what's the point of having this >>>>>>>> feature if the CRC is discarded either way? >>>>>>>> >>>>>>>> We're probably missing something and would really appreciate any >>>>>>>> help with this. >>>>>>>> >>>>>>> >>>>>>> Hi Viacheslav, >>>>>>> >>>>>>> As you said default behavior is to strip the CRC from packet, even >>>>>>> some devices doesn't support having CRC in the packet it is removed >>>>>>> by HW automatically. In this case application can't access to >>>>>>> the CRC. >>>>>>> >>>>>>> For the devices that has capability to keep CRC, KEEP_CRC offload >>>>>>> should enable having CRC as part of the packet. There is no special >>>>>>> field to store the CRC. >>>>>>> >>>>>> I'm asking because I'm seeing a common pattern in the code base: if >>>>>> the hardware didn't remove the CRC, the driver does this itself. >>>>>> Grepping the code for "crc_len" will show you what I mean. One of >>>>>> the most apparent examples of this happening can be seen in >>>>>> drivers/net/e1000/em_rxtx.c: >>>>>> >>>>>> /* >>>>>>     * This is the last buffer of the received packet. >>>>>>     * If the CRC is not stripped by the hardware: >>>>>>     *   - Subtract the CRC    length from the total packet length. >>>>>>     *   - If the last buffer only contains the whole CRC or a part >>>>>>     *     of it, free the mbuf associated to the last buffer. >>>>>>     *     If part of the CRC is also contained in the previous >>>>>>     *     mbuf, subtract the length of that CRC part from the >>>>>>     *     data length of the previous mbuf. >>>>>>     */ >>>>>> >>>>>> I don't understand why this is necessary, and whether this is just a >>>>>> particularity of this driver or how the feature is supposed to be >>>>>> implemented everywhere. I haven't checked every driver, but it seems >>>>>> like a lot of them do something similar to this. >>>>> >>>>> That looks wrong to me too, cc'ed maintainers for comment. >>>>> >>>>> That piece of code seems remaining from first upstream of the driver >>>>> (2012), it is before KEEP_CRC change, looks like it is missed. >>>>> >>>>> CRC should be kept in the packet if driver supports it and user >>>>> requested KEEP_CRC offload. >>>>> >>>>> But Rx stats should not include CRC, as it is common to use >>>>> 'm->pkt_len' >>>>> for received packet stat, when CRC is in packet that should taken >>>>> into account for stats. >>>> >>>> I agree with Ferruh. PMD will advertise KEEP_CRC offload in >>>> rte_eth_dev_info->rx_offload_capa. If it is supported, CRC will always >>>> keep in the packets. If user request KEEP_CRC offload in >>>> rte_eth_rxmode, the driver will subtract CRC length from data length >>>> to remove CRC from packet data, and user can get the CRC after the >>>> end of >>> the packet. >>>> >>> >>> Hi Wenjun, >>> >>> What we said is slightly different, it is OK to subtract the length >>> from *stats*, >>> but I think driver shouldn't remove the CRC from packet data. >> You are right, the driver will never remove the CRC from packet data. > > Just to be sure we are on same page, driver won't remove the CRC only > when KEEP_CRC is requested by user. Otherwise it will remove the CRC > by default. Yes, this sounds the most reasonable. Thanks for the help, Ferruh and Wenjun!