RE: CRC offload from application's POV

["Wu, Wenjun1" <wenjun1.wu@intel.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Wednesday, October 12, 2022 4:21 PM
> To: Wu, Wenjun1 <wenjun1.wu@intel.com>; Viacheslav Galaktionov
> <Viacheslav.Galaktionov@arknetworks.am>; Su, Simei <simei.su@intel.com>
> Cc: Denis Pryazhennikov <Denis.Pryazhennikov@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.yigit@amd.com>
> >> Sent: Wednesday, October 12, 2022 4:07 PM
> >> To: Wu, Wenjun1 <wenjun1.wu@intel.com>; Viacheslav Galaktionov
> >> <Viacheslav.Galaktionov@arknetworks.am>; Su, Simei
> >> <simei.su@intel.com>
> >> Cc: Denis Pryazhennikov <Denis.Pryazhennikov@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.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.
> > 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.