From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v2 2/2] ethdev: add new offload flag to keep CRC Date: Mon, 16 Apr 2018 18:23:31 +0100 Message-ID: <5271b825-cfcc-e098-d34a-dd92f79d3a47@intel.com> References: <20180320112631.107105-1-ferruh.yigit@intel.com> <20180321194730.52068-2-ferruh.yigit@intel.com> <15555561.lU3UqzoiBB@xps> <315855d9-2a6a-667a-4032-62c947323b02@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Neil Horman , John McNamara , Marko Kovacevic , "dev@dpdk.org" To: Shahaf Shuler , Thomas Monjalon Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 0453FCFA4 for ; Mon, 16 Apr 2018 19:23:33 +0200 (CEST) In-Reply-To: Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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. >>>> >>>> >>>> >>> >