From: Shahaf Shuler <shahafs@mellanox.com>
To: Ferruh Yigit <ferruh.yigit@intel.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: Sun, 1 Apr 2018 07:10:25 +0000 [thread overview]
Message-ID: <DB7PR05MB4426E02DAAD369D94D9A25F7C3A70@DB7PR05MB4426.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <315855d9-2a6a-667a-4032-62c947323b02@intel.com>
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.
> >>
> >>
> >>
> >
next prev parent reply other threads:[~2018-04-01 7:10 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
2018-04-01 7:10 ` Shahaf Shuler [this message]
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=DB7PR05MB4426E02DAAD369D94D9A25F7C3A70@DB7PR05MB4426.eurprd05.prod.outlook.com \
--to=shahafs@mellanox.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=john.mcnamara@intel.com \
--cc=marko.kovacevic@intel.com \
--cc=nhorman@tuxdriver.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).