Re: CRC offload from application's POV

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 10/12/2022 3:29 AM, Wu, Wenjun1 wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Tuesday, October 11, 2022 9:46 PM
>> To: Viacheslav Galaktionov <Viacheslav.Galaktionov@arknetworks.am>; Su,
>> Simei <simei.su@intel.com>; Wu, Wenjun1 <wenjun1.wu@intel.com>
>> Cc: Denis Pryazhennikov <Denis.Pryazhennikov@arknetworks.am>;
>> 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.