Hi Olivier, > -----Original Message----- > From: Olivier Matz [mailto:olivier.matz at 6wind.com] > Sent: Friday, June 10, 2016 1:00 PM > To: Jan Viktorin <viktorin at rehivetech.com>; Hunt, David > <david.hunt at intel.com> > Cc: Shreyansh Jain <shreyansh.jain at nxp.com>; dev at dpdk.org; > jerin.jacob at caviumnetworks.com > Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool > operations > > 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. > > > 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? > > 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.
Agree - mempool should be agnostic of the mbufs using it. But, mempool should be aware of the allocator it is using, in my opinion. And, agree with your argument of "MEMPOOL_F_PKT_ALLOC + MEMPOOL_F_SP_PUT" - it is bad semantics. > > > > In overall we would get exactly 2 approaches (and not more): > > > > * 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 > > future) > > 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. > 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_<something>(..., name) > > 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? Er. I think I should clarify the context. I was referring to the 'command-line-argument-for-selecting-external-mem-allocator' comment. I was just highlighting that probably it would cause conflict with the two APIs. But, having said that, I agree with you about "...applications are encouraged to use rte_pktmbuf_pool_create() to create a pool of mbuf...". > > > > 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. Agree. Flags are not pretty way of handling mutually exclusive features - they are not intuitive. Semantically cleaner API is better approach. > > 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. Fair enough. We can slowly make changes to all applications to show 'best practice' of creating buffer or packet pools. > > Regards, > Olivier - Shreyansh
