From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id 93DAC1B19C for ; Tue, 24 Oct 2017 04:08:26 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Oct 2017 19:08:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,424,1503385200"; d="scan'208";a="166710079" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.136]) ([10.241.225.136]) by fmsmga006.fm.intel.com with ESMTP; 23 Oct 2017 19:08:25 -0700 To: Olivier Matz , dev@dpdk.org Cc: thomas@monjalon.net References: <20171023121623.27466-1-olivier.matz@6wind.com> From: Ferruh Yigit Message-ID: <4171e3c8-31eb-4c40-117b-87ac6a585764@intel.com> Date: Mon, 23 Oct 2017 19:08:25 -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: <20171023121623.27466-1-olivier.matz@6wind.com> 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: Tue, 24 Oct 2017 02:08:27 -0000 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. 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. > It is likely that some PMDs do not set the proper flags when doing vlan > offload, and it would be worth making a pass on all of them. This is the worrying statement :) > > Link: [1] http://dpdk.org/ml/archives/dev/2017-June/067712.html > Signed-off-by: Olivier Matz > --- > > An alternative would have been to keep PKT_RX_VLAN_PKT and > PKT_RX_QINQ_PKT and recycle them for this new feature. I think it will be confusing to use same names. > > I choosed this way (rename the flags) because: > - changing the name is a good way to highlight that something > changed +1 > - the old name was not that good (PKT_blabla_PKT) > - there was no reaction to the proposition in [1] > > I'm also open to recycle the old name if we find good reasons > for it. It would make the patch smaller since there would be no > modification in drivers. > > In both cases, it is important that PMD maintainers check > that their use of VLAN flags is correct. Example: > - vlan is not stripped and tci is saved: PKT_RX_VLAN > - vlan is stripped and tci is saved: PKT_RX_VLAN | PKT_RX_VLAN_STRIPPED > This can be completed by the packet type which describes the > packet data. I hope the API documentation is clear enough. > > Ferruh, do you think you could check with PMD maintainers for next > version? Sure I can try, but according current experience there is no efficient way to do this right now. > 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" > 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. > 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 Thanks, ferruh <...>