[dpdk-dev] [PATCH 1/5] mempool: add external mempool manager support

Olivier MATZ olivier.matz at 6wind.com
Fri Mar 4 10:05:21 CET 2016


Hi David,

>>
>>> @@ -622,6 +607,10 @@ rte_mempool_xmem_create(const char *name,
>>> unsigned n, unsigned elt_size,
>>>
>>>       mp->elt_va_end = mp->elt_va_start;
>>>
>>> +    /* Parameters are setup. Call the mempool handler alloc */
>>> +    if ((rte_mempool_ext_alloc(mp, name, n, socket_id, flags)) == NULL)
>>> +        goto exit;
>>> +
>>
>> I think some memory needs to be freed here. At least 'te'.
> 
> Done in v2

Please note that in the meanwhile, this fix has been pushed (as we need
it for next release):
http://dpdk.org/browse/dpdk/commit/lib/librte_mempool/rte_mempool.c?id=86f36ff9578b5f3d697c8fcf6072dcb70e2b246f


>>> diff --git a/lib/librte_mempool/rte_mempool.h
>>> b/lib/librte_mempool/rte_mempool.h
>>> index 6e2390a..620cfb7 100644
>>> --- a/lib/librte_mempool/rte_mempool.h
>>> +++ b/lib/librte_mempool/rte_mempool.h
>>> @@ -88,6 +88,8 @@ extern "C" {
>>>   struct rte_mempool_debug_stats {
>>>       uint64_t put_bulk;         /**< Number of puts. */
>>>       uint64_t put_objs;         /**< Number of objects successfully
>>> put. */
>>> +    uint64_t put_pool_bulk;    /**< Number of puts into pool. */
>>> +    uint64_t put_pool_objs;    /**< Number of objects into pool. */
>>>       uint64_t get_success_bulk; /**< Successful allocation number. */
>>>       uint64_t get_success_objs; /**< Objects successfully allocated. */
>>>       uint64_t get_fail_bulk;    /**< Failed allocation number. */
>>
>> I think the comment of put_pool_objs is not very clear.
>> Shouldn't we have the same stats for get?
>>
> 
> Not used, removed. Covered by put_bulk.

I guess you mean it will be removed in v3? It is still there in the v2
(the field, not the comment that has been fixed).

Shouldn't we have the same stats for get?


>>>   /**
>>>    * The RTE mempool structure.
>>>    */
>>>   struct rte_mempool {
>>>       char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
>>> -    struct rte_ring *ring;           /**< Ring to store objects. */
>>>       phys_addr_t phys_addr;           /**< Phys. addr. of mempool
>>> struct. */
>>>       int flags;                       /**< Flags of the mempool. */
>>>       uint32_t size;                   /**< Size of the mempool. */
>>> @@ -194,6 +270,11 @@ struct rte_mempool {
>>>
>>>       unsigned private_data_size;      /**< Size of private data. */
>>>
>>> +    /* Common pool data structure pointer */
>>> +    void *rt_pool __rte_cache_aligned;
>>
>> What is the meaning of rt_pool?
> 
> I agree that it's probably not a very good name. Since it's basically
> the pointer which is used by the handlers callbacks, maybe we should
> call it mempool_storage? That leaves it generic enough that it can point
> at a ring, an array, or whatever else is needed for a particular handler.

My question was more about the "rt_" prefix. Maybe I missed something
obvious? I think "pool" or "pool_handler" is ok.


>>> @@ -769,8 +853,7 @@ __mempool_put_bulk(struct rte_mempool *mp, void *
>>> const *obj_table,
>>>
>>>   #if RTE_MEMPOOL_CACHE_MAX_SIZE > 0
>>>       /* cache is not enabled or single producer or non-EAL thread */
>>> -    if (unlikely(cache_size == 0 || is_mp == 0 ||
>>> -             lcore_id >= RTE_MAX_LCORE))
>>> +    if (unlikely(cache_size == 0 || lcore_id >= RTE_MAX_LCORE))
>>>           goto ring_enqueue;
>>>
>>>       /* Go straight to ring if put would overflow mem allocated for
>>> cache */
>>
>> If I understand well, we now always use the cache, even if the mempool
>> is single-producer. I was wondering if it would have a performance
>> impact... I suppose that using the cache is more efficient than the ring
>> in single-producer mode, so it may increase performance. Do you have an
>> idea of the impact here?
> 
> I've seen very little in performance gain, maybe a couple of percent for
> some tests, and up to 10% drop for some single core tests. I'll do some
> more specific testing based on SP versus MP.

OK thanks!


>>> diff --git a/lib/librte_mempool/rte_mempool_default.c
>>> b/lib/librte_mempool/rte_mempool_default.c
>>> new file mode 100644
>>> index 0000000..2493dc1
>>> --- /dev/null
>>> +++ b/lib/librte_mempool/rte_mempool_default.c
>>> +#include "rte_mempool_internal.h"
>>> +
>>> +/*
>>> + * Indirect jump table to support external memory pools
>>> + */
>>> +struct rte_mempool_handler_list mempool_handler_list = {
>>> +    .sl =  RTE_SPINLOCK_INITIALIZER ,
>>> +    .num_handlers = 0
>>> +};
>>> +
>>> +/* TODO Convert to older mechanism of an array of stucts */
>>> +int16_t
>>> +add_handler(struct rte_mempool_handler *h)
>>> +{
>>> +    int16_t handler_idx;
>>> +
>>> +    /*  */
>>> +    rte_spinlock_lock(&mempool_handler_list.sl);
>>> +
>>> +    /* Check whether jump table has space */
>>> +    if (mempool_handler_list.num_handlers >=
>>> RTE_MEMPOOL_MAX_HANDLER_IDX) {
>>> +        rte_spinlock_unlock(&mempool_handler_list.sl);
>>> +        RTE_LOG(ERR, MEMPOOL,
>>> +                "Maximum number of mempool handlers exceeded\n");
>>> +        return -1;
>>> +    }
>>> +
>>> +    if ((h->put == NULL) || (h->get == NULL) ||
>>> +        (h->get_count == NULL)) {
>>> +        rte_spinlock_unlock(&mempool_handler_list.sl);
>>> +         RTE_LOG(ERR, MEMPOOL,
>>> +                    "Missing callback while registering mempool
>>> handler\n");
>>> +        return -1;
>>> +    }
>>> +
>>> +    /* add new handler index */
>>> +    handler_idx = mempool_handler_list.num_handlers++;
>>> +
>>> +    snprintf(mempool_handler_list.handler[handler_idx].name,
>>> +                RTE_MEMPOOL_NAMESIZE, "%s", h->name);
>>> +    mempool_handler_list.handler[handler_idx].alloc = h->alloc;
>>> +    mempool_handler_list.handler[handler_idx].put = h->put;
>>> +    mempool_handler_list.handler[handler_idx].get = h->get;
>>> +    mempool_handler_list.handler[handler_idx].get_count = h->get_count;
>>> +
>>> +    rte_spinlock_unlock(&mempool_handler_list.sl);
>>> +
>>> +    return handler_idx;
>>> +}
>>
>> Why not using a similar mechanism than what we have for PMDs?
>>
>>     void rte_eal_driver_register(struct rte_driver *driver)
>>     {
>>         TAILQ_INSERT_TAIL(&dev_driver_list, driver, next);
>>     }
>>
>> To do that, you just need to add a TAILQ_ENTRY() in your
>> rte_mempool_handler structure. This would avoid to duplicate the
>> structure into a static array whose size is limited.
>>
>> Accessing to the callbacks would be easier:
>>
>>     return mp->mp_handler->put(mp->rt_pool, obj_table, n);
>>
>> instead of:
>>
>>     return (mempool_handler_list.handler[mp->handler_idx].put)
>>                     (mp->rt_pool, obj_table, n);
>>
>> If we really want to copy the handlers somewhere, it could be in
>> the mempool structure. It would avoid an extra dereference
>> (note the first '.' instead of '->'):
>>
>>     return mp.mp_handler->put(mp->rt_pool, obj_table, n);
>>
>> After doing that, we could ask ourself if the wrappers are still
>> useful or not. I would have say that they could be removed.
>>
>>
>> The spinlock could be kept, although it may look a bit overkill:
>> - I don't expect to have several loading at the same time
>> - There is no unregister() function, so there is no risk to
>>   browse the list atomically
>>
> 
> Already discussed previously, index needed over pointer because of
> secondary processes.

Could you add a comment stating this? It may help for next readers
to have this info.

>> Last thing, I think this code should go in rte_mempool.c, not in
>> rte_mempool_default.c.
> 
> I was trying to keep the default handlers together in their own file,
> rather than having them in with the mempool framework. I think it's
> better having them separate, and new handlers can go in their own files
> also. no?

OK for the following functions:
 common_ring_mp_put()
 common_ring_sp_put()
 common_ring_mc_get()
 common_ring_sc_get()
 common_ring_get_count()
 rte_mempool_common_ring_alloc()  (note: only this one has
       a rte_mempool prefix, maybe it should be fixed)

The other functions are part of the framework to add an external
handler, I don't think they should go in rte_mempool_default.c
They could either go in rte_mempool.c or in another file
rte_mempool_handler.c.


>>> --- /dev/null
>>> +++ b/lib/librte_mempool/rte_mempool_internal.h
>>
>> Is it the proper name?
>> We could imagine a mempool handler provided by a plugin, and
>> in this case this code should go in rte_mempool.h.
> 
> I was trying to keep the public APIs in rte_mempool.h, and aal the
> private stuff in rte_mempool_internal.h. Maybe a better name would be
> rte_mempool_private.h?

Are these functions internal? I mean, is it possible for an application
or an external PMD (.so) to provide its own handler? I think it would
be really interesting to have this capability.

Then, I would prefer to have this either in rte_mempool.h or in
rte_mempool_handler.h (that would be coherent with the .c)

>>> +
>>> +struct rte_mempool_handler {
>>> +    char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool handler */
>>
>> I would use a const char * here instead.
>>
> 
> Would we then have to allocate the memory for the string elsewhere? I
> would have thought this is the more straightforward method.

My initial question was, can we just have something like this:

	// my_handler.c
	static struct rte_mempool_handler handler_stack = {
		.name = "my_handler",
		.alloc = my_alloc,
		...
	};

	// rte_mempool_handler.c
	int16_t
	rte_mempool_register_handler(struct rte_mempool_handler *h)
	{
		...
		handler->name = h->name; /* instead of snprintf */
		...
	}

But it won't be possible as the structures will be shared with
a secondary process. So the name[RTE_MEMPOOL_NAMESIZE] is fine.



Regards,
Olivier


More information about the dev mailing list