Re: [PATCH v6 1/5] mempool: support external handler

["Hunt, David" <david.hunt@intel.com>]

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.