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

Hunt, David david.hunt at intel.com
Fri Jun 10 11:34:38 CEST 2016


Hi all,

On 10/6/2016 8:29 AM, 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.
>
>> 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.
>
>
>> 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?
>
>
>> 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.

I would tend to agree, even though yesterday I proposed making that 
change. However,
thinking about it some more, I'm not totally happy with the
MEMPOOL_F_PKT_ALLOC addition. It adds yet another method of creating a 
mempool,
and I think that may introduce some confusion with some developers.

I also like the suggestion of rte_pktmbuf_pool_create_<something>(..., 
name) suggested
above, I was thinking the same myself last night, and I would prefer 
that rather than adding the
MEMPOOL_F_PKT_ALLOC flag. Developers can add that function into their 
apps as a wrapper
to rte_mempool_create_empty->rte_mempool_set_ops_byname should the need 
to have
more than one pktmbuf allocator. Otherwise they can use the one that 
makes use of the
RTE_MBUF_DEFAULT_MEMPOOL_OPS config setting.


> 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.
>
> Regards,
> Olivier



More information about the dev mailing list