* CRC offload from application's POV @ 2022-10-11 10:48 Viacheslav Galaktionov 2022-10-11 11:36 ` Ferruh Yigit 0 siblings, 1 reply; 10+ messages in thread From: Viacheslav Galaktionov @ 2022-10-11 10:48 UTC (permalink / raw) To: dev; +Cc: Denis Pryazhennikov 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. Thank you in advance, Viacheslav ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CRC offload from application's POV 2022-10-11 10:48 CRC offload from application's POV Viacheslav Galaktionov @ 2022-10-11 11:36 ` Ferruh Yigit 2022-10-11 11:54 ` Viacheslav Galaktionov 0 siblings, 1 reply; 10+ messages in thread From: Ferruh Yigit @ 2022-10-11 11:36 UTC (permalink / raw) To: Viacheslav Galaktionov; +Cc: Denis Pryazhennikov, dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CRC offload from application's POV 2022-10-11 11:36 ` Ferruh Yigit @ 2022-10-11 11:54 ` Viacheslav Galaktionov 2022-10-11 13:46 ` Ferruh Yigit 0 siblings, 1 reply; 10+ messages in thread From: Viacheslav Galaktionov @ 2022-10-11 11:54 UTC (permalink / raw) To: Ferruh Yigit; +Cc: Denis Pryazhennikov, dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CRC offload from application's POV 2022-10-11 11:54 ` Viacheslav Galaktionov @ 2022-10-11 13:46 ` Ferruh Yigit 2022-10-12 2:29 ` Wu, Wenjun1 0 siblings, 1 reply; 10+ messages in thread From: Ferruh Yigit @ 2022-10-11 13:46 UTC (permalink / raw) To: Viacheslav Galaktionov, Simei Su, Wenjun Wu; +Cc: Denis Pryazhennikov, dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: CRC offload from application's POV 2022-10-11 13:46 ` Ferruh Yigit @ 2022-10-12 2:29 ` Wu, Wenjun1 2022-10-12 8:06 ` Ferruh Yigit 0 siblings, 1 reply; 10+ messages in thread From: Wu, Wenjun1 @ 2022-10-12 2:29 UTC (permalink / raw) To: Ferruh Yigit, Viacheslav Galaktionov, Su, Simei; +Cc: Denis Pryazhennikov, dev > -----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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CRC offload from application's POV 2022-10-12 2:29 ` Wu, Wenjun1 @ 2022-10-12 8:06 ` Ferruh Yigit 2022-10-12 8:18 ` Wu, Wenjun1 0 siblings, 1 reply; 10+ messages in thread From: Ferruh Yigit @ 2022-10-12 8:06 UTC (permalink / raw) To: Wu, Wenjun1, Viacheslav Galaktionov, Su, Simei; +Cc: Denis Pryazhennikov, dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: CRC offload from application's POV 2022-10-12 8:06 ` Ferruh Yigit @ 2022-10-12 8:18 ` Wu, Wenjun1 2022-10-12 8:21 ` Ferruh Yigit 0 siblings, 1 reply; 10+ messages in thread From: Wu, Wenjun1 @ 2022-10-12 8:18 UTC (permalink / raw) To: Ferruh Yigit, Viacheslav Galaktionov, Su, Simei; +Cc: Denis Pryazhennikov, dev > -----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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CRC offload from application's POV 2022-10-12 8:18 ` Wu, Wenjun1 @ 2022-10-12 8:21 ` Ferruh Yigit 2022-10-12 8:49 ` Viacheslav Galaktionov 2022-10-12 9:10 ` Wu, Wenjun1 0 siblings, 2 replies; 10+ messages in thread From: Ferruh Yigit @ 2022-10-12 8:21 UTC (permalink / raw) To: Wu, Wenjun1, Viacheslav Galaktionov, Su, Simei; +Cc: Denis Pryazhennikov, dev 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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CRC offload from application's POV 2022-10-12 8:21 ` Ferruh Yigit @ 2022-10-12 8:49 ` Viacheslav Galaktionov 2022-10-12 9:10 ` Wu, Wenjun1 1 sibling, 0 replies; 10+ messages in thread From: Viacheslav Galaktionov @ 2022-10-12 8:49 UTC (permalink / raw) To: Ferruh Yigit, Wu, Wenjun1, Su, Simei; +Cc: Denis Pryazhennikov, dev 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@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. Yes, this sounds the most reasonable. Thanks for the help, Ferruh and Wenjun! ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: CRC offload from application's POV 2022-10-12 8:21 ` Ferruh Yigit 2022-10-12 8:49 ` Viacheslav Galaktionov @ 2022-10-12 9:10 ` Wu, Wenjun1 1 sibling, 0 replies; 10+ messages in thread From: Wu, Wenjun1 @ 2022-10-12 9:10 UTC (permalink / raw) To: Ferruh Yigit, Viacheslav Galaktionov, Su, Simei; +Cc: Denis Pryazhennikov, dev > -----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. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-10-12 9:10 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-11 10:48 CRC offload from application's POV Viacheslav Galaktionov 2022-10-11 11:36 ` Ferruh Yigit 2022-10-11 11:54 ` Viacheslav Galaktionov 2022-10-11 13:46 ` Ferruh Yigit 2022-10-12 2:29 ` Wu, Wenjun1 2022-10-12 8:06 ` Ferruh Yigit 2022-10-12 8:18 ` Wu, Wenjun1 2022-10-12 8:21 ` Ferruh Yigit 2022-10-12 8:49 ` Viacheslav Galaktionov 2022-10-12 9:10 ` Wu, Wenjun1
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).