From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from wes1-so2.wedos.net (wes1-so2.wedos.net [46.28.106.16]) by dpdk.org (Postfix) with ESMTP id E34FB275D for ; Fri, 10 Jun 2016 10:55:02 +0200 (CEST) Received: from pcviktorin.fit.vutbr.cz (pcviktorin.fit.vutbr.cz [147.229.13.147]) by wes1-so2.wedos.net (Postfix) with ESMTPSA id 3rQwwk3m3Vz2nh; Fri, 10 Jun 2016 10:55:02 +0200 (CEST) Date: Fri, 10 Jun 2016 10:49:52 +0200 From: Jan Viktorin To: Olivier Matz Cc: "Hunt, David" , Shreyansh Jain , "dev@dpdk.org" , "jerin.jacob@caviumnetworks.com" Message-ID: <20160610104952.55997f4d@pcviktorin.fit.vutbr.cz> In-Reply-To: <575A6C68.3090007@6wind.com> References: <1464874043-67467-1-git-send-email-david.hunt@intel.com> <1464965906-108927-1-git-send-email-david.hunt@intel.com> <1464965906-108927-2-git-send-email-david.hunt@intel.com> <5756931C.70805@intel.com> <57593962.6010802@intel.com> <20160609150918.502322c8@pcviktorin.fit.vutbr.cz> <575A6C68.3090007@6wind.com> Organization: RehiveTech MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations 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: Fri, 10 Jun 2016 08:55:03 -0000 On Fri, 10 Jun 2016 09:29:44 +0200 Olivier Matz wrote: > Hi, > > On 06/09/2016 03:09 PM, Jan Viktorin wrote: > >>> My suggestion is to have an additional flag, > >>> 'MEMPOOL_F_PKT_ALLOC', which, if specified, would: > >>> > >>> ... #define MEMPOOL_F_SC_GET 0x0008 #define > >>> MEMPOOL_F_PKT_ALLOC 0x0010 ... > >>> > >>> in rte_mempool_create_empty: ... after checking the other > >>> MEMPOOL_F_* flags... > >>> > >>> if (flags & MEMPOOL_F_PKT_ALLOC) rte_mempool_set_ops_byname(mp, > >>> RTE_MBUF_DEFAULT_MEMPOOL_OPS) > >>> > >>> And removing the redundant call to rte_mempool_set_ops_byname() > >>> in rte_pktmbuf_create_pool(). > >>> > >>> Thereafter, rte_pktmbuf_pool_create can be changed to: > >>> > >>> ... mp = rte_mempool_create_empty(name, n, elt_size, cache_size, > >>> - sizeof(struct rte_pktmbuf_pool_private), socket_id, 0); > >>> + sizeof(struct rte_pktmbuf_pool_private), socket_id, + > >>> MEMPOOL_F_PKT_ALLOC); if (mp == NULL) return NULL; > >> > >> Yes, this would simplify somewhat the creation of a pktmbuf pool, > >> in that it replaces the rte_mempool_set_ops_byname with a flag bit. > >> However, I'm not sure we want to introduce a third method of > >> creating a mempool to the developers. If we introduced this, we > >> would then have: 1. rte_pktmbuf_pool_create() 2. > >> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC set (which > >> would use the configured custom handler) 3. > >> rte_mempool_create_empty() with MEMPOOL_F_PKT_ALLOC __not__ set > >> followed by a call to rte_mempool_set_ops_byname() (would allow > >> several different custom handlers to be used in one application > >> > >> Does anyone else have an opinion on this? Oliver, Jerin, Jan? > > > > I am quite careful about this topic as I don't feel to be very > > involved in all the use cases. My opinion is that the _new API_ > > should be able to cover all cases and the _old API_ should be > > backwards compatible, however, built on top of the _new API_. > > > > I.e. I think, the flags MEMPOOL_F_SP_PUT, MEMPOOL_F_SC_GET (relicts > > of the old API) should be accepted by the old API ONLY. The > > rte_mempool_create_empty should not process them. > > The rte_mempool_create_empty() function already processes these flags > (SC_GET, SP_PUT) as of today. Yes, I consider it quite strange. When thinking more about the mempool API, I'd move the flags processing to the rte_mempool_create. Semantically, it makes more sense as the "empty" clearly describes that it is empty. But with the flags, it is not... What is the reason to have those flags there? > > > Similarly for a potential MEMPOOL_F_PKT_ALLOC, I would not polute the > > rte_mempool_create_empty by this anymore. > > +1 > > I think we should stop adding flags. Flags are prefered for independent > features. Here, what would be the behavior with MEMPOOL_F_PKT_ALLOC + > MEMPOOL_F_SP_PUT? +1 :) > > Another reason to not add this flag is the rte_mempool library > should not be aware of mbufs. The mbuf pools rely on mempools, but > not the contrary. > > > > In overall we would get exactly 2 approaches (and not more): > > future) Well, now I can see that I've just written the same thing but with my own words :). > > > > * using rte_mempool_create with flags calling the > > rte_mempool_create_empty and rte_mempool_set_ops_byname internally > > (so this layer can be marked as deprecated and removed in the > > Agree. This was one of the objective of my mempool rework patchset: > provide a more flexible API, and avoid functions with 10 to 15 > arguments. > > > * using rte_mempool_create_empty + rte_mempool_set_ops_byname - > > allowing any customizations but with the necessity to change the > > applications (new preferred API) > > Yes. > And if required, maybe a third API is possible in case of mbuf pools. I don't count this. It's an upper layer, but yes. > Indeed, the applications are encouraged to use rte_pktmbuf_pool_create() > to create a pool of mbuf instead of mempool API. If an application > wants to select specific ops for it, we could add: > > rte_pktmbuf_pool_create_(..., name) Seems like a good idea. > > instead of using the mempool API. > I think this is what Shreyansh suggests when he says: > > It sounds fine if calls to rte_mempool_* can select an external > handler *optionally* - but, if we pass it as command line, it would > be binding (at least, semantically) for rte_pktmbuf_* calls as well. > Isn't it? I think, the question here is whether the processing of such optional command line specification is up to the application or up the the DPDK core. If we leave it in applications, it's just a matter of API. > > > So, the old applications can stay as they are (OK, with a possible > > new flag MEMPOOL_F_PKT_ALLOC) and the new one can do the same but you > > have to set the ops explicitly. > > > > The more different ways of using those APIs we have, the greater hell > > we have to maintain. > > I'm really not in favor of a MEMPOOL_F_PKT_ALLOC flag in mempool api. Your arguments are valid, +1. > > I think David's patch is already a good step forward. Let's do it > step by step. Next step is maybe to update some applications (at least > testpmd) to select a new pool handler dynamically. We can probably make an API to process the command line by applications that configures a mempool automatically. So, it would be a oneliner or something like that. Like rte_mempool_create_from_devargs(...). Jan > > Regards, > Olivier