[RFC] ethdev: add new offload flag to keep CRC

  DEV_RX_OFFLOAD_KEEP_CRC offload flag added.

DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
default behavior in PMDs is to keep the CRC until this flag removed

Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
- Setting both KEEP_CRC & CRC_STRIP is INVALID
- Setting only CRC_STRIP PMD should strip the CRC
- Setting only KEEP_CRC PMD should keep the CRC
- Not setting both PMD should keep the CRC

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 10 ----------
 lib/librte_ethdev/rte_ethdev.h       |  4 ++++
 2 files changed, 4 insertions(+), 10 deletions(-)
  

On 06/09/2018 01:57 AM, Ferruh Yigit wrote:
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
> default behavior in PMDs is to keep the CRC until this flag removed
>
> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> - Setting both KEEP_CRC & CRC_STRIP is INVALID

ethdev layer should enforce it.

> - Setting only CRC_STRIP PMD should strip the CRC
> - Setting only KEEP_CRC PMD should keep the CRC
> - Not setting both PMD should keep the CRC
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

- rte_rx_offload_names should be updated as well
- testpmd should be updated

I'm not sure that I understand the transition plan. I think PMDs which 
support
KEEP_CRC should be updated by the patch to advertise the offload. Otherwise,
generic checks in ethdev will not allow to enable it. Other option is to 
simply
drop it on ethdev layer (since basically it changes nothing for PMD now) and
encourage PMD maintainers to advertise and take it into account if the 
feature
is supported (however, if the bit is dropped on ethdev layer, the code 
would
be unused/dead regardless application request). Too complicated. I guess
there are not so many PMDs which support KEEP_CRC. Better to update them.

The patch should encourage applications which would like to keep CRC to
use the offload since the next release will drop CRC_STRIP and it will
change behaviour (no KEEP_CRC => strip it)
  

On 6/9/2018 11:11 AM, Andrew Rybchenko wrote:
> On 06/09/2018 01:57 AM, Ferruh Yigit wrote:
>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>>
>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>> default behavior in PMDs is to keep the CRC until this flag removed
>>
>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>> - Setting both KEEP_CRC & CRC_STRIP is INVALID
> 
> ethdev layer should enforce it.

OK, will add this check.

> 
>> - Setting only CRC_STRIP PMD should strip the CRC
>> - Setting only KEEP_CRC PMD should keep the CRC
>> - Not setting both PMD should keep the CRC
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> - rte_rx_offload_names should be updated as well
> - testpmd should be updated
> 
> I'm not sure that I understand the transition plan. I think PMDs which support
> KEEP_CRC should be updated by the patch to advertise the offload. Otherwise,
> generic checks in ethdev will not allow to enable it. 

Right, PMDs should be updated to advertise this offload, will do it.

> Other option is to simply
> drop it on ethdev layer (since basically it changes nothing for PMD now) and
> encourage PMD maintainers to advertise and take it into account if the feature
> is supported (however, if the bit is dropped on ethdev layer, the code would
> be unused/dead regardless application request). Too complicated. I guess
> there are not so many PMDs which support KEEP_CRC. Better to update them

> The patch should encourage applications which would like to keep CRC to
> use the offload since the next release will drop CRC_STRIP and it will
> change behaviour (no KEEP_CRC => strip it)

+1, will check apps.
  

On Fri,  8 Jun 2018 23:57:09 +0100
Ferruh Yigit <ferruh.yigit@intel.com> wrote:

> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> 
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
> default behavior in PMDs is to keep the CRC until this flag removed

This won't work for virtual devices that never keep CRC.
Maybe wording should be changed.
  

On 6/11/2018 4:25 PM, Stephen Hemminger wrote:
> On Fri,  8 Jun 2018 23:57:09 +0100
> Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>>
>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release
>> default behavior in PMDs is to keep the CRC until this flag removed
> 
> This won't work for virtual devices that never keep CRC.
> Maybe wording should be changed.

Yes, it won't work for virtual devices, but it is already the case, no offload
flag means keep CRC.

This task is to replace that behavior gradually. What will be replaced is:
flag: DEV_RX_OFFLOAD_CRC_STRIP => DEV_RX_OFFLOAD_KEEP_CRC
default, no flag, behavior : KEEP_CRC => STRIP_CRC

But because of gradually switch, this release, 18.08, both flags will exits and
no flag default still will be KEEP_CRC

For virtual devices, for this release, since virtual PMDs don't announce any CRC
related offload capability, applications can't explicitly request to keep the CRC.
But no flag case (default keep CRC) will be wrong and ignored by virtual PMDs.

on 18.11 when DEV_RX_OFFLOAD_CRC_STRIP is removed and default behavior is STRIP
CRC, it will be correct for virtual device.