[dpdk-dev] [PATCH] doc: announce ethdev CRC strip flag deprecation

[dpdk-dev] [PATCH] doc: announce ethdev CRC strip flag deprecation

[Ferruh Yigit]

Make CRC stripping default behavior, deprecate flag
DEV_RX_OFFLOAD_CRC_STRIP.

Introduce a new flag to let applications request keep CRC.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b40c57f28..6e1266b20 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -147,6 +147,12 @@ Deprecation Notices
   successful. This modification will only impact the PMDs, not the
   applications.
 
+* ethdev: Make CRC stript default behavior without any flag required and add a
+  new offload flag to let application request for keeping CRC if PMD reports
+  capability for it.
+  ``DEV_RX_OFFLOAD_CRC_STRIP`` flag will be removed.
+  ``DEV_RX_OFFLOAD_KEETP_CRC`` will be added.
+
 * i40e: The default flexible payload configuration which extracts the first 16
   bytes of the payload for RSS will be deprecated starting from 18.02. If
   required the previous behavior can be configured using existing flow
-- 
2.13.6

Re: [dpdk-dev] [PATCH] doc: announce ethdev CRC strip flag deprecation

[Thomas Monjalon]

20/03/2018 12:26, Ferruh Yigit:
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> +* ethdev: Make CRC stript default behavior without any flag required and add a

s/stript/stripping/

> +  new offload flag to let application request for keeping CRC if PMD reports
> +  capability for it.
> +  ``DEV_RX_OFFLOAD_CRC_STRIP`` flag will be removed.
> +  ``DEV_RX_OFFLOAD_KEETP_CRC`` will be added.

s/KEETP/KEEP/

I think we should introduce the new flag without removing the old one
for one release.
Setting both flags would be an error.
Setting no flag would mean stripping.
So the CRC_STRIP flag would be just ignored by PMDs.

Opinions?

Re: [dpdk-dev] [PATCH] doc: announce ethdev CRC strip flag deprecation

[Ferruh Yigit]

On 3/20/2018 11:35 AM, Thomas Monjalon wrote:
> 20/03/2018 12:26, Ferruh Yigit:
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> +* ethdev: Make CRC stript default behavior without any flag required and add a
> 
> s/stript/stripping/
> 
>> +  new offload flag to let application request for keeping CRC if PMD reports
>> +  capability for it.
>> +  ``DEV_RX_OFFLOAD_CRC_STRIP`` flag will be removed.
>> +  ``DEV_RX_OFFLOAD_KEETP_CRC`` will be added.
> 
> s/KEETP/KEEP/
> 
> I think we should introduce the new flag without removing the old one
> for one release.
> Setting both flags would be an error.
> Setting no flag would mean stripping.
> So the CRC_STRIP flag would be just ignored by PMDs.
> 
> Opinions?

Introducing KEEP_CRC in this release is OK, since no PMD announces this
capability, it should be safe.

Since many PMD's offloading patches are not applied yet, deciding on "Setting no
flag would mean stripping" helps to get this correct at first place for them.
Only this is ABI break, but I am not aware of any PMD that implements not
stripping CRC case so I hope this is acceptable.

If there is no objection, I will update notice to only remove flag in next release.

[dpdk-dev] [PATCH v2 1/2] doc: announce ethdev CRC strip flag deprecation

[Ferruh Yigit]

Make CRC stripping default behavior, deprecate flag
DEV_RX_OFFLOAD_CRC_STRIP.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

---

v2:
* Make CRC behavior active in this release
---
 doc/guides/rel_notes/deprecation.rst | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index b40c57f28..ed97c98dd 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -147,6 +147,10 @@ Deprecation Notices
   successful. This modification will only impact the PMDs, not the
   applications.
 
+* ethdev: Make CRC stripping default behavior in v18.05 without any flag
+  and remove flag completely in v18.08
+  ``DEV_RX_OFFLOAD_CRC_STRIP`` flag will be removed.
+
 * i40e: The default flexible payload configuration which extracts the first 16
   bytes of the payload for RSS will be deprecated starting from 18.02. If
   required the previous behavior can be configured using existing flow
-- 
2.13.6

[dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Ferruh Yigit]

DEV_RX_OFFLOAD_KEEP_CRC offload flag added.

DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release but default
behavior in PMDs is to strip the CRC independent from this flag.

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 strip the CRC

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 lib/librte_ether/rte_ethdev.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 5e13dca6a..ab1030d42 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -939,6 +939,9 @@ struct rte_eth_conf {
 #define DEV_RX_OFFLOAD_SCATTER		0x00002000
 #define DEV_RX_OFFLOAD_TIMESTAMP	0x00004000
 #define DEV_RX_OFFLOAD_SECURITY         0x00008000
+/* Invalid to set both DEV_RX_OFFLOAD_CRC_STRIP and DEV_RX_OFFLOAD_KEEP_CRC
+ * No DEV_RX_OFFLOAD_CRC_STRIP flag means stripping CRC */
+#define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000
 #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
 				 DEV_RX_OFFLOAD_UDP_CKSUM | \
 				 DEV_RX_OFFLOAD_TCP_CKSUM)
-- 
2.13.6

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Thomas Monjalon]

21/03/2018 20:47, Ferruh Yigit:
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> 
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release but default
> behavior in PMDs is to strip the CRC independent from this flag.
> 
> 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 strip the CRC

It is the same as saying that DEV_RX_OFFLOAD_CRC_STRIP has no effect :)

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Shahaf Shuler]

Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> 
> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release but default
> behavior in PMDs is to strip the CRC independent from this flag.
> 
> 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 strip the CRC

We cannot have such default behavior, it may break existing applications.

The API of ethdev offloads says (even though it is not well documented) :  DEV_RX_OFFLOAD_CRC_STRIP (emphasis the STRIP). 
meaning, if set -> STRIP, if not set -> don't strip.  I am aware to at least one application which wants to have the CRC, and for that purpose it naturally don't set the offload flag. 

The fact some PMDs lack the ability to toggle the CRC stripping should be exposed in the "limitations" section of their related guide. 

Up to here, this is the behavior as defined by the API. 

Now, we want to change it, and I think it makes sense. However I think we should take similar approach to what we did with the ethdev offloads API:
1. at first stage a new offload flag is exposed DEV_RX_OFFLOAD_KEEP_CRC and implemented on the PMDs. 
2. there is a conversion function on ethdev. Which for example converts ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the PMDs.
3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and remove of the conversion functions. 

More below,

> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 5e13dca6a..ab1030d42 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -939,6 +939,9 @@ struct rte_eth_conf {
>  #define DEV_RX_OFFLOAD_SCATTER		0x00002000
>  #define DEV_RX_OFFLOAD_TIMESTAMP	0x00004000
>  #define DEV_RX_OFFLOAD_SECURITY         0x00008000
> +/* Invalid to set both DEV_RX_OFFLOAD_CRC_STRIP and
> +DEV_RX_OFFLOAD_KEEP_CRC
> + * No DEV_RX_OFFLOAD_CRC_STRIP flag means stripping CRC */
> +#define DEV_RX_OFFLOAD_KEEP_CRC		0x00010000

Need also comment on DEV_RX_OFFLOAD_CRC_STRIP to say it is deprecated.

>  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM |
> \
>  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
>  				 DEV_RX_OFFLOAD_TCP_CKSUM)
> --
> 2.13.6

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Thomas Monjalon]

29/03/2018 07:38, Shahaf Shuler:
> Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
> > DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> > 
> > DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release but default
> > behavior in PMDs is to strip the CRC independent from this flag.
> > 
> > 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 strip the CRC
> 
> We cannot have such default behavior, it may break existing applications.
> 
> The API of ethdev offloads says (even though it is not well documented) :  DEV_RX_OFFLOAD_CRC_STRIP (emphasis the STRIP). 
> meaning, if set -> STRIP, if not set -> don't strip.  I am aware to at least one application which wants to have the CRC, and for that purpose it naturally don't set the offload flag. 
> 
> The fact some PMDs lack the ability to toggle the CRC stripping should be exposed in the "limitations" section of their related guide. 
> 
> Up to here, this is the behavior as defined by the API. 
> 
> Now, we want to change it, and I think it makes sense. However I think we should take similar approach to what we did with the ethdev offloads API:
> 1. at first stage a new offload flag is exposed DEV_RX_OFFLOAD_KEEP_CRC and implemented on the PMDs.

This is what is proposed above, except that we change the default behaviour.
If we introduce a new flag which is the contrary of the old one, we need
to choose which one is the default, so which one has no effect.

> 2. there is a conversion function on ethdev. Which for example converts ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the PMDs.
> 3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and remove of the conversion functions. 

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Shahaf Shuler]

