From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v2 2/2] ethdev: add new offload flag to keep CRC Date: Tue, 17 Apr 2018 00:13:45 +0200 Message-ID: <5080470.gr8dEl1KFW@xps> References: <20180320112631.107105-1-ferruh.yigit@intel.com> <5271b825-cfcc-e098-d34a-dd92f79d3a47@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: dev@dpdk.org, Neil Horman , John McNamara , Marko Kovacevic To: Ferruh Yigit , Shahaf Shuler Return-path: Received: from out3-smtp.messagingengine.com (out3-smtp.messagingengine.com [66.111.4.27]) by dpdk.org (Postfix) with ESMTP id BD62CAAC8 for ; Tue, 17 Apr 2018 00:13:48 +0200 (CEST) In-Reply-To: <5271b825-cfcc-e098-d34a-dd92f79d3a47@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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?