CRC offload from application's POV

[Viacheslav Galaktionov]

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