Thursday, March 29, 2018 10:43 AM, Thomas Monjalon:
> 29/03/2018 07:38, Shahaf Shuler:
> > Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
> > > DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> > >
> > > DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release but
> > > default behavior in PMDs is to strip the CRC independent from this flag.
> > >
> > > 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 strip the CRC
> >
> > We cannot have such default behavior, it may break existing applications.
> >
> > The API of ethdev offloads says (even though it is not well documented) :
> DEV_RX_OFFLOAD_CRC_STRIP (emphasis the STRIP).
> > meaning, if set -> STRIP, if not set -> don't strip.  I am aware to at least one
> application which wants to have the CRC, and for that purpose it naturally
> don't set the offload flag.
> >
> > The fact some PMDs lack the ability to toggle the CRC stripping should be
> exposed in the "limitations" section of their related guide.
> >
> > Up to here, this is the behavior as defined by the API.
> >
> > Now, we want to change it, and I think it makes sense. However I think we
> should take similar approach to what we did with the ethdev offloads API:
> > 1. at first stage a new offload flag is exposed DEV_RX_OFFLOAD_KEEP_CRC
> and implemented on the PMDs.
> 
> This is what is proposed above, except that we change the default
> behaviour.
> If we introduce a new flag which is the contrary of the old one, we need to
> choose which one is the default, so which one has no effect.

The default behavior should remain as long as we don't deprecate the DEV_RX_OFFLOAD_CRC_STRIP flag.

Logic should be :
Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
 - Setting both KEEP_CRC & CRC_STRIP is INVALID - enforced by ethdev. 
 - Setting only CRC_STRIP PMD should strip the CRC
 - Setting only KEEP_CRC PMD should keep the CRC
 - Not setting both PMD should *not* strip the CRC 

> 
> > 2. there is a conversion function on ethdev. Which for example converts
> ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the
> PMDs.
> > 3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and
> remove of the conversion functions.
> 
> 
> 

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Ferruh Yigit]

On 3/29/2018 8:56 AM, Shahaf Shuler wrote:
> Thursday, March 29, 2018 10:43 AM, Thomas Monjalon:
>> 29/03/2018 07:38, Shahaf Shuler:
>>> Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
>>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>>>>
>>>> DEV_RX_OFFLOAD_CRC_STRIP flag will remain one more release but
>>>> default behavior in PMDs is to strip the CRC independent from this flag.
>>>>
>>>> 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 strip the CRC
>>>
>>> We cannot have such default behavior, it may break existing applications.
>>>
>>> The API of ethdev offloads says (even though it is not well documented) :
>> DEV_RX_OFFLOAD_CRC_STRIP (emphasis the STRIP).
>>> meaning, if set -> STRIP, if not set -> don't strip.  I am aware to at least one
>> application which wants to have the CRC, and for that purpose it naturally
>> don't set the offload flag.
>>>
>>> The fact some PMDs lack the ability to toggle the CRC stripping should be
>> exposed in the "limitations" section of their related guide.
>>>
>>> Up to here, this is the behavior as defined by the API.
>>>
>>> Now, we want to change it, and I think it makes sense. However I think we
>> should take similar approach to what we did with the ethdev offloads API:
>>> 1. at first stage a new offload flag is exposed DEV_RX_OFFLOAD_KEEP_CRC
>> and implemented on the PMDs.
>>
>> This is what is proposed above, except that we change the default
>> behaviour.
>> If we introduce a new flag which is the contrary of the old one, we need to
>> choose which one is the default, so which one has no effect.
> 
> The default behavior should remain as long as we don't deprecate the DEV_RX_OFFLOAD_CRC_STRIP flag.
> 
> Logic should be :
> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>  - Setting both KEEP_CRC & CRC_STRIP is INVALID - enforced by ethdev. 
>  - Setting only CRC_STRIP PMD should strip the CRC
>  - Setting only KEEP_CRC PMD should keep the CRC
>  - Not setting both PMD should *not* strip the CRC 

Hi Shahaf,

I think we have two options,

1- This is v1 of this patch and also what you suggest:
v18.05:
- Send deprecation notice

v18.08:
- Add KEEP_CRC flag
- Change the meaning not setting CRC_STRIP to "strip the CRC"

v18.11:
- Remove the CRC_STRIP flag completely

I think this is more proper but takes more time.


2- Based on all the reality that all devices are doing CRC strip already:

v18.05:
- Add KEEP_CRC flag
- Change the meaning not setting CRC_STRIP to "strip the CRC"

