Hi David, > -----Original Message----- > From: Hunt, David [mailto:david.hunt at intel.com] > Sent: Thursday, June 09, 2016 3:10 PM > To: Shreyansh Jain <shreyansh.jain at nxp.com>; dev at dpdk.org > Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com; > jerin.jacob at caviumnetworks.com > Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool > operations > > Hi Shreyansh, > > On 8/6/2016 2:48 PM, Shreyansh Jain wrote: > > Hi David, > > > > Thanks for explanation. I have some comments inline... > > > >> -----Original Message----- > >> From: Hunt, David [mailto:david.hunt at intel.com] > >> Sent: Tuesday, June 07, 2016 2:56 PM > >> To: Shreyansh Jain <shreyansh.jain at nxp.com>; dev at dpdk.org > >> Cc: olivier.matz at 6wind.com; viktorin at rehivetech.com; > >> jerin.jacob at caviumnetworks.com > >> Subject: Re: [dpdk-dev] [PATCH v8 1/3] mempool: support external mempool > >> operations > >> > >> Hi Shreyansh, > >> > >> On 6/6/2016 3:38 PM, Shreyansh Jain wrote: > >>> Hi, > >>> > >>> (Apologies for overly-eager email sent on this thread earlier. Will be > more > >> careful in future). > >>> This is more of a question/clarification than a comment. (And I have > taken > >> only some snippets from original mail to keep it cleaner) > >>> <snip> > >>>> +MEMPOOL_REGISTER_OPS(ops_mp_mc); > >>>> +MEMPOOL_REGISTER_OPS(ops_sp_sc); > >>>> +MEMPOOL_REGISTER_OPS(ops_mp_sc); > >>>> +MEMPOOL_REGISTER_OPS(ops_sp_mc); > >>> <snip> > >>> > >>> From the above what I understand is that multiple packet pool handlers > can > >> be created. > >>> I have a use-case where application has multiple pools but only the > packet > >> pool is hardware backed. Using the hardware for general buffer > requirements > >> would prove costly. > >>> From what I understand from the patch, selection of the pool is based > on > >> the flags below. > >> > >> The flags are only used to select one of the default handlers for > >> backward compatibility through > >> the rte_mempool_create call. If you wish to use a mempool handler that > >> is not one of the > >> defaults, (i.e. a new hardware handler), you would use the > >> rte_create_mempool_empty > >> followed by the rte_mempool_set_ops_byname call. > >> So, for the external handlers, you create and empty mempool, then set > >> the operations (ops) > >> for that particular mempool. > > I am concerned about the existing applications (for example, l3fwd). > > Explicit calls to 'rte_create_mempool_empty->rte_mempool_set_ops_byname' > model would require modifications to these applications. > > Ideally, without any modifications, these applications should be able to > use packet pools (backed by hardware) and buffer pools (backed by > ring/others) - transparently. > > > > If I go by your suggestions, what I understand is, doing the above without > modification to applications would be equivalent to: > > > > struct rte_mempool_ops custom_hw_allocator = {...} > > > > thereafter, in config/common_base: > > > > CONFIG_RTE_DEFAULT_MEMPOOL_OPS="custom_hw_allocator" > > > > calls to rte_pktmbuf_pool_create would use the new allocator. > > Yes, correct. But only for calls to rte_pktmbuf_pool_create(). Calls to > rte_mempool_create will continue to use the default handlers (ring based).
Agree with you. But, some applications continue to use rte_mempool_create for allocating packet pools. Thus, even with a custom handler available (which, most probably, would be a hardware packet buffer handler), application would unintentionally end up not using it. Probably, such applications should be changed? (e.g. pipeline). > > But, another problem arises here. > > > > There are two distinct paths for allocations of a memory pool: > > 1. A 'pkt' pool: > > rte_pktmbuf_pool_create > > \- rte_mempool_create_empty > > | \- rte_mempool_set_ops_byname(..ring_mp_mc..) > > | > > `- rte_mempool_set_ops_byname > > (...RTE_MBUF_DEFAULT_MEMPOOL_OPS..) > > /* Override default 'ring_mp_mc' of > > * rte_mempool_create */ > > > > 2. Through generic mempool create API > > rte_mempool_create > > \- rte_mempool_create_empty > > (passing pktmbuf and pool constructors) > > > > I found various instances in example applications where > rte_mempool_create() is being called directly for packet pools - bypassing > the more semantically correct call to rte_pktmbuf_* for packet pools. > > > > In (2) control path, RTE_MBUF_DEFAULT_MEMPOOLS_OPS wouldn't be able to > replace custom handler operations for packet buffer allocations. > > > > From a performance point-of-view, Applications should be able to select > between packet pools and non-packet pools. > > This is intended for backward compatibility, and API consistency. Any > applications that use > rte_mempool_create directly will continue to use the default mempool > handlers. If the need > to use a custeom hander, they will need to be modified to call the newer > API, > rte_mempool_create_empty and rte_mempool_set_ops_byname. My understanding was that applications should be oblivious of how their pools are managed, except that they do understand packet pools should be faster (or accelerated) than non-packet pools. (Of course, some applications may be designed to explicitly take advantage of an available handler through rte_mempool_create_empty=>rte_mempool_set_ops_byname calls.) In that perspective, I was expecting that applications should be calling: -> rte_pktmbuf_* for all packet relation operations -> rte_mempool_* for non-packet or explicit hardware handlers And leave rest of the mempool handler related magic to DPDK framework. > > > >>> <snip> > >>>> + /* > >>>> + * Since we have 4 combinations of the SP/SC/MP/MC examine the > flags to > >>>> + * set the correct index into the table of ops structs. > >>>> + */ > >>>> + if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET)) > >>>> + rte_mempool_set_ops_byname(mp, "ring_sp_sc"); > >>>> + else if (flags & MEMPOOL_F_SP_PUT) > >>>> + rte_mempool_set_ops_byname(mp, "ring_sp_mc"); > >>>> + else if (flags & MEMPOOL_F_SC_GET) > >>>> + rte_mempool_set_ops_byname(mp, "ring_mp_sc"); > >>>> + else > >>>> + rte_mempool_set_ops_byname(mp, "ring_mp_mc"); > >>>> + > > My suggestion is to have an additional flag, 'MEMPOOL_F_PKT_ALLOC', which, > if specified, would: I read through some previous discussions and realized that something similar [1] had already been proposed earlier. I didn't want to hijack this thread with an old discussions - it was unintentional. [1] http://article.gmane.org/gmane.comp.networking.dpdk.devel/39803 But, [1] would make the distinction of *type* of pool and its corresponding handler, whether default or external/custom, quite clear. > > > > ... > > #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? > > Regards, > Dave. > - Shreyansh