[dpdk-dev] [PATCH v8 1/3] mempool: support external mempool operations

Jan Viktorin viktorin at rehivetech.com
Fri Jun 10 10:49:52 CEST 2016


On Fri, 10 Jun 2016 09:29:44 +0200
Olivier Matz <olivier.matz at 6wind.com> 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_<something>(..., 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


More information about the dev mailing list