v18.08:
- Remove the CRC_STRIP flag completely


With option two since not setting both KEEP_CRC & CRC_STRIP will mean "strip the
CRC", it won't require a change in PMDs, only we can pay attention to get new
PMDs according plan.

This can be more problematic for the case that application ask for keeping CRC,
because the way to say this was not setting CRC_STRIP, it changed to setting
KEEP_CRC without notification. This can be less problem since many PMD already
doesn't support keep crc.

Are you aware of any use case that is broken with option 2?


> 
>>
>>> 2. there is a conversion function on ethdev. Which for example converts
>> ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the
>> PMDs.
>>> 3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and
>> remove of the conversion functions.
>>
>>
>>
> 

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Shahaf Shuler]

Thursday, March 29, 2018 4:32 PM, Ferruh Yigit:
> On 3/29/2018 8:56 AM, Shahaf Shuler wrote:
> > Thursday, March 29, 2018 10:43 AM, Thomas Monjalon:
> >> 29/03/2018 07:38, Shahaf Shuler:
> >>> Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
> >>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> >
> > Logic should be :
> > Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> >  - Setting both KEEP_CRC & CRC_STRIP is INVALID - enforced by ethdev.
> >  - Setting only CRC_STRIP PMD should strip the CRC
> >  - Setting only KEEP_CRC PMD should keep the CRC
> >  - Not setting both PMD should *not* strip the CRC
> 
> Hi Shahaf,
> 
> I think we have two options,
> 
> 1- This is v1 of this patch and also what you suggest:
> v18.05:
> - Send deprecation notice
> 
> v18.08:
> - Add KEEP_CRC flag
> - Change the meaning not setting CRC_STRIP to "strip the CRC"

I think the above line ...

> 
> v18.11:
> - Remove the CRC_STRIP flag completely

Should be here.

It is better to change the default behavior only once the STRIP_CRC flags is removed. 
Without it on v18.08 you break application that wants to have the CRC, and on v18.11 you break the application which actually used it.
It is better to have all the application changes in one release - 18.11. 

> 
> I think this is more proper but takes more time.

Me too. 

> 
> 
> 2- Based on all the reality that all devices are doing CRC strip already:
> 
> v18.05:
> - Add KEEP_CRC flag
> - Change the meaning not setting CRC_STRIP to "strip the CRC"
> 
> v18.08:
> - Remove the CRC_STRIP flag completely
> 
> 
> With option two since not setting both KEEP_CRC & CRC_STRIP will mean
> "strip the CRC", it won't require a change in PMDs, only we can pay attention
> to get new PMDs according plan.
> 
> This can be more problematic for the case that application ask for keeping
> CRC, because the way to say this was not setting CRC_STRIP, it changed to
> setting KEEP_CRC without notification. This can be less problem since many
> PMD already doesn't support keep crc.

but what about those which do support? 
You break application which uses PMDs which support this offload for the sake of the PMD which don't have this capability. 

I think #1 is the clean one. 

> 
> Are you aware of any use case that is broken with option 2?
> 
> 
> >
> >>
> >>> 2. there is a conversion function on ethdev. Which for example
> >>> converts
> >> ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the
> PMDs.
> >>> 3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and
> >> remove of the conversion functions.
> >>
> >>
> >>
> >

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Ferruh Yigit]

