> -----Original Message----- > From: Ferruh Yigit <ferruh.yi...@amd.com> > Sent: Wednesday, October 12, 2022 4:21 PM > To: Wu, Wenjun1 <wenjun1...@intel.com>; Viacheslav Galaktionov > <viacheslav.galaktio...@arknetworks.am>; Su, Simei <simei...@intel.com> > Cc: Denis Pryazhennikov <denis.pryazhenni...@arknetworks.am>; > dev@dpdk.org > Subject: Re: CRC offload from application's POV > > On 10/12/2022 9:18 AM, Wu, Wenjun1 wrote: > > > > > >> -----Original Message----- > >> From: Ferruh Yigit <ferruh.yi...@amd.com> > >> Sent: Wednesday, October 12, 2022 4:07 PM > >> To: Wu, Wenjun1 <wenjun1...@intel.com>; Viacheslav Galaktionov > >> <viacheslav.galaktio...@arknetworks.am>; Su, Simei > >> <simei...@intel.com> > >> Cc: Denis Pryazhennikov <denis.pryazhenni...@arknetworks.am>; > >> 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 <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. > >>> > >> > >> 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.
Hi Ferruh, I have double check the PMD with Simei, we think you are right.
