[dpdk-dev] [PATCH v6 1/5] mempool: support external handler
Hunt, David
david.hunt at intel.com
Thu Jun 2 13:23:41 CEST 2016
On 6/1/2016 6:54 PM, Jan Viktorin wrote:
>
> mp->populated_size--;
> @@ -383,13 +349,16 @@ rte_mempool_populate_phys(struct rte_mempool *mp, char *vaddr,
> unsigned i = 0;
> size_t off;
> struct rte_mempool_memhdr *memhdr;
> - int ret;
>
> /* create the internal ring if not already done */
> if ((mp->flags & MEMPOOL_F_RING_CREATED) == 0) {
> - ret = rte_mempool_ring_create(mp);
> - if (ret < 0)
> - return ret;
> + rte_errno = 0;
> + mp->pool = rte_mempool_ops_alloc(mp);
> + if (mp->pool == NULL) {
> + if (rte_errno == 0)
> + return -EINVAL;
> + return -rte_errno;
> Are you sure the rte_errno is a positive value?
If the user returns EINVAL, or similar, we want to return negative, as
per the rest of DPDK.
>> @@ -204,9 +205,13 @@ struct rte_mempool_memhdr {
>> struct rte_mempool {
>> char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
>> struct rte_ring *ring; /**< Ring to store objects. */
>> + union {
>> + void *pool; /**< Ring or pool to store objects */
> What about calling this pdata or priv? I think, it can improve some doc comments.
> Describing a "pool" may refer to both the rte_mempool itself or to the mp->pool
> pointer. The "priv" would help to understand the code a bit.
>
> Then the rte_mempool_alloc_t can be called rte_mempool_priv_alloc_t. Or something
> similar. It's than clear enough, what the function should do...
I'd lean towards pdata or maybe pool_data. Not sure about the function
name change though,
the function does return a pool_data pointer, which we put into
mp->pool_data.
>> + uint64_t *pool_id; /**< External mempool identifier */
> Why is pool_id a pointer? Is it a typo? I've understood it should be 64b wide
> from the discussion (Olivier, Jerin, David):
Yes, typo.
> uint32_t trailer_size; /**< Size of trailer (after elt). */
>
> unsigned private_data_size; /**< Size of private data. */
> + /**
> + * Index into rte_mempool_handler_table array of mempool handler ops
> s/rte_mempool_handler_table/rte_mempool_ops_table/
Done.
>> + * structs, which contain callback function pointers.
>> + * We're using an index here rather than pointers to the callbacks
>> + * to facilitate any secondary processes that may want to use
>> + * this mempool.
>> + */
>> + int32_t handler_idx;
> s/handler_idx/ops_idx/
>
> What about ops_index? Not a big deal...
I agree ops_index is better. Changed.
>>
>> struct rte_mempool_cache *local_cache; /**< Per-lcore local cache */
>>
>> @@ -325,6 +338,199 @@ void rte_mempool_check_cookies(const struct rte_mempool *mp,
>> #define __mempool_check_cookies(mp, obj_table_const, n, free) do {} while(0)
>> #endif /* RTE_LIBRTE_MEMPOOL_DEBUG */
>>
>> +#define RTE_MEMPOOL_HANDLER_NAMESIZE 32 /**< Max length of handler name. */
>> +
>> +/**
>> + * typedef for allocating the external pool.
> What about:
>
> function prototype to provide an implementation specific data
>
>> + * The handler's alloc function does whatever it needs to do to grab memory
>> + * for this handler, and sets the *pool opaque pointer in the rte_mempool
>> + * struct. In the default handler, *pool points to a ring,in other handlers,
> What about:
>
> The function should provide a memory heap representation or another private data
> used for allocation by the rte_mempool_ops. E.g. the default ops provides an
> instance of the rte_ring for this purpose.
>
>> + * it will mostlikely point to a different type of data structure.
>> + * It will be transparent to the application programmer.
> I'd add something like this:
>
> The function should not touch the given *mp instance.
Agreed. Reworked somewhat.
> ...because it's goal is NOT to set the mp->pool, this is done by the
> rte_mempool_populate_phys - the caller of this rte_mempool_ops_alloc.
>
> This is why I've suggested to pass the rte_mempool as const in the v5.
> Is there any reason to modify the rte_mempool contents by the implementation?
> I think, we just need to read the flags, socket_id, etc.
Yes, I agree it should be const. Changed.
>> + */
>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>> +
>> +/** Free the external pool opaque data (that pointed to by *pool) */
> What about:
>
> /** Free the opaque private data stored in the mp->pool pointer. */
I've merged the two versions of the comment:
/** Free the opaque private data pointed to by mp->pool_data pointer */
>> +typedef void (*rte_mempool_free_t)(void *p);
>> +
>> +/**
>> + * Put an object in the external pool.
>> + * The *p pointer is the opaque data for a given mempool handler (ring,
>> + * array, linked list, etc)
> The obj_table is not documented. Is it really a table? I'd called an array instead.
You're probably right, but it's always been called obj_table, and I'm
not sure
this patchset is the place to change it. Maybe a follow up patch?
>> + */
>> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);
> unsigned int
Done
>
>> +
>> +/** Get an object from the external pool. */
>> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n);
> unsigned int
Done
>
>> +
>> +/** Return the number of available objects in the external pool. */
> Is the number of available objects or the total number of all objects
> (so probably a constant value)?
It's intended to be the umber of available objects
>
>> +typedef unsigned (*rte_mempool_get_count)(void *p);
> Should it be const void *p?
Yes, it could be. Changed.
>
>> +
>> +/** Structure defining a mempool handler. */
> What about:
>
> /** Structure defining mempool operations. */
Changed.
>> +struct rte_mempool_ops {
>> + char name[RTE_MEMPOOL_HANDLER_NAMESIZE]; /**< Name of mempool handler */
> s/RTE_MEMPOOL_HANDLER_NAMESIZE/RTE_MEMPOOL_OPS_NAMESIZE/
Done
>> + rte_mempool_alloc_t alloc; /**< Allocate the external pool. */
> What about:
>
> Allocate the private data for this rte_mempool_ops.
Changed to /**< Allocate private data */
>> + rte_mempool_free_t free; /**< Free the external pool. */
>> + rte_mempool_put_t put; /**< Put an object. */
>> + rte_mempool_get_t get; /**< Get an object. */
>> + rte_mempool_get_count get_count; /**< Get the number of available objs. */
>> +} __rte_cache_aligned;
>> +
>> +#define RTE_MEMPOOL_MAX_HANDLER_IDX 16 /**< Max registered handlers */
> s/RTE_MEMPOOL_MAX_HANDLER_IDX/RTE_MEMPOOL_MAX_OPS_IDX/
Changed
>> +
>> +/**
>> + * Structure storing the table of registered handlers, each of which contain
>> + * the function pointers for the mempool handler functions.
>> + * Each process has it's own storage for this handler struct aray so that
>> + * the mempools can be shared across primary and secondary processes.
>> + * The indices used to access the array are valid across processes, whereas
>> + * any function pointers stored directly in the mempool struct would not be.
>> + * This results in us simply having "handler_idx" in the mempool struct.
>> + */
>> +struct rte_mempool_handler_table {
> s/rte_mempool_handler_table/rte_mempool_ops_table/
Done
>> + rte_spinlock_t sl; /**< Spinlock for add/delete. */
>> + uint32_t num_handlers; /**< Number of handlers in the table. */
> s/num_handlers/num_ops/
Done
>> + /**
>> + * Storage for all possible handlers.
>> + */
>> + struct rte_mempool_ops handler_ops[RTE_MEMPOOL_MAX_HANDLER_IDX];
> s/handler_ops/ops/
Done
>> +} __rte_cache_aligned;
>> +
>> +/** Array of registered handlers */
>> +extern struct rte_mempool_handler_table rte_mempool_handler_table;
> s/rte_mempool_handler_table/rte_mempool_ops_table/
Done
>> +
>> +/**
>> + * @internal Get the mempool handler from its index.
>> + *
>> + * @param handler_idx
>> + * The index of the handler in the handler table. It must be a valid
>> + * index: (0 <= idx < num_handlers).
>> + * @return
>> + * The pointer to the handler in the table.
>> + */
>> +static inline struct rte_mempool_ops *
>> +rte_mempool_handler_get(int handler_idx)
> rte_mempool_ops_get(int ops_idx)
Done
>> +{
>> + return &rte_mempool_handler_table.handler_ops[handler_idx];
>> +}
>> +
>> +/**
>> + * @internal wrapper for external mempool manager alloc callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @return
>> + * The opaque pointer to the external pool.
>> + */
>> +void *
>> +rte_mempool_ops_alloc(struct rte_mempool *mp);
>> +
>> +/**
>> + * @internal wrapper for external mempool manager get callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @param obj_table
>> + * Pointer to a table of void * pointers (objects).
>> + * @param n
>> + * Number of objects to get.
>> + * @return
>> + * - 0: Success; got n objects.
>> + * - <0: Error; code of handler get function.
>> + */
>> +static inline int
>> +rte_mempool_ops_dequeue_bulk(struct rte_mempool *mp,
>> + void **obj_table, unsigned n)
>> +{
>> + struct rte_mempool_ops *handler;
> *ops
Done
>> +
>> + handler = rte_mempool_handler_get(mp->handler_idx);
>> + return handler->get(mp->pool, obj_table, n);
>> +}
>> +
>> +/**
>> + * @internal wrapper for external mempool manager put callback.
>> + *
>> + * @param mp
>> + * Pointer to the memory pool.
>> + * @param obj_table
>> + * Pointer to a table of void * pointers (objects).
>> + * @param n
>> + * Number of objects to put.
>> + * @return
>> + * - 0: Success; n objects supplied.
>> + * - <0: Error; code of handler put function.
>> + */
>> +static inline int
>> +rte_mempool_ops_enqueue_bulk(struct rte_mempool *mp, void * const *obj_table,
>> + unsigned n)
>> +{
>> + struct rte_mempool_ops *handler;
> *ops
Done
>> + * @return
>> + * - 0: Sucess; the new handler is configured.
>> + * - EINVAL - Invalid handler name provided
>> + * - EEXIST - mempool already has a handler assigned
> They are returned as -EINVAL and -EEXIST.
>
> IMHO, using "-" here is counter-intuitive:
>
> - EINVAL
>
> does it mean a positive with "-" or negative value?
EINVAL is positive, so it's returning negative. Common usage in DPDK,
afaics.
>> + */
>> +int
>> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
> rte_mempool_set_ops
>
> What about rte_mempool_set_ops_byname? Not a big deal...
I agree. rte_mempool_set_ops_byname
>> +
>> +/**
>> + * Register an external pool handler.
> Register mempool operations
Done
>> + *
>> + * @param h
>> + * Pointer to the external pool handler
>> + * @return
>> + * - >=0: Sucess; return the index of the handler in the table.
>> + * - EINVAL - missing callbacks while registering handler
>> + * - ENOSPC - the maximum number of handlers has been reached
> - -EINVAL
> - -ENOSPC
:)
>> + */
>> +int rte_mempool_handler_register(const struct rte_mempool_ops *h);
> rte_mempool_ops_register
Done
>> +
>> +/**
>> + * Macro to statically register an external pool handler.
> What about adding:
>
> Note that the rte_mempool_ops_register fails silently here when
> more then RTE_MEMPOOL_MAX_OPS_IDX is registered.
Done
>> + */
>> +#define MEMPOOL_REGISTER_HANDLER(h)
> MEMPOOL_REGISTER_OPS
Done
> \
>> + void mp_hdlr_init_##h(void); \
>> + void __attribute__((constructor, used)) mp_hdlr_init_##h(void) \
>> + { \
>> + rte_mempool_handler_register(&h); \
>> + }
>> +
>> /**
>> * An object callback function for mempool.
>> *
>> @@ -774,7 +980,7 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
>> cache->len += n;
>>
>> if (cache->len >= flushthresh) {
>> - rte_ring_mp_enqueue_bulk(mp->ring, &cache->objs[cache_size],
>> + rte_mempool_ops_enqueue_bulk(mp, &cache->objs[cache_size],
>> cache->len - cache_size);
>> cache->len = cache_size;
>> }
>> @@ -785,19 +991,10 @@ ring_enqueue:
>>
>> /* push remaining objects in ring */
>> #ifdef RTE_LIBRTE_MEMPOOL_DEBUG
>> - if (is_mp) {
>> - if (rte_ring_mp_enqueue_bulk(mp->ring, obj_table, n) < 0)
>> - rte_panic("cannot put objects in mempool\n");
>> - }
>> - else {
>> - if (rte_ring_sp_enqueue_bulk(mp->ring, obj_table, n) < 0)
>> - rte_panic("cannot put objects in mempool\n");
>> - }
>> + if (rte_mempool_ops_enqueue_bulk(mp, obj_table, n) < 0)
>> + rte_panic("cannot put objects in mempool\n");
>> #else
>> - if (is_mp)
>> - rte_ring_mp_enqueue_bulk(mp->ring, obj_table, n);
>> - else
>> - rte_ring_sp_enqueue_bulk(mp->ring, obj_table, n);
>> + rte_mempool_ops_enqueue_bulk(mp, obj_table, n);
>> #endif
>> }
>>
>> @@ -922,7 +1119,7 @@ rte_mempool_put(struct rte_mempool *mp, void *obj)
>> */
>> static inline int __attribute__((always_inline))
>> __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
>> - unsigned n, int is_mc)
>> + unsigned n, __rte_unused int is_mc)
> unsigned int
Done
>> {
>> int ret;
>> struct rte_mempool_cache *cache;
>> @@ -945,7 +1142,8 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
>> uint32_t req = n + (cache_size - cache->len);
>>
>> /* How many do we require i.e. number to fill the cache + the request */
>> - ret = rte_ring_mc_dequeue_bulk(mp->ring, &cache->objs[cache->len], req);
>> + ret = rte_mempool_ops_dequeue_bulk(mp,
>> + &cache->objs[cache->len], req);
>> if (unlikely(ret < 0)) {
>> /*
>> * In the offchance that we are buffer constrained,
>> @@ -972,10 +1170,7 @@ __mempool_get_bulk(struct rte_mempool *mp, void **obj_table,
>> ring_dequeue:
>>
>> /* get remaining objects from ring */
>> - if (is_mc)
>> - ret = rte_ring_mc_dequeue_bulk(mp->ring, obj_table, n);
>> - else
>> - ret = rte_ring_sc_dequeue_bulk(mp->ring, obj_table, n);
>> + ret = rte_mempool_ops_dequeue_bulk(mp, obj_table, n);
>>
>> if (ret < 0)
>> __MEMPOOL_STAT_ADD(mp, get_fail, n);
>> diff --git a/lib/librte_mempool/rte_mempool_handler.c b/lib/librte_mempool/rte_mempool_handler.c
>> new file mode 100644
>> index 0000000..ed85d65
>> --- /dev/null
>> +++ b/lib/librte_mempool/rte_mempool_handler.c
> rte_mempool_ops.c
Done.
>
>> @@ -0,0 +1,141 @@
>> +/*-
>> + * BSD LICENSE
>> + *
>> + * Copyright(c) 2016 Intel Corporation. All rights reserved.
>> + * Copyright(c) 2016 6WIND S.A.
>> + * All rights reserved.
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * * Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * * Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in
>> + * the documentation and/or other materials provided with the
>> + * distribution.
>> + * * Neither the name of Intel Corporation nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <string.h>
>> +
>> +#include <rte_mempool.h>
>> +
>> +/* indirect jump table to support external memory pools */
>> +struct rte_mempool_handler_table rte_mempool_handler_table = {
> rte_mempool_ops_table
Done
>> + .sl = RTE_SPINLOCK_INITIALIZER ,
>> + .num_handlers = 0
> num_ops
Done
>> +};
>> +
>> +/* add a new handler in rte_mempool_handler_table, return its index */
>> +int
>> +rte_mempool_handler_register(const struct rte_mempool_ops *h)
>> +{
>> + struct rte_mempool_ops *handler;
> *ops
Done
>> + int16_t handler_idx;
> ops_idx
Done
>> +
>> + rte_spinlock_lock(&rte_mempool_handler_table.sl);
>> +
>> + if (rte_mempool_handler_table.num_handlers >=
>> + RTE_MEMPOOL_MAX_HANDLER_IDX) {
>> + rte_spinlock_unlock(&rte_mempool_handler_table.sl);
>> + RTE_LOG(ERR, MEMPOOL,
>> + "Maximum number of mempool handlers exceeded\n");
>> + return -ENOSPC;
>> + }
>> +
>> + if (h->put == NULL || h->get == NULL || h->get_count == NULL) {
>> + rte_spinlock_unlock(&rte_mempool_handler_table.sl);
>> + RTE_LOG(ERR, MEMPOOL,
>> + "Missing callback while registering mempool handler\n");
>> + return -EINVAL;
>> + }
>> +
>> + handler_idx = rte_mempool_handler_table.num_handlers++;
>> + handler = &rte_mempool_handler_table.handler_ops[handler_idx];
>> + snprintf(handler->name, sizeof(handler->name), "%s", h->name);
>> + handler->alloc = h->alloc;
>> + handler->put = h->put;
>> + handler->get = h->get;
>> + handler->get_count = h->get_count;
>> +
>> + rte_spinlock_unlock(&rte_mempool_handler_table.sl);
>> +
>> + return handler_idx;
>> +}
>> +
>> +/* wrapper to allocate an external pool handler */
>> +void *
>> +rte_mempool_ops_alloc(struct rte_mempool *mp)
>> +{
>> + struct rte_mempool_ops *handler;
> *ops
Done
>> +
>> + handler = rte_mempool_handler_get(mp->handler_idx);
>> + if (handler->alloc == NULL)
>> + return NULL;
>> + return handler->alloc(mp);
>> +}
>> +
>> +/* wrapper to free an external pool handler */
>> +void
>> +rte_mempool_ops_free(struct rte_mempool *mp)
>> +{
>> + struct rte_mempool_ops *handler;
> *ops
Done
>> +
>> + handler = rte_mempool_handler_get(mp->handler_idx);
>> + if (handler->free == NULL)
>> + return;
>> + return handler->free(mp);
>> +}
>> +
>> +/* wrapper to get available objects in an external pool handler */
>> +unsigned int
>> +rte_mempool_ops_get_count(const struct rte_mempool *mp)
>> +{
>> + struct rte_mempool_ops *handler;
> *ops
Done
>> +
>> + handler = rte_mempool_handler_get(mp->handler_idx);
>> + return handler->get_count(mp->pool);
>> +}
>> +
>> +/* sets a handler previously registered by rte_mempool_handler_register */
>> +int
>> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name)
>> +{
>> + struct rte_mempool_ops *handler = NULL;
> *ops
Done
>> + unsigned i;
>> +
>> + /* too late, the mempool is already populated */
>> + if (mp->flags & MEMPOOL_F_RING_CREATED)
>> + return -EEXIST;
>> +
>> + for (i = 0; i < rte_mempool_handler_table.num_handlers; i++) {
>> + if (!strcmp(name,
>> + rte_mempool_handler_table.handler_ops[i].name)) {
>> + handler = &rte_mempool_handler_table.handler_ops[i];
>> + break;
>> + }
>> + }
>> +
>> + if (handler == NULL)
>> + return -EINVAL;
>> +
>> + mp->handler_idx = i;
>> + return 0;
>> +}
> Regards
> Jan
Thanks,
Dave.
More information about the dev
mailing list