CRC offload from application's POV

[Ferruh Yigit]

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.