From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Shahaf Shuler <shahafs@mellanox.com>,
Thomas Monjalon <thomas@monjalon.net>
Cc: Neil Horman <nhorman@tuxdriver.com>,
John McNamara <john.mcnamara@intel.com>,
Marko Kovacevic <marko.kovacevic@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC
Date: Thu, 29 Mar 2018 14:32:15 +0100 [thread overview]
Message-ID: <315855d9-2a6a-667a-4032-62c947323b02@intel.com> (raw)
In-Reply-To: <DB7PR05MB4426C7D0AE948BB2CFEBB5F3C3A20@DB7PR05MB4426.eurprd05.prod.outlook.com>
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.
>>
>>
>>
>
next prev parent reply other threads:[~2018-03-29 13:32 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-03-20 11:26 [dpdk-dev] [PATCH] doc: announce ethdev CRC strip flag deprecation Ferruh Yigit
2018-03-20 11:35 ` Thomas Monjalon
2018-03-20 17:23 ` Ferruh Yigit
2018-03-21 19:47 ` [dpdk-dev] [PATCH v2 1/2] " Ferruh Yigit
2018-03-21 19:47 ` [dpdk-dev] [PATCH v2 2/2] ethdev: add new offload flag to keep CRC Ferruh Yigit
2018-03-21 21:05 ` Thomas Monjalon
2018-03-29 5:38 ` Shahaf Shuler
2018-03-29 7:43 ` Thomas Monjalon
2018-03-29 7:56 ` Shahaf Shuler
2018-03-29 13:32 ` Ferruh Yigit [this message]
2018-04-01 7:10 ` Shahaf Shuler
2018-04-16 17:23 ` Ferruh Yigit
2018-04-16 22:13 ` Thomas Monjalon
2018-04-17 13:12 ` [dpdk-dev] [PATCH v3] doc: announce ethdev CRC strip flag deprecation Ferruh Yigit
2018-04-17 13:31 ` Andrew Rybchenko
2018-04-17 13:36 ` Ferruh Yigit
2018-04-17 13:39 ` [dpdk-dev] [PATCH v4] " Ferruh Yigit
2018-04-17 13:47 ` Shahaf Shuler
2018-05-28 1:05 ` Thomas Monjalon
2018-08-01 15:27 ` Maxime Coquelin
2018-08-02 8:50 ` Shahaf Shuler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=315855d9-2a6a-667a-4032-62c947323b02@intel.com \
--to=ferruh.yigit@intel.com \
--cc=dev@dpdk.org \
--cc=john.mcnamara@intel.com \
--cc=marko.kovacevic@intel.com \
--cc=nhorman@tuxdriver.com \
--cc=shahafs@mellanox.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).