From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 5B0A31B895 for ; Wed, 25 Oct 2017 02:42:56 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Oct 2017 17:42:55 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,430,1503385200"; d="scan'208";a="166675711" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.60]) ([10.241.225.60]) by fmsmga005.fm.intel.com with ESMTP; 24 Oct 2017 17:42:55 -0700 To: Olivier MATZ Cc: dev@dpdk.org, thomas@monjalon.net References: <20171023121623.27466-1-olivier.matz@6wind.com> <4171e3c8-31eb-4c40-117b-87ac6a585764@intel.com> <20171024160922.fu7qiciondyikgvb@platinum> From: Ferruh Yigit Message-ID: Date: Tue, 24 Oct 2017 17:42:55 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171024160922.fu7qiciondyikgvb@platinum> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH] mbuf: rename deprecated VLAN flags X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Oct 2017 00:42:57 -0000 On 10/24/2017 9:09 AM, Olivier MATZ wrote: > Hi Ferruh, > > On Mon, Oct 23, 2017 at 07:08:25PM -0700, Ferruh Yigit wrote: >> On 10/23/2017 5:16 AM, Olivier Matz wrote: >>> PKT_RX_VLAN_PKT and PKT_RX_QINQ_PKT are deprecated for a while. >>> As explained in [1], these flags were kept to let the applications and >>> PMDs move to the new flag. There is also a need to support Rx vlan >>> offload without vlan strip (at least for the ixgbe driver). >>> >>> This patch renames the old flags for this feature, knowing that some >>> PMDs were using PKT_RX_VLAN_PKT and PKT_RX_QINQ_PKT to indicate that >>> the vlan tci has been saved in the mbuf structure. >> >> So this patch merges two steps, >> - remove old flags >> - Add new flag to separate stripped and saved vlan info. >> >> Overall looks like a good idea. >> >> Just to confirm, in old usage, "PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED", >> both were meaning same thing, right. And "PKT_RX_VLAN_PKT" kept for backward >> compatibility. >> So previous "PKT_RX_VLAN_PKT" was way to say "stripped and saved", converting it >> to "PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED" as done in this patch should be safe. >> Only may be missing cases that only "PKT_RX_VLAN" is valid. > > Before this patch: > - PKT_RX_VLAN_PKT | PKT_RX_VLAN_STRIPPED: stripped and saved > - PKT_RX_VLAN_STRIPPED: unused, but also means stripped and saved > - PKT_RX_VLAN_PKT: undefined, but present in several drivers. In some of > them, I think it means 'not stripped and saved'. > > After this patch: > - PKT_RX_VLAN: not stripped and saved > - PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED: stripped and saved > - PKT_RX_VLAN_STRIPPED: same (stripped and saved), PKT_RX_VLAN is implied > I think we can forbid to have this flag alone. What I understand is PKT_RX_VLAN_STRIPPED doesn't implies "PKT_RX_VLAN", but both should be used together, only using "PKT_RX_VLAN_STRIPPED" is not valid, this is OK too. > >> And is "PKT_RX_VLAN_STRIPPED" implies the "PKT_RX_VLAN"? This was mentioned is >> the initial proposal, but it looks like that is not the case in this patch. > > Yes, I can clarify it in the documentation. > >> [...] >>> This patch has no impact on applications that do not >>> use the deprecated flags. >> >> I guess this is only true if "PKT_RX_VLAN_STRIPPED" implies the "PKT_RX_VLAN" > > From what I see, currently PKT_RX_VLAN_STRIPPED is always used > with PKT_RX_VLAN_PKT in PMDs, so I think there will be not impact. > >>> For other applications, renaming >>> the flag would restore the same behavior. >> >> Not sure, for an old application, using "PKT_RX_VLAN_PKT", can't just rename to >> "PKT_RX_VLAN" and use as it is because now it doesn't say if vlan stripped or >> not. So old applications seems may be effected, if so this may require to be >> notified in advance. > > Old applications using PKT_RX_VLAN_PKT have been notified > since 16.11 that the flag is deprecated. > > If the application is relying on the undocumented fact that > PKT_RX_VLAN_PKT means "not stripped and saved" on some PMDs, it will > continue to work as is after renaming. OK. > >>> But, since we are >>> close to the release, applying it early after the release could >>> also be considered. >> >> Is there any benefit to be in this release, one think I can think of is 17.11 >> being LTS, is there any other? >> >> And what do you think doing in two steps, >> - in this release remove deprecated flags >> - in the beginning of the next release introduce the new flags > > I think one good side effect of having only one patch is that > it doesn't break applications that were using an undocumented > behavior of a PMD (it just requires a rename). Makes sense, overall patch looks good to me, only concern is again making ethdev layer update in rc2. If there is no objection, and Thomas is also OK, lets get this into rc2. Meanwhile patch requires update because of qede PMD update, would you mind sending a new version based on latest next-net? Thanks, ferruh > > Olivier >