> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@amd.com> > Sent: Tuesday, October 11, 2022 9:46 PM > To: Viacheslav Galaktionov <viacheslav.galaktio...@arknetworks.am>; Su, > Simei <simei...@intel.com>; Wu, Wenjun1 <wenjun1...@intel.com> > Cc: Denis Pryazhennikov <denis.pryazhenni...@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.
