From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 2E4865690 for ; Tue, 14 Jun 2016 11:16:08 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga102.fm.intel.com with ESMTP; 14 Jun 2016 02:16:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.26,470,1459839600"; d="scan'208";a="827688065" Received: from irsmsx151.ger.corp.intel.com ([163.33.192.59]) by orsmga003.jf.intel.com with ESMTP; 14 Jun 2016 02:16:06 -0700 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.51]) by IRSMSX151.ger.corp.intel.com ([169.254.4.151]) with mapi id 14.03.0248.002; Tue, 14 Jun 2016 10:15:26 +0100 From: "Ananyev, Konstantin" To: Olivier MATZ , "dev@dpdk.org" CC: "johndale@cisco.com" , "Zhang, Helin" , "adrien.mazarguil@6wind.com" , "rahul.lakkireddy@chelsio.com" , "alejandro.lucero@netronome.com" , "sony.chacko@qlogic.com" Thread-Topic: [PATCH v2] mbuf: new flag when Vlan is stripped Thread-Index: AQHRuCT1tJhClSk81UqMy5GQmX82V5/nje2wgAAMQQCAABLpEIABAIiAgAAbMxA= Date: Tue, 14 Jun 2016 09:15:25 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836B707E8@irsmsx105.ger.corp.intel.com> References: <1463993205-5623-1-git-send-email-olivier.matz@6wind.com> <1464359593-3534-1-git-send-email-olivier.matz@6wind.com> <2601191342CEEE43887BDE71AB97725836B70190@irsmsx105.ger.corp.intel.com> <575EDA24.3020405@6wind.com> <2601191342CEEE43887BDE71AB97725836B703CC@irsmsx105.ger.corp.intel.com> <575FC133.3090205@6wind.com> In-Reply-To: <575FC133.3090205@6wind.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2] mbuf: new flag when Vlan is stripped X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Jun 2016 09:16:08 -0000 Hi Olivier >=20 > Hi Konstantin, >=20 > On 06/13/2016 06:31 PM, Ananyev, Konstantin wrote: > > > > Hi Olivier, > > > >> > >> Hi Konstantin, > >> > >> On 06/13/2016 04:42 PM, Ananyev, Konstantin wrote: > >>>> The behavior of PKT_RX_VLAN_PKT was not very well defined, resulting= in > >>>> PMDs not advertising the same flags in similar conditions. > >>>> > >>>> Following discussion in [1], introduce 2 new flags PKT_RX_VLAN_STRIP= PED > >>>> and PKT_RX_QINQ_STRIPPED that are better defined: > >>>> > >>>> PKT_RX_VLAN_STRIPPED: a vlan has been stripped by the hardware an= d its > >>>> tci is saved in mbuf->vlan_tci. This can only happen if vlan stri= pping > >>>> is enabled in the RX configuration of the PMD. > >>>> > >>>> For now, the old flag PKT_RX_VLAN_PKT is kept but marked as deprecat= ed. > >>>> It should be removed from applications and PMDs in a future revision= . > >>> > >>> I am not sure it has to be deprecated & removed. > >>> ixgbe (and igb as I can read the specs) devices can provide informati= on is that > >>> a vlan packet or not even when vlan stripping is disabled. > >>> Right now ixgbe PMD do carry thins information to the user, > >>> and I suppose igb could be improved to carry it too. > >>> So obviously we need a way to pass that information to the upper laye= r. > >>> I remember it was a discussion about introducing new packet_type > >>> instead of ol_flag value PKT_RX_VLAN_PKT. > >>> But right now it is not there, and again I don't know how easy it wou= ld be to replace > >>> one with another without performance considering that packet_type is = not supported > >>> now by ixgbe vRX. > >>> If we would be able to replace it, then yes we can deprecate and drop= the PKT_RX_VLAN_PKT. > >>> But till then, I think we'd better keep it. > >> > >> I think the packet_type feature would be more appropriate than a flag > >> for carrying this kind of info. > >> > >> Currently the behavior of PKT_RX_VLAN_PKT is not properly defined, > >> and it is not the same on all PMDs. So, from an application > >> perspective, it's not usable except if it knows that the underlying > >> PMD is an ixgbe. > > > > Yes, but it might be apps which do use that ixgbe functionality. > > > >> This is not acceptable for a generic API and that's > >> why I think this flag, as it is today, should be deprecated. > > > > I suppose we can't deprecate existing functionality without > > providing working alternative. > > I agree there is no proper way to know right now which device > > supports it, which not, but to me it means we should add such ability, > > not deprecate existing and (I believe) useful functionality. > > > >> > >> It won't prevent an application from using the flag right after my > >> commit, but it will warn the user that the flag should not be used > >> as is. If someone is willing to work on this feature for 16.11, why > >> not but again, I think using the packet_type is more appropriate. > > > > I am not against providing that information via packet_type. > > What I am saying: > > 1) right now it is not here. > > 2) it might not that easy in terms of performance. > > > >> The problem is that I don't want to have this flag in this state > >> forever, and I also don't want to add in rte_mbuf.h a comment > >> saying "this flag does this on ixgbe and that on other drivers". > > > > Then we need either: > > - implement it as ptype > > - add user ability to query is that flag is supported by the underlying= device. > > > >> > >> If we decide to generalize the ixgbe behavior for all PMDs for this > >> flag, it will break the applications relying on this flag but with > >> other PMDs. So for the same reason we added a new PKT_RX_VLAN_STRIPPED > >> we cannot change the behavior of an existing flag. > > > > Ok, then let's make PKT_RX_VLAN_STRIPPED =3D=3D PKT_RX_VLAN, > > and assign new value to the PKT_RX_VLAN. > > Or have PKT_RX_VLAN_STRIPPED =3D=3D PKT_RX_VLAN and create a new one: > > PKT_RX_VLAN_PRESENT or so. > > ? > > >=20 > I think adding this new flag/packet_type is a new feature, > because only ixgbe was behaving like this, and this was not > documented. To me, marking the old flag as deprecated is > a good compromise to keep the application relying on this > working. If you feel the term "deprecated" is not adapted, > we could reword it to something weaker. Yes, that would do I think. Basically my only concern that we will mark it as deprecated, and then will remove it (as it is deprecated), without providing anything new to replace it.=20 >=20 > We should try to not stay in that state too long, Agree. > and anybody willing to implement this feature would be welcome. For my > part, this is not something I plan to do yet. >=20 Ok, we'll see what we can do for 16.11. But no hard promises right now either :) Thanks Konstantin > Regards, > Olivier