On 4/1/2018 8:10 AM, Shahaf Shuler wrote:
> Thursday, March 29, 2018 4:32 PM, Ferruh Yigit:
>> On 3/29/2018 8:56 AM, Shahaf Shuler wrote:
>>> Thursday, March 29, 2018 10:43 AM, Thomas Monjalon:
>>>> 29/03/2018 07:38, Shahaf Shuler:
>>>>> Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
>>>>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
>>>
>>> Logic should be :
>>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
>>>  - Setting both KEEP_CRC & CRC_STRIP is INVALID - enforced by ethdev.
>>>  - Setting only CRC_STRIP PMD should strip the CRC
>>>  - Setting only KEEP_CRC PMD should keep the CRC
>>>  - Not setting both PMD should *not* strip the CRC
>>
>> Hi Shahaf,
>>
>> I think we have two options,
>>
>> 1- This is v1 of this patch and also what you suggest:
>> v18.05:
>> - Send deprecation notice
>>
>> v18.08:
>> - Add KEEP_CRC flag
>> - Change the meaning not setting CRC_STRIP to "strip the CRC"
> 
> I think the above line ...
> 
>>
>> v18.11:
>> - Remove the CRC_STRIP flag completely
> 
> Should be here.
> 
> It is better to change the default behavior only once the STRIP_CRC flags is removed. 
> Without it on v18.08 you break application that wants to have the CRC, and on v18.11 you break the application which actually used it.
> It is better to have all the application changes in one release - 18.11. 
> 
>>
>> I think this is more proper but takes more time.
> 
> Me too. 
> 
>>
>>
>> 2- Based on all the reality that all devices are doing CRC strip already:
>>
>> v18.05:
>> - Add KEEP_CRC flag
>> - Change the meaning not setting CRC_STRIP to "strip the CRC"
>>
>> v18.08:
>> - Remove the CRC_STRIP flag completely
>>
>>
>> With option two since not setting both KEEP_CRC & CRC_STRIP will mean
>> "strip the CRC", it won't require a change in PMDs, only we can pay attention
>> to get new PMDs according plan.
>>
>> This can be more problematic for the case that application ask for keeping
>> CRC, because the way to say this was not setting CRC_STRIP, it changed to
>> setting KEEP_CRC without notification. This can be less problem since many
>> PMD already doesn't support keep crc.
> 
> but what about those which do support? 
> You break application which uses PMDs which support this offload for the sake of the PMD which don't have this capability. 
> 
> I think #1 is the clean one. 


Any decision here? So will we go with first version of this patch [1]?

[1]
https://dpdk.org/dev/patchwork/patch/36288/

> 
>>
>> Are you aware of any use case that is broken with option 2?
>>
>>
>>>
>>>>
>>>>> 2. there is a conversion function on ethdev. Which for example
>>>>> converts
>>>> ~DEV_RX_OFFLOAD_CRC_STRIP -> DEV_RX_OFFLOAD_KEEP_CRC for the
>> PMDs.
>>>>> 3. deprecation of DEV_RX_OFFLOAD_CRC_STRIP for applications and
>>>> remove of the conversion functions.
>>>>
>>>>
>>>>
>>>
> 

Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC

[Thomas Monjalon]

16/04/2018 19:23, Ferruh Yigit:
> On 4/1/2018 8:10 AM, Shahaf Shuler wrote:
> > Thursday, March 29, 2018 4:32 PM, Ferruh Yigit:
> >> On 3/29/2018 8:56 AM, Shahaf Shuler wrote:
> >>> Thursday, March 29, 2018 10:43 AM, Thomas Monjalon:
> >>>> 29/03/2018 07:38, Shahaf Shuler:
> >>>>> Wednesday, March 21, 2018 9:48 PM, Ferruh Yigit:
> >>>>>> DEV_RX_OFFLOAD_KEEP_CRC offload flag added.
> >>>
> >>> Logic should be :
> >>> Until DEV_RX_OFFLOAD_CRC_STRIP flag is removed:
> >>>  - Setting both KEEP_CRC & CRC_STRIP is INVALID - enforced by ethdev.
> >>>  - Setting only CRC_STRIP PMD should strip the CRC
> >>>  - Setting only KEEP_CRC PMD should keep the CRC
> >>>  - Not setting both PMD should *not* strip the CRC
> >>
> >> Hi Shahaf,
> >>
> >> I think we have two options,
> >>
> >> 1- This is v1 of this patch and also what you suggest:
> >> v18.05:
> >> - Send deprecation notice
> >>
> >> v18.08:
> >> - Add KEEP_CRC flag
> >> - Change the meaning not setting CRC_STRIP to "strip the CRC"
> > 
> > I think the above line ...
> > 
> >>
> >> v18.11:
> >> - Remove the CRC_STRIP flag completely
> > 
> > Should be here.
> > 
> > It is better to change the default behavior only once the STRIP_CRC flags is removed. 
> > Without it on v18.08 you break application that wants to have the CRC, and on v18.11 you break the application which actually used it.
> > It is better to have all the application changes in one release - 18.11. 
> > 
> >>
> >> I think this is more proper but takes more time.
> > 
> > Me too. 
> > 
> >>
> >>
> >> 2- Based on all the reality that all devices are doing CRC strip already:
> >>
> >> v18.05:
> >> - Add KEEP_CRC flag
> >> - Change the meaning not setting CRC_STRIP to "strip the CRC"
> >>
> >> v18.08:
> >> - Remove the CRC_STRIP flag completely
> >>
> >>
> >> With option two since not setting both KEEP_CRC & CRC_STRIP will mean
> >> "strip the CRC", it won't require a change in PMDs, only we can pay attention
> >> to get new PMDs according plan.
> >>
> >> This can be more problematic for the case that application ask for keeping
> >> CRC, because the way to say this was not setting CRC_STRIP, it changed to
> >> setting KEEP_CRC without notification. This can be less problem since many
> >> PMD already doesn't support keep crc.
> > 
> > but what about those which do support? 
> > You break application which uses PMDs which support this offload for the sake of the PMD which don't have this capability. 
> > 
> > I think #1 is the clean one. 
> 
> 
> Any decision here? So will we go with first version of this patch [1]?
> 
> [1]
> https://dpdk.org/dev/patchwork/patch/36288/

Please do a v3 according to what Shahaf is proposing:

- add KEEP_CRC in 18.08 + implement in PMDs + translate ~STRIP_CRC
- remove STRIP_CRC in 18.11 + toggle default to stripping

Or, are we able to do it quickly in 18.05-rc1, and remove in 18.08
with other offloads?

[dpdk-dev] [PATCH v3] doc: announce ethdev CRC strip flag deprecation

[Ferruh Yigit]

Make CRC stripping default behavior, deprecate flag
DEV_RX_OFFLOAD_CRC_STRIP.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

---
v2:
* Make CRC behavior active in this release

v3:
* Distribute switch into next two releases for more smooth transition
  v18.08: add new flag to keep crc, and implement this in PMDs
  v18.11: remove crc strip flag and make default behavior to strip crc
---
 doc/guides/rel_notes/deprecation.rst | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index fd9def20c..3ee1bd05e 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -100,6 +100,26 @@ Deprecation Notices
   between the VF representor and the VF or the parent PF. Those new fields
   are to be included in ``rte_eth_dev_info`` struct.
 
+* ethdev: A new offloading flag ``DEV_RX_OFFLOAD_KEEP_CRC`` will be added in v18.08,
+  This will replace the usage of not setting ``DEV_RX_OFFLOAD_CRC_STRIP`` flag
+  and will be implemented in PMDs accordingly.
+  In v18.08 both ``DEV_RX_OFFLOAD_KEEP_CRC`` and ``DEV_RX_OFFLOAD_CRC_STRIP`` flags
+  will be available, usage will be:
+  ``CRC_STRIP``: Strip CRC from packet
+  ``KEEP_CRC``: Keep CRC in packet
+  Both ``CRC_STRIP`` & ``KEEP_CRC``: Invalid
+  No flag: Keep CRC in packet
+
+* ethdev: In v18.11 ``DEV_RX_OFFLOAD_CRC_STRIP`` offload flag will be removed, default
+  behavior without any flag will be changed to CRC strip.
+  To keep CRC ``DEV_RX_OFFLOAD_KEEP_CRC`` flag is required.
+  ``KEEP_CRC``: Keep CRC in packet
+  No flag: Strip CRC from packet
+
+* ethdev: Make CRC stripping default behavior in v18.05 without any flag
+  and remove flag completely in v18.08
+  ``DEV_RX_OFFLOAD_CRC_STRIP`` flag will be removed.
+
 * i40e: The default flexible payload configuration which extracts the first 16
   bytes of the payload for RSS will be deprecated starting from 18.02. If
   required the previous behavior can be configured using existing flow
-- 
2.14.3

Re: [dpdk-dev] [PATCH v3] doc: announce ethdev CRC strip flag deprecation

[Andrew Rybchenko]

On 04/17/2018 04:12 PM, Ferruh Yigit wrote:
> Make CRC stripping default behavior, deprecate flag
> DEV_RX_OFFLOAD_CRC_STRIP.
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Re: [dpdk-dev] [PATCH v3] doc: announce ethdev CRC strip flag deprecation

[Ferruh Yigit]

On 4/17/2018 2:12 PM, Ferruh Yigit wrote:
> Make CRC stripping default behavior, deprecate flag
> DEV_RX_OFFLOAD_CRC_STRIP.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> ---
> v2:
> * Make CRC behavior active in this release
> 
> v3:
> * Distribute switch into next two releases for more smooth transition
>   v18.08: add new flag to keep crc, and implement this in PMDs
>   v18.11: remove crc strip flag and make default behavior to strip crc
> ---
>  doc/guides/rel_notes/deprecation.rst | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index fd9def20c..3ee1bd05e 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -100,6 +100,26 @@ Deprecation Notices
>    between the VF representor and the VF or the parent PF. Those new fields
>    are to be included in ``rte_eth_dev_info`` struct.
>  
> +* ethdev: A new offloading flag ``DEV_RX_OFFLOAD_KEEP_CRC`` will be added in v18.08,
> +  This will replace the usage of not setting ``DEV_RX_OFFLOAD_CRC_STRIP`` flag
> +  and will be implemented in PMDs accordingly.
> +  In v18.08 both ``DEV_RX_OFFLOAD_KEEP_CRC`` and ``DEV_RX_OFFLOAD_CRC_STRIP`` flags
> +  will be available, usage will be:
> +  ``CRC_STRIP``: Strip CRC from packet
> +  ``KEEP_CRC``: Keep CRC in packet
> +  Both ``CRC_STRIP`` & ``KEEP_CRC``: Invalid
> +  No flag: Keep CRC in packet
> +
> +* ethdev: In v18.11 ``DEV_RX_OFFLOAD_CRC_STRIP`` offload flag will be removed, default
> +  behavior without any flag will be changed to CRC strip.
> +  To keep CRC ``DEV_RX_OFFLOAD_KEEP_CRC`` flag is required.
> +  ``KEEP_CRC``: Keep CRC in packet
> +  No flag: Strip CRC from packet
> +
> +* ethdev: Make CRC stripping default behavior in v18.05 without any flag
> +  and remove flag completely in v18.08
> +  ``DEV_RX_OFFLOAD_CRC_STRIP`` flag will be removed.
> +

This last part is git artifact, I will send a new version to fix it.

>  * i40e: The default flexible payload configuration which extracts the first 16
>    bytes of the payload for RSS will be deprecated starting from 18.02. If
>    required the previous behavior can be configured using existing flow
> 

[dpdk-dev] [PATCH v4] doc: announce ethdev CRC strip flag deprecation

[Ferruh Yigit]

Make CRC stripping default behavior, deprecate flag
DEV_RX_OFFLOAD_CRC_STRIP.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
v2:
* Make CRC behavior active in this release

v3:
* Distribute switch into next two releases for more smooth transition
  v18.08: add new flag to keep crc, and implement this in PMDs
  v18.11: remove crc strip flag and make default behavior to strip crc

v4:
* Remove git artifact
---
 doc/guides/rel_notes/deprecation.rst | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index fd9def20c..dbc405450 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -100,6 +100,22 @@ Deprecation Notices
   between the VF representor and the VF or the parent PF. Those new fields
   are to be included in ``rte_eth_dev_info`` struct.
 
+* ethdev: A new offloading flag ``DEV_RX_OFFLOAD_KEEP_CRC`` will be added in v18.08,
+  This will replace the usage of not setting ``DEV_RX_OFFLOAD_CRC_STRIP`` flag
+  and will be implemented in PMDs accordingly.
+  In v18.08 both ``DEV_RX_OFFLOAD_KEEP_CRC`` and ``DEV_RX_OFFLOAD_CRC_STRIP`` flags
+  will be available, usage will be:
+  ``CRC_STRIP``: Strip CRC from packet
+  ``KEEP_CRC``: Keep CRC in packet
+  Both ``CRC_STRIP`` & ``KEEP_CRC``: Invalid
+  No flag: Keep CRC in packet
+
+* ethdev: In v18.11 ``DEV_RX_OFFLOAD_CRC_STRIP`` offload flag will be removed, default
+  behavior without any flag will be changed to CRC strip.
+  To keep CRC ``DEV_RX_OFFLOAD_KEEP_CRC`` flag is required.
+  ``KEEP_CRC``: Keep CRC in packet
+  No flag: Strip CRC from packet
+
 * i40e: The default flexible payload configuration which extracts the first 16
   bytes of the payload for RSS will be deprecated starting from 18.02. If
   required the previous behavior can be configured using existing flow
-- 
2.14.3

Re: [dpdk-dev] [PATCH v4] doc: announce ethdev CRC strip flag deprecation

[Shahaf Shuler]

Tuesday, April 17, 2018 4:40 PM, Ferruh Yigit:
> Subject: [PATCH v4] doc: announce ethdev CRC strip flag deprecation
> 
> Make CRC stripping default behavior, deprecate flag
> DEV_RX_OFFLOAD_CRC_STRIP.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> v2:
> * Make CRC behavior active in this release
> 
> v3:
> * Distribute switch into next two releases for more smooth transition
>   v18.08: add new flag to keep crc, and implement this in PMDs
>   v18.11: remove crc strip flag and make default behavior to strip crc
> 
> v4:
> * Remove git artifact
> ---
>  doc/guides/rel_notes/deprecation.rst | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index fd9def20c..dbc405450 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -100,6 +100,22 @@ Deprecation Notices
>    between the VF representor and the VF or the parent PF. Those new fields
>    are to be included in ``rte_eth_dev_info`` struct.
> 
> +* ethdev: A new offloading flag ``DEV_RX_OFFLOAD_KEEP_CRC`` will be
> +added in v18.08,
> +  This will replace the usage of not setting
> +``DEV_RX_OFFLOAD_CRC_STRIP`` flag
> +  and will be implemented in PMDs accordingly.
> +  In v18.08 both ``DEV_RX_OFFLOAD_KEEP_CRC`` and
> +``DEV_RX_OFFLOAD_CRC_STRIP`` flags
> +  will be available, usage will be:
> +  ``CRC_STRIP``: Strip CRC from packet
> +  ``KEEP_CRC``: Keep CRC in packet
> +  Both ``CRC_STRIP`` & ``KEEP_CRC``: Invalid
> +  No flag: Keep CRC in packet
> +
> +* ethdev: In v18.11 ``DEV_RX_OFFLOAD_CRC_STRIP`` offload flag will be
> +removed, default
> +  behavior without any flag will be changed to CRC strip.
> +  To keep CRC ``DEV_RX_OFFLOAD_KEEP_CRC`` flag is required.
> +  ``KEEP_CRC``: Keep CRC in packet
> +  No flag: Strip CRC from packet
> +
>  * i40e: The default flexible payload configuration which extracts the first 16
>    bytes of the payload for RSS will be deprecated starting from 18.02. If
>    required the previous behavior can be configured using existing flow
> --
> 2.14.3

Acked-by: Shahaf Shuler <shahafs@mellanox.com>

Re: [dpdk-dev] [PATCH v4] doc: announce ethdev CRC strip flag deprecation

[Thomas Monjalon]

17/04/2018 15:47, Shahaf Shuler:
> Tuesday, April 17, 2018 4:40 PM, Ferruh Yigit:
> > Subject: [PATCH v4] doc: announce ethdev CRC strip flag deprecation
> > 
> > Make CRC stripping default behavior, deprecate flag
> > DEV_RX_OFFLOAD_CRC_STRIP.
> > 
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> Acked-by: Shahaf Shuler <shahafs@mellanox.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Re: [dpdk-dev] [PATCH v4] doc: announce ethdev CRC strip flag deprecation

[Maxime Coquelin]

On 04/17/2018 03:39 PM, Ferruh Yigit wrote:
> Make CRC stripping default behavior, deprecate flag
> DEV_RX_OFFLOAD_CRC_STRIP.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> v2:
> * Make CRC behavior active in this release
> 
> v3:
> * Distribute switch into next two releases for more smooth transition
>    v18.08: add new flag to keep crc, and implement this in PMDs
>    v18.11: remove crc strip flag and make default behavior to strip crc
> 
> v4:
> * Remove git artifact
> ---
>   doc/guides/rel_notes/deprecation.rst | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 

Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Re: [dpdk-dev] [PATCH v4] doc: announce ethdev CRC strip flag deprecation

[Shahaf Shuler]

Wednesday, August 1, 2018 6:27 PM, Maxime Coquelin:
> Subject: Re: [dpdk-dev] [PATCH v4] doc: announce ethdev CRC strip flag
> deprecation
> 
> 
> 
> On 04/17/2018 03:39 PM, Ferruh Yigit wrote:
> > Make CRC stripping default behavior, deprecate flag
> > DEV_RX_OFFLOAD_CRC_STRIP.
> >
> > Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > ---
> > v2:
> > * Make CRC behavior active in this release
> >
> > v3:
> > * Distribute switch into next two releases for more smooth transition
> >    v18.08: add new flag to keep crc, and implement this in PMDs
> >    v18.11: remove crc strip flag and make default behavior to strip
> > crc
> >
> > v4:
> > * Remove git artifact
> > ---
> >   doc/guides/rel_notes/deprecation.rst | 16 ++++++++++++++++
> >   1 file changed, 16 insertions(+)
> >
> 
> Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Acked-by: Shahaf Shuler <shahafs@mellanox.com>