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 E18E9326C for ; Sun, 8 Jan 2017 13:39:58 +0100 (CET) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga104.fm.intel.com with ESMTP; 08 Jan 2017 04:39:57 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,335,1477983600"; d="scan'208";a="1109588015" Received: from irsmsx108.ger.corp.intel.com ([163.33.3.3]) by fmsmga002.fm.intel.com with ESMTP; 08 Jan 2017 04:39:56 -0800 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.38]) by IRSMSX108.ger.corp.intel.com ([169.254.11.173]) with mapi id 14.03.0248.002; Sun, 8 Jan 2017 12:39:55 +0000 From: "Ananyev, Konstantin" To: Adrien Mazarguil CC: "Bie, Tiwei" , "dev@dpdk.org" , "Lu, Wenzhuo" , "Mcnamara, John" , "olivier.matz@6wind.com" , "thomas.monjalon@6wind.com" , "Zhang, Helin" , "Dai, Wei" , "Wang, Xiao W" Thread-Topic: [PATCH v5 3/8] ethdev: reserve capability flags for PMD-specific API Thread-Index: AQHSZlwkSqPRhUiAJ0W/zN9yefcZuKEoXWKAgAAGlQCAAAT1YIAAImCAgAALQACAAGj3AIAAkIQAgAAq7VCAAHlygIAERIAg Date: Sun, 8 Jan 2017 12:39:55 +0000 Message-ID: <2601191342CEEE43887BDE71AB9772583F10241C@irsmsx105.ger.corp.intel.com> References: <1483514502-32841-1-git-send-email-tiwei.bie@intel.com> <1483514502-32841-4-git-send-email-tiwei.bie@intel.com> <2601191342CEEE43887BDE71AB9772583F0FEE0C@irsmsx105.ger.corp.intel.com> <20170104143923.GA57552@dpdk19> <2601191342CEEE43887BDE71AB9772583F0FEE6D@irsmsx105.ger.corp.intel.com> <20170104170011.GB56511@dpdk19> <2601191342CEEE43887BDE71AB9772583F100375@irsmsx105.ger.corp.intel.com> <20170104235608.GA133542@dpdk19> <20170105083322.GK12822@6wind.com> <2601191342CEEE43887BDE71AB9772583F1006B7@irsmsx105.ger.corp.intel.com> <20170105182141.GS12822@6wind.com> In-Reply-To: <20170105182141.GS12822@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 v5 3/8] ethdev: reserve capability flags for PMD-specific API 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: Sun, 08 Jan 2017 12:39:59 -0000 Hi Adrien, >=20 > Hi Konstantin, >=20 > On Thu, Jan 05, 2017 at 11:32:38AM +0000, Ananyev, Konstantin wrote: > > Hi Adrien, > > > > > > > > On Thu, Jan 05, 2017 at 07:56:08AM +0800, Tiwei Bie wrote: > > > > On Thu, Jan 05, 2017 at 01:44:18AM +0800, Ananyev, Konstantin wrote= : > > > > [...] > > > > > > > > > > > > > > I understand that. > > > > > > > My question was: suppose user would like to create a bonded d= evice over 2 NICs. > > > > > > > One of them is ixgbe, while other would be some other type. > > > > > > > In future get_dev_info() for each of them might return DEV_RX= _OFFLOAD_RESERVED_0 bit as set. > > > > > > > But it would mean completely different thing. > > > > > > > How bonded device would know that to deal properly? > > > > > > > > > > > > > > Another example - user has 2 NICs of different type and would= like to send the same packet on both of them simultaneously. > > > > > > > As PKT_TX_RESERVED might mean different things for these devi= ces, and user would like to use let say > > > > > > > PKT_TX_IXGBE_MACSEC on one of them, he would need to do a cop= y of them, instead just increment a refcnt. > > > > > > > > > > > > > > Similar issues might arise at RX handling: user got a packet = with PKT_RX_RESERVED_0 set. > > > > > > > What does it really mean if there are different NIC types in = the system? > > > > > > > The only way to answer that question, as I can see, is to ke= ep track from what NIC that packet was received. > > > > > > > Which I suppose, is not always convenient. > > > > > > > > > > > > > > > > > > > The main purpose is to put the PMD-specific APIs in a separate > > > > > > namespace instead of mixing the PMD-specific APIs and global AP= Is > > > > > > up, and also save the bits in mbuf.ol_flags. > > > > > > > > > > > > There are other ways to achieve this goal, such as introducing > > > > > > the PMD specific ol_flags in mbuf second cache line as you said= . > > > > > > I just thought defining some reserved bits seems to be the most > > > > > > simple way which won't introduce many changes. > > > > > > > > > > > > What's your suggestions? Should I just revert the changes and > > > > > > define the generic flags directly? > > > > > > > > > > Yes, that would be my preference. > > > > > As I said above - spending extra bit in ol_flags doesn't look li= ke a big problem to me. > > > > > In return there would be no need to consider how to handle all th= at confusing scenarios in future. > > > > > > > > Okay. I'll update my patches. Thanks a lot for your comments. > > > > > > Well, I do not agree with Konstantin (no one saw this coming eh?) > > > > :) > > > > >and do not think you need to update your series again. > > > > > > PMD-specific symbols have nothing to do in the global namespace in my > > > opinion, they are not versioned and may evolve without notice. Neithe= r > > > applications nor the bonding PMD can rely on them. That's the trade-o= ff. > > > > Not sure I do understand your reasoning. > > For me MACSEC offload is just one of many HW offloads that we support > > and should be treated in exactly the same way. > > Applications should be able to use it in a transparent and reliable way= , > > not only under some limited conditions. > > Otherwise what is the point to introduce it at all? >=20 > Well my first reply to this thread was asking why isn't the whole API glo= bal > from the start then? That's good question, and my preference would always be to have the API to configure this feature as generic one. I guess the main reason why it is not right now we don't reach an agreement how this API should look like:=20 http://dpdk.org/ml/archives/dev/2016-September/047810.html But I'll leave it to the author to provide the real reason here.=20 >=20 > Given there are valid reasons for it not to and no plan to make it so in = the > near future, applications must be aware that they are including > rte_pmd_ixgbe.h to use it. That in itself is a limiting condition, right? Yes, it is definitely a limiting factor. Though even if API to configure device to use macsec would be PMD specific = right now, The API to query that capability and the API to use it at datapath (mbuf.ol= _flags) still can be (and I think should be) device independent and transparent to use. = =20 >=20 > > Yes, right now it is supported only by ixgbe PMD, but why that should b= e the > > reason to treat is as second-class citizen? > > Let say PKT_TX_TUNNEL_* offloads also are supported only by one PMD rig= ht now. >=20 > You are right about PKT_TX_TUNNEL_*, however these flags exist on their o= wn > and are not tied to any API function calls, unlike in this series where > PKT_TX_MACSEC can only be used if the DEV_TX_OFFLOAD_MACSEC_INSERT > capability is present=20 I don't think PKT_TX_TUNNEL_* 'exists on its own'. To use it well behaving app have to: 1) Query that device does provide that capability: DEV_TX_OFFLOAD_*_TNL_TSO 2) configure PMD( & device) to use that capability 3) use that offload at run-time TX code (mb->ol_flags |=3D ...; mb->tx_offl= oad =3D ...) For PKT_TX_TUNNEL_* 2) is pretty simple - user just need to make sure that full-featured TX function will be selected: txconf.txq_flags =3D 0; ...; rte_eth_tx_queue_setup(..., &txconf); =20 For TX_MACSEC, as I understand 2) will be more complicated and right now is PMD specific, but anyway the main pattern remains the same. So at least 1) and 3) could be kept device neutral. >and the whole thing was configured through > rte_pmd_ixgbe_macsec_*() calls after including rte_pmd_ixgbe.h. >=20 > To be clear it is not about MACsec per se (as a standardized protocol, I > think related definitions for offloads have their place), but it has to d= o > with the fact that the rest of the API is PMD-specific and there is a > dependency between them. >=20 > > > Therefore until APIs are made global, the safe compromise is to defin= e > > > neutral, reserved symbols that any PMD can use to implement their own > > > temporary APIs for testing purposes. These can be renamed later witho= ut > > > changing their value as long as a single PMD uses them. > > > > Ok, so what we'll gain by introducing PKT_TX_RESERVED instead of PKT_TX= _MACSEC? > > As I said in my previous mail the redefinition for the same ol_flag bit= (and dev capabilities) > > by different PMD might create a lot of confusion in future. > > Does the potential saving of 1 bit really worth it? >=20 > That is one benefit, but my point is mainly to keep applications aware th= at > they are using an API defined by a single PMD, which may be temporary and > whose symbols are not versioned. As applications have to use PMD specific functions to configure it they def= initely are aware. >=20 > Consider this: >=20 > rte_mbuf.h: >=20 > #define PKT_TX_RESERVED_0 (1 << 42) >=20 > rte_pmd_ixgbe.h: >=20 > #define PKT_TX_MACSEC PKT_TX_RESERVED_0 >=20 > That way, applications have to get the PKT_TX_MACSEC definition where the > rest of the API is also defined. >=20 > Other PMDs may reuse PKT_TX_RESERVED_0 and other reserved flags to implem= ent > their own experimental APIs. That's the main thing I am opposed to. I think that by allowing PMD to redefine meaning of mbuf.ol_flags and dev_info.(rx| tx)_offload_capa=20 we just asking for trouble. Let say tomorrow, i40e will redefine DEV_TX_OFFLOAD_RESERVED_0 and PKT_TX_R= ESERVED_0 to implement new specific TX offload (PKT_TX_FEATURE_X). Now let say we have an application that works over both ixgbe and i40e and would like to use both TX_MACSEC and TC_FEATURE_X offloads whenever the= y are available. As I can see, with the approach you proposed the only way for the applicati= on to make it is to support 2 different TX code paths (or at least some parts of it). To me that way looks inconvenient to the users and source of future trouble= s. Same for RX: somewhere at upper layer user got a packet with PKT_RX_RESERV= ED_0 set. What does it really mean if there are different NIC types in the system? >=20 > Applications and the bonding PMD can easily be made aware that such reser= ved > flags cannot be shared between ports unless they know what the underlying > PMD is, which is already a requirement to use this API in the first place > (for instance, calling rte_pmd_ixgbe_macsec_*() functions with another > vendor's port_id may crash the application). I am talking about that code: rte_eth_bond_create(const char *name, uint8_t mode, uint8_t socket_id) { ... /* Take the first dev's offload capabilities */ internals->rx_offload_capa =3D dev_info.rx_offload_capa; internals->tx_offload_capa =3D dev_info.tx_offload_capa; ... internals->rx_offload_capa &=3D dev_info.rx_offload_capa; internals->tx_offload_capa &=3D dev_info.tx_offload_capa; Obviously with what you are suggesting it is not valid any more. Bonded device need to support a MASK of all device reserved offloads to exc= lude them from common subset.=20 Any user app(/lib) that does similar thing would also have to be changed. >=20 > So the idea if/when the API is made global is to rename PKT_TX_RESERVED_0= to > PKT_TX_MACSEC and keep its original value. >=20 > If other PMDs also implemented PKT_TX_RESERVED_0 in the meantime, it is > redefined using a different value. If there is no room left to do so, the= se > PMDs are out of luck I guess, and their specific API is disabled/removed > until something gets re-designed. >=20 > How about this? I still think that we shouldn't allow PMDs to redefine mbuf.olflags and dev_info.(rx|tx)_offload_capa.=20 See above for my reasons. Konstantin