[dpdk-dev,v5,1/3] mempool: support external handler

  Until now, the objects stored in mempool mempool were internally stored a
ring. This patch introduce the possibility to register external handlers
replacing the ring.

The default behavior remains unchanged, but calling the new function
rte_mempool_set_handler() right after rte_mempool_create_empty() allows to
change the handler that will be used when populating the mempool.

v5 changes: rebasing on top of 35 patch set mempool work.

Signed-off-by: David Hunt <david.hunt@intel.com>
Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 app/test/test_mempool_perf.c               |   1 -
 lib/librte_mempool/Makefile                |   2 +
 lib/librte_mempool/rte_mempool.c           |  73 ++++------
 lib/librte_mempool/rte_mempool.h           | 212 +++++++++++++++++++++++++----
 lib/librte_mempool/rte_mempool_default.c   | 147 ++++++++++++++++++++
 lib/librte_mempool/rte_mempool_handler.c   | 139 +++++++++++++++++++
 lib/librte_mempool/rte_mempool_version.map |   4 +
 7 files changed, 506 insertions(+), 72 deletions(-)
 create mode 100644 lib/librte_mempool/rte_mempool_default.c
 create mode 100644 lib/librte_mempool/rte_mempool_handler.c
  

Hello David,

please, see my comments inline.

I didn't see the previous versions of the mempool (well, only very roughly) so I am
probably missing some points... My point of view is as a user of the handler API.
I need to understand the API to implement a custom handler for my purposes.

On Thu, 19 May 2016 14:44:59 +0100
David Hunt <david.hunt@intel.com> wrote:

> Until now, the objects stored in mempool mempool were internally stored a

s/mempool mempool/mempool/

stored _in_ a ring?

> ring. This patch introduce the possibility to register external handlers
> replacing the ring.
> 
> The default behavior remains unchanged, but calling the new function
> rte_mempool_set_handler() right after rte_mempool_create_empty() allows to
> change the handler that will be used when populating the mempool.
> 
> v5 changes: rebasing on top of 35 patch set mempool work.
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> ---
> app/test/test_mempool_perf.c               |   1 -
>  lib/librte_mempool/Makefile                |   2 +
>  lib/librte_mempool/rte_mempool.c           |  73 ++++------
>  lib/librte_mempool/rte_mempool.h           | 212 +++++++++++++++++++++++++----
>  lib/librte_mempool/rte_mempool_default.c   | 147 ++++++++++++++++++++
>  lib/librte_mempool/rte_mempool_handler.c   | 139 +++++++++++++++++++
>  lib/librte_mempool/rte_mempool_version.map |   4 +
>  7 files changed, 506 insertions(+), 72 deletions(-)
>  create mode 100644 lib/librte_mempool/rte_mempool_default.c
>  create mode 100644 lib/librte_mempool/rte_mempool_handler.c
> 
> diff --git a/app/test/test_mempool_perf.c b/app/test/test_mempool_perf.c
> index cdc02a0..091c1df 100644
> --- a/app/test/test_mempool_perf.c
> +++ b/app/test/test_mempool_perf.c
> @@ -161,7 +161,6 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg)
>  							   n_get_bulk);
>  				if (unlikely(ret < 0)) {
>  					rte_mempool_dump(stdout, mp);
> -					rte_ring_dump(stdout, mp->ring);
>  					/* in this case, objects are lost... */
>  					return -1;
>  				}

I think, this should be in a separate patch explaining the reason to remove it.

> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
> index 43423e0..f19366e 100644
> --- a/lib/librte_mempool/Makefile
> +++ b/lib/librte_mempool/Makefile
> @@ -42,6 +42,8 @@ LIBABIVER := 2
>  
>  # all source are stored in SRCS-y
>  SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_handler.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_default.c
>  # install includes
>  SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>  
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 1ab6701..6ec2b3f 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj, phys_addr_t physaddr)
>  #endif
>  
>  	/* enqueue in ring */
> -	rte_ring_sp_enqueue(mp->ring, obj);
> +	rte_mempool_ext_put_bulk(mp, &obj, 1);

I suppose this is OK, however, replacing "enqueue" by "put" (semantically) sounds to me
like a bug. Enqueue is put into a queue. Put is to drop a reference.

>  }
>  
>  /* call obj_cb() for each mempool element */
> @@ -300,40 +300,6 @@ rte_mempool_xmem_usage(__rte_unused void *vaddr, uint32_t elt_num,
>  	return (size_t)paddr_idx << pg_shift;
>  }
>  
> -/* create the internal ring */
> -static int
> -rte_mempool_ring_create(struct rte_mempool *mp)
> -{
> -	int rg_flags = 0, ret;
> -	char rg_name[RTE_RING_NAMESIZE];
> -	struct rte_ring *r;
> -
> -	ret = snprintf(rg_name, sizeof(rg_name),
> -		RTE_MEMPOOL_MZ_FORMAT, mp->name);
> -	if (ret < 0 || ret >= (int)sizeof(rg_name))
> -		return -ENAMETOOLONG;
> -
> -	/* ring flags */
> -	if (mp->flags & MEMPOOL_F_SP_PUT)
> -		rg_flags |= RING_F_SP_ENQ;
> -	if (mp->flags & MEMPOOL_F_SC_GET)
> -		rg_flags |= RING_F_SC_DEQ;
> -
> -	/* Allocate the ring that will be used to store objects.
> -	 * Ring functions will return appropriate errors if we are
> -	 * running as a secondary process etc., so no checks made
> -	 * in this function for that condition.
> -	 */
> -	r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
> -		mp->socket_id, rg_flags);
> -	if (r == NULL)
> -		return -rte_errno;
> -
> -	mp->ring = r;
> -	mp->flags |= MEMPOOL_F_RING_CREATED;
> -	return 0;
> -}

This is a big change. I suggest (if possible) to make a separate patch with
something like "replace rte_mempool_ring_create by ...". Where is this code
placed now?

> -
>  /* free a memchunk allocated with rte_memzone_reserve() */
>  static void
>  rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr *memhdr,
> @@ -351,7 +317,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
>  	void *elt;
>  
>  	while (!STAILQ_EMPTY(&mp->elt_list)) {
> -		rte_ring_sc_dequeue(mp->ring, &elt);
> +		rte_mempool_ext_get_bulk(mp, &elt, 1);

Similar as for put_bulk... Replacing "dequeue" by "get" (semantically) sounds to me
like a bug. Dequeue is drop from a queue. Get is to obtain a reference.

>  		(void)elt;
>  		STAILQ_REMOVE_HEAD(&mp->elt_list, next);
>  		mp->populated_size--;
> @@ -380,15 +346,18 @@ 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_ext_alloc(mp);
> +		if (mp->pool == NULL) {
> +			if (rte_errno == 0)
> +				return -EINVAL;
> +			else
> +				return -rte_errno;
> +		}
>  	}
> -

Is this a whitespace change?

>  	/* mempool is already populated */
>  	if (mp->populated_size >= mp->size)
>  		return -ENOSPC;
> @@ -700,7 +669,7 @@ rte_mempool_free(struct rte_mempool *mp)
>  	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>  
>  	rte_mempool_free_memchunks(mp);
> -	rte_ring_free(mp->ring);
> +	rte_mempool_ext_free(mp);
>  	rte_memzone_free(mp->mz);
>  }
>  
> @@ -812,6 +781,20 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
>  		RTE_PTR_ADD(mp, MEMPOOL_HEADER_SIZE(mp, 0));
>  
>  	te->data = mp;
> +
> +	/*
> +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
> +	 * set the correct index into the handler table.
> +	 */
> +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
> +		rte_mempool_set_handler(mp, "ring_sp_sc");
> +	else if (flags & MEMPOOL_F_SP_PUT)
> +		rte_mempool_set_handler(mp, "ring_sp_mc");
> +	else if (flags & MEMPOOL_F_SC_GET)
> +		rte_mempool_set_handler(mp, "ring_mp_sc");
> +	else
> +		rte_mempool_set_handler(mp, "ring_mp_mc");
> +

Do I understand it well that this code preserves behaviour of the previous API?
Because otherwise it looks strange.

>  	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
>  	TAILQ_INSERT_TAIL(mempool_list, te, next);
>  	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> @@ -927,7 +910,7 @@ rte_mempool_count(const struct rte_mempool *mp)
>  	unsigned count;
>  	unsigned lcore_id;
>  
> -	count = rte_ring_count(mp->ring);
> +	count = rte_mempool_ext_get_count(mp);
>  
>  	if (mp->cache_size == 0)
>  		return count;
> @@ -1120,7 +1103,7 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  
>  	fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>  	fprintf(f, "  flags=%x\n", mp->flags);
> -	fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> +	fprintf(f, "  pool=%p\n", mp->pool);
>  	fprintf(f, "  phys_addr=0x%" PRIx64 "\n", mp->mz->phys_addr);
>  	fprintf(f, "  nb_mem_chunks=%u\n", mp->nb_mem_chunks);
>  	fprintf(f, "  size=%"PRIu32"\n", mp->size);
> @@ -1141,7 +1124,7 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	}
>  
>  	cache_count = rte_mempool_dump_cache(f, mp);
> -	common_count = rte_ring_count(mp->ring);
> +	common_count = rte_mempool_ext_get_count(mp);
>  	if ((cache_count + common_count) > mp->size)
>  		common_count = mp->size - cache_count;
>  	fprintf(f, "  common_pool_count=%u\n", common_count);
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 60339bd..ed2c110 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -67,6 +67,7 @@
>  #include <inttypes.h>
>  #include <sys/queue.h>
>  
> +#include <rte_spinlock.h>
>  #include <rte_log.h>
>  #include <rte_debug.h>
>  #include <rte_lcore.h>
> @@ -203,7 +204,15 @@ struct rte_mempool_memhdr {
>   */
>  struct rte_mempool {
>  	char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
> -	struct rte_ring *ring;           /**< Ring to store objects. */
> +	void *pool;                      /**< Ring or ext-pool to store objects. */
> +	/**
> +	 * Index into the array of structs containing callback fn 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. Any function pointers stored in the mempool
> +	 * directly would not be valid for secondary processes.
> +	 */

I think, this comment should go to the rte_mempool_handler_table definition
leaving a here a short note about it.

> +	int32_t handler_idx;
>  	const struct rte_memzone *mz;    /**< Memzone where pool is allocated */
>  	int flags;                       /**< Flags of the mempool. */
>  	int socket_id;                   /**< Socket id passed at mempool creation. */
> @@ -325,6 +334,175 @@ 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. */
> +
> +/** Allocate the external pool. */

What is the purpose of this callback?
What exactly does it allocate?
Some rte_mempool internals?
Or the memory?
What does it return?

> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
> +
> +/** Free the external pool. */

Why this *_free callback does not accept the rte_mempool param?

> +typedef void (*rte_mempool_free_t)(void *p);
> +
> +/** Put an object in the external pool. */

What is the *p pointer?
What is the obj_table?
Why is it void *?
Why is it const?

> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);

Probably, "unsigned int n" is better.

> +
> +/** Get an object from the external pool. */
> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n);

Probably, "unsigned int n" is better.

> +
> +/** Return the number of available objects in the external pool. */

What is the purpose of the *_get_count callback? I guess it can introduce
race conditions...

> +typedef unsigned (*rte_mempool_get_count)(void *p);

unsigned int

> +
> +/** Structure defining a mempool handler. */

Later in the text, I suggested to rename rte_mempool_handler to rte_mempool_ops.
I believe that it explains the purpose of this struct better. It would improve
consistency in function names (the *_ext_* mark is very strange and inconsistent).

> +struct rte_mempool_handler {
> +	char name[RTE_MEMPOOL_HANDLER_NAMESIZE]; /**< Name of mempool handler */
> +	rte_mempool_alloc_t alloc;       /**< Allocate the external pool. */
> +	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 number of registered handlers */
> +
> +/** Structure storing the table of registered handlers. */
> +struct rte_mempool_handler_table {
> +	rte_spinlock_t sl;     /**< Spinlock for add/delete. */
> +	uint32_t num_handlers; /**< Number of handlers in the table. */
> +	/** Storage for all possible handlers. */
> +	struct rte_mempool_handler handler[RTE_MEMPOOL_MAX_HANDLER_IDX];
> +};

The handlers are implemented as an array due to multi-process access.
Is it correct? I'd expect a note about it here.

> +
> +/** Array of registered handlers */
> +extern struct rte_mempool_handler_table rte_mempool_handler_table;
> +
> +/**
> + * @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 struct rte_mempool_handler *
> +rte_mempool_handler_get(int handler_idx)
> +{
> +	return &rte_mempool_handler_table.handler[handler_idx];

Is it always safe? Can we belive the handler_idx is inside the boundaries?
At least some RTE_VERIFY would be nice here...

> +}
> +
> +/**
> + * @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_ext_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.

Should this doc be more specific about the possible failures?

> + */
> +static inline int
> +rte_mempool_ext_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	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.

Should this doc be more specific about the possible failures?

> + */
> +static inline int
> +rte_mempool_ext_put_bulk(struct rte_mempool *mp, void * const *obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	handler = rte_mempool_handler_get(mp->handler_idx);
> +	return handler->put(mp->pool, obj_table, n);
> +}
> +
> +/**
> + * @internal wrapper for external mempool manager get_count callback.
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @return
> + *   The number of available objects in the external pool.
> + */
> +unsigned

unsigned int

> +rte_mempool_ext_get_count(const struct rte_mempool *mp);
> +
> +/**
> + * @internal wrapper for external mempool manager free callback.
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + */
> +void
> +rte_mempool_ext_free(struct rte_mempool *mp);
> +
> +/**
> + * Set the handler of a mempool
> + *
> + * This can only be done on a mempool that is not populated, i.e. just after
> + * a call to rte_mempool_create_empty().
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @param name
> + *   Name of the handler.
> + * @return
> + *   - 0: Sucess; the new handler is configured.
> + *   - <0: Error (errno)

Should this doc be more specific about the possible failures?

The body of rte_mempool_set_handler does not set errno at all.
It returns e.g. -EEXIST.

> + */
> +int
> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
> +
> +/**
> + * Register an external pool handler.
> + *
> + * @param h
> + *   Pointer to the external pool handler
> + * @return
> + *   - >=0: Sucess; return the index of the handler in the table.
> + *   - <0: Error (errno)

Should this doc be more specific about the possible failures?

> + */
> +int rte_mempool_handler_register(struct rte_mempool_handler *h);
> +
> +/**
> + * Macro to statically register an external pool handler.
> + */
> +#define MEMPOOL_REGISTER_HANDLER(h)					\
> +	void mp_hdlr_init_##h(void);					\
> +	void __attribute__((constructor, used)) mp_hdlr_init_##h(void)	\
> +	{								\
> +		rte_mempool_handler_register(&h);			\
> +	}
> +

There might be a little catch. If there is no more room for handlers, calling the
rte_mempool_handler_register would fail silently as the error reporting does not
work when calling a constructor (or at least, this is my experience).

Not a big deal but...

>  /**
>   * An object callback function for mempool.
>   *
> @@ -736,7 +914,7 @@ void rte_mempool_dump(FILE *f, struct rte_mempool *mp);
>   */
>  static inline void __attribute__((always_inline))
>  __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
> -		    unsigned n, int is_mp)
> +		    unsigned n, __rte_unused int is_mp)
>  {
>  	struct rte_mempool_cache *cache;
>  	uint32_t index;
> @@ -774,7 +952,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_ext_put_bulk(mp, &cache->objs[cache_size],
>  				cache->len - cache_size);
>  		cache->len = cache_size;
>  	}
> @@ -782,26 +960,10 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
>  	return;
>  
>  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");
> -	}
> -#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);
> -#endif
> +	rte_mempool_ext_put_bulk(mp, obj_table, n);

This is a big change. Does it remove the RTE_LIBRTE_MEMPOOL_DEBUG config option
entirely? If so, I suggest to first do this in a separated patch and then
replace the original *_enqueue_bulk by your *_ext_put_bulk (or better *_ops_put_bulk
as I explain below).

>  }
>  
> -
>  /**
>   * Put several objects back in the mempool (multi-producers safe).
>   *
> @@ -922,7 +1084,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)
>  {
>  	int ret;
>  	struct rte_mempool_cache *cache;
> @@ -945,7 +1107,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_ext_get_bulk(mp,
> +			&cache->objs[cache->len], req);
>  		if (unlikely(ret < 0)) {
>  			/*
>  			 * In the offchance that we are buffer constrained,
> @@ -972,10 +1135,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_ext_get_bulk(mp, obj_table, n);
>  
>  	if (ret < 0)
>  		__MEMPOOL_STAT_ADD(mp, get_fail, n);
> diff --git a/lib/librte_mempool/rte_mempool_default.c b/lib/librte_mempool/rte_mempool_default.c
> new file mode 100644
> index 0000000..a6ac65a
> --- /dev/null
> +++ b/lib/librte_mempool/rte_mempool_default.c
> @@ -0,0 +1,147 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> + *   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_errno.h>
> +#include <rte_ring.h>
> +#include <rte_mempool.h>
> +
> +static int
> +common_ring_mp_put(void *p, void * const *obj_table, unsigned n)
> +{
> +	return rte_ring_mp_enqueue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static int
> +common_ring_sp_put(void *p, void * const *obj_table, unsigned n)
> +{
> +	return rte_ring_sp_enqueue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static int
> +common_ring_mc_get(void *p, void **obj_table, unsigned n)
> +{
> +	return rte_ring_mc_dequeue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static int
> +common_ring_sc_get(void *p, void **obj_table, unsigned n)
> +{
> +	return rte_ring_sc_dequeue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static unsigned
> +common_ring_get_count(void *p)
> +{
> +	return rte_ring_count((struct rte_ring *)p);
> +}
> +
> +
> +static void *
> +common_ring_alloc(struct rte_mempool *mp)
> +{
> +	int rg_flags = 0, ret;
> +	char rg_name[RTE_RING_NAMESIZE];
> +	struct rte_ring *r;
> +
> +	ret = snprintf(rg_name, sizeof(rg_name),
> +		RTE_MEMPOOL_MZ_FORMAT, mp->name);
> +	if (ret < 0 || ret >= (int)sizeof(rg_name)) {
> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}
> +
> +	/* ring flags */
> +	if (mp->flags & MEMPOOL_F_SP_PUT)
> +		rg_flags |= RING_F_SP_ENQ;
> +	if (mp->flags & MEMPOOL_F_SC_GET)
> +		rg_flags |= RING_F_SC_DEQ;
> +
> +	/* Allocate the ring that will be used to store objects.
> +	 * Ring functions will return appropriate errors if we are
> +	 * running as a secondary process etc., so no checks made
> +	 * in this function for that condition. */
> +	r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
> +		mp->socket_id, rg_flags);
> +
> +	return r;
> +}
> +
> +static void
> +common_ring_free(void *p)
> +{
> +	rte_ring_free((struct rte_ring *)p);
> +}
> +
> +static struct rte_mempool_handler handler_mp_mc = {
> +	.name = "ring_mp_mc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_mp_put,
> +	.get = common_ring_mc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +static struct rte_mempool_handler handler_sp_sc = {
> +	.name = "ring_sp_sc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_sp_put,
> +	.get = common_ring_sc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +static struct rte_mempool_handler handler_mp_sc = {
> +	.name = "ring_mp_sc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_mp_put,
> +	.get = common_ring_sc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +static struct rte_mempool_handler handler_sp_mc = {
> +	.name = "ring_sp_mc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_sp_put,
> +	.get = common_ring_mc_get,
> +	.get_count = common_ring_get_count,
> +};
> +

Introducing those handlers can go as a separate patch. IMHO, that would simplify
the review process a lot. First introduce the mechanism, then add something
inside.

I'd also note that those handlers are always available and what kind of memory
do they use...

> +MEMPOOL_REGISTER_HANDLER(handler_mp_mc);
> +MEMPOOL_REGISTER_HANDLER(handler_sp_sc);
> +MEMPOOL_REGISTER_HANDLER(handler_mp_sc);
> +MEMPOOL_REGISTER_HANDLER(handler_sp_mc);
> diff --git a/lib/librte_mempool/rte_mempool_handler.c b/lib/librte_mempool/rte_mempool_handler.c
> new file mode 100644
> index 0000000..78611f8
> --- /dev/null
> +++ b/lib/librte_mempool/rte_mempool_handler.c
> @@ -0,0 +1,139 @@
> +/*-
> + *   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 = {
> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
> +	.num_handlers = 0
> +};
> +
> +/* add a new handler in rte_mempool_handler_table, return its index */

It seems to me that there is no way how to put some opaque pointer into the
handler. In such case I would expect I can do something like:

struct my_handler {
	struct rte_mempool_handler h;
	...
} handler;

rte_mempool_handler_register(&handler.h);

But I cannot because you copy the contents of the handler. By the way, this
should be documented.

How can I pass an opaque pointer here? The only way I see is through the
rte_mempool.pool. In that case, what about renaming the rte_mempool_handler
to rte_mempool_ops? Because semantically, it is not a handler, it just holds
the operations.

This would improve some namings:

rte_mempool_ext_alloc -> rte_mempool_ops_alloc
rte_mempool_ext_free -> rte_mempool_ops_free
rte_mempool_ext_get_count -> rte_mempool_ops_get_count
rte_mempool_handler_register -> rte_mempool_ops_register

seems to be more readable to me. The *_ext_* mark does not say anything valuable.
It just scares a bit :).

> +int
> +rte_mempool_handler_register(struct rte_mempool_handler *h)
> +{
> +	struct rte_mempool_handler *handler;
> +	int16_t handler_idx;
> +
> +	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[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_ext_alloc(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	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_ext_free(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	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
> +rte_mempool_ext_get_count(const struct rte_mempool *mp)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	handler = rte_mempool_handler_get(mp->handler_idx);
> +	return handler->get_count(mp->pool);
> +}
> +
> +/* set the handler of a mempool */

The doc comment should say "this sets a handler previously registered by
the rte_mempool_handler_register function ...". I was confused and didn't
understand how the handlers are inserted into the table.

> +int
> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name)
> +{
> +	struct rte_mempool_handler *handler = NULL;
> +	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[i].name)) {
> +			handler = &rte_mempool_handler_table.handler[i];
> +			break;
> +		}
> +	}
> +
> +	if (handler == NULL)
> +		return -EINVAL;
> +
> +	mp->handler_idx = i;
> +	return 0;
> +}
> diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> index f63461b..a0e9aed 100644
> --- a/lib/librte_mempool/rte_mempool_version.map
> +++ b/lib/librte_mempool/rte_mempool_version.map
> @@ -19,6 +19,8 @@ DPDK_2.0 {
>  DPDK_16.7 {
>  	global:
>  
> +	rte_mempool_handler_table;
> +
>  	rte_mempool_check_cookies;
>  	rte_mempool_obj_iter;
>  	rte_mempool_mem_iter;
> @@ -29,6 +31,8 @@ DPDK_16.7 {
>  	rte_mempool_populate_default;
>  	rte_mempool_populate_anon;
>  	rte_mempool_free;
> +	rte_mempool_set_handler;
> +	rte_mempool_handler_register;
>  
>  	local: *;
>  } DPDK_2.0;

Regards
Jan
  

On 5/23/2016 1:35 PM, Jan Viktorin wrote:
> Hello David,
>
> please, see my comments inline.
>
> I didn't see the previous versions of the mempool (well, only very roughly) so I am
> probably missing some points... My point of view is as a user of the handler API.
> I need to understand the API to implement a custom handler for my purposes.

Thanks for the review, Jan.

I'm working on the changes now, will post soon. I'll reply to each of 
you're emails when I'm ready with the patch.

Regards,
David.
  

On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote:
> Until now, the objects stored in mempool mempool were internally stored a
> ring. This patch introduce the possibility to register external handlers
> replacing the ring.
> 
> The default behavior remains unchanged, but calling the new function
> rte_mempool_set_handler() right after rte_mempool_create_empty() allows to
> change the handler that will be used when populating the mempool.
> 
> v5 changes: rebasing on top of 35 patch set mempool work.
> 
> Signed-off-by: David Hunt <david.hunt@intel.com>
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  app/test/test_mempool_perf.c               |   1 -
>  lib/librte_mempool/Makefile                |   2 +
>  lib/librte_mempool/rte_mempool.c           |  73 ++++------
>  lib/librte_mempool/rte_mempool.h           | 212 +++++++++++++++++++++++++----
>  lib/librte_mempool/rte_mempool_default.c   | 147 ++++++++++++++++++++
>  lib/librte_mempool/rte_mempool_handler.c   | 139 +++++++++++++++++++
>  lib/librte_mempool/rte_mempool_version.map |   4 +
>  7 files changed, 506 insertions(+), 72 deletions(-)
>  create mode 100644 lib/librte_mempool/rte_mempool_default.c
>  create mode 100644 lib/librte_mempool/rte_mempool_handler.c
> 
> diff --git a/app/test/test_mempool_perf.c b/app/test/test_mempool_perf.c
> index cdc02a0..091c1df 100644
> --- a/app/test/test_mempool_perf.c
> +++ b/app/test/test_mempool_perf.c
> @@ -161,7 +161,6 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg)
>  							   n_get_bulk);
>  				if (unlikely(ret < 0)) {
>  					rte_mempool_dump(stdout, mp);
> -					rte_ring_dump(stdout, mp->ring);
>  					/* in this case, objects are lost... */
>  					return -1;
>  				}
> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
> index 43423e0..f19366e 100644
> --- a/lib/librte_mempool/Makefile
> +++ b/lib/librte_mempool/Makefile
> @@ -42,6 +42,8 @@ LIBABIVER := 2
>  
>  # all source are stored in SRCS-y
>  SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_handler.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_default.c
>  # install includes
>  SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>  
> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
> index 1ab6701..6ec2b3f 100644
> --- a/lib/librte_mempool/rte_mempool.c
> +++ b/lib/librte_mempool/rte_mempool.c
> @@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj, phys_addr_t physaddr)
>  #endif
>  
>  	/* enqueue in ring */
> -	rte_ring_sp_enqueue(mp->ring, obj);
> +	rte_mempool_ext_put_bulk(mp, &obj, 1);
>  }
>  
>  /* call obj_cb() for each mempool element */
> @@ -300,40 +300,6 @@ rte_mempool_xmem_usage(__rte_unused void *vaddr, uint32_t elt_num,
>  	return (size_t)paddr_idx << pg_shift;
>  }
>  
> -/* create the internal ring */
> -static int
> -rte_mempool_ring_create(struct rte_mempool *mp)
> -{
> -	int rg_flags = 0, ret;
> -	char rg_name[RTE_RING_NAMESIZE];
> -	struct rte_ring *r;
> -
> -	ret = snprintf(rg_name, sizeof(rg_name),
> -		RTE_MEMPOOL_MZ_FORMAT, mp->name);
> -	if (ret < 0 || ret >= (int)sizeof(rg_name))
> -		return -ENAMETOOLONG;
> -
> -	/* ring flags */
> -	if (mp->flags & MEMPOOL_F_SP_PUT)
> -		rg_flags |= RING_F_SP_ENQ;
> -	if (mp->flags & MEMPOOL_F_SC_GET)
> -		rg_flags |= RING_F_SC_DEQ;
> -
> -	/* Allocate the ring that will be used to store objects.
> -	 * Ring functions will return appropriate errors if we are
> -	 * running as a secondary process etc., so no checks made
> -	 * in this function for that condition.
> -	 */
> -	r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
> -		mp->socket_id, rg_flags);
> -	if (r == NULL)
> -		return -rte_errno;
> -
> -	mp->ring = r;
> -	mp->flags |= MEMPOOL_F_RING_CREATED;
> -	return 0;
> -}
> -
>  /* free a memchunk allocated with rte_memzone_reserve() */
>  static void
>  rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr *memhdr,
> @@ -351,7 +317,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
>  	void *elt;
>  
>  	while (!STAILQ_EMPTY(&mp->elt_list)) {
> -		rte_ring_sc_dequeue(mp->ring, &elt);
> +		rte_mempool_ext_get_bulk(mp, &elt, 1);
>  		(void)elt;
>  		STAILQ_REMOVE_HEAD(&mp->elt_list, next);
>  		mp->populated_size--;
> @@ -380,15 +346,18 @@ 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_ext_alloc(mp);
> +		if (mp->pool == NULL) {
> +			if (rte_errno == 0)
> +				return -EINVAL;
> +			else
> +				return -rte_errno;
> +		}
>  	}
> -
>  	/* mempool is already populated */
>  	if (mp->populated_size >= mp->size)
>  		return -ENOSPC;
> @@ -700,7 +669,7 @@ rte_mempool_free(struct rte_mempool *mp)
>  	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
>  
>  	rte_mempool_free_memchunks(mp);
> -	rte_ring_free(mp->ring);
> +	rte_mempool_ext_free(mp);
>  	rte_memzone_free(mp->mz);
>  }
>  
> @@ -812,6 +781,20 @@ rte_mempool_create_empty(const char *name, unsigned n, unsigned elt_size,
>  		RTE_PTR_ADD(mp, MEMPOOL_HEADER_SIZE(mp, 0));
>  
>  	te->data = mp;
> +
> +	/*
> +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
> +	 * set the correct index into the handler table.
> +	 */
> +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
> +		rte_mempool_set_handler(mp, "ring_sp_sc");
> +	else if (flags & MEMPOOL_F_SP_PUT)
> +		rte_mempool_set_handler(mp, "ring_sp_mc");
> +	else if (flags & MEMPOOL_F_SC_GET)
> +		rte_mempool_set_handler(mp, "ring_mp_sc");
> +	else
> +		rte_mempool_set_handler(mp, "ring_mp_mc");

IMO, We should decouple the implementation specific flags of _a_
external pool manager implementation from the generic rte_mempool_create_empty
function as going further when we introduce new flags for custom HW accelerated
external pool manager then this common code will be bloated.

> +
>  	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
>  	TAILQ_INSERT_TAIL(mempool_list, te, next);
>  	rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> @@ -927,7 +910,7 @@ rte_mempool_count(const struct rte_mempool *mp)
>  	unsigned count;
>  	unsigned lcore_id;
>  
> -	count = rte_ring_count(mp->ring);
> +	count = rte_mempool_ext_get_count(mp);
>  
>  	if (mp->cache_size == 0)
>  		return count;
> @@ -1120,7 +1103,7 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  
>  	fprintf(f, "mempool <%s>@%p\n", mp->name, mp);
>  	fprintf(f, "  flags=%x\n", mp->flags);
> -	fprintf(f, "  ring=<%s>@%p\n", mp->ring->name, mp->ring);
> +	fprintf(f, "  pool=%p\n", mp->pool);
>  	fprintf(f, "  phys_addr=0x%" PRIx64 "\n", mp->mz->phys_addr);
>  	fprintf(f, "  nb_mem_chunks=%u\n", mp->nb_mem_chunks);
>  	fprintf(f, "  size=%"PRIu32"\n", mp->size);
> @@ -1141,7 +1124,7 @@ rte_mempool_dump(FILE *f, struct rte_mempool *mp)
>  	}
>  
>  	cache_count = rte_mempool_dump_cache(f, mp);
> -	common_count = rte_ring_count(mp->ring);
> +	common_count = rte_mempool_ext_get_count(mp);
>  	if ((cache_count + common_count) > mp->size)
>  		common_count = mp->size - cache_count;
>  	fprintf(f, "  common_pool_count=%u\n", common_count);
> diff --git a/lib/librte_mempool/rte_mempool.h b/lib/librte_mempool/rte_mempool.h
> index 60339bd..ed2c110 100644
> --- a/lib/librte_mempool/rte_mempool.h
> +++ b/lib/librte_mempool/rte_mempool.h
> @@ -67,6 +67,7 @@
>  #include <inttypes.h>
>  #include <sys/queue.h>
>  
> +#include <rte_spinlock.h>
>  #include <rte_log.h>
>  #include <rte_debug.h>
>  #include <rte_lcore.h>
> @@ -203,7 +204,15 @@ struct rte_mempool_memhdr {
>   */
>  struct rte_mempool {
>  	char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
> -	struct rte_ring *ring;           /**< Ring to store objects. */
> +	void *pool;                      /**< Ring or ext-pool to store objects. */
> +	/**
> +	 * Index into the array of structs containing callback fn 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. Any function pointers stored in the mempool
> +	 * directly would not be valid for secondary processes.
> +	 */
> +	int32_t handler_idx;
>  	const struct rte_memzone *mz;    /**< Memzone where pool is allocated */
>  	int flags;                       /**< Flags of the mempool. */
>  	int socket_id;                   /**< Socket id passed at mempool creation. */
> @@ -325,6 +334,175 @@ 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. */
> +
> +/** Allocate the external pool. */
> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
> +
> +/** Free the external pool. */
> +typedef void (*rte_mempool_free_t)(void *p);
> +
> +/** Put an object in the external pool. */
> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);
> +
> +/** Get an object from the external pool. */
> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n);
> +
> +/** Return the number of available objects in the external pool. */
> +typedef unsigned (*rte_mempool_get_count)(void *p);
> +
> +/** Structure defining a mempool handler. */
> +struct rte_mempool_handler {
> +	char name[RTE_MEMPOOL_HANDLER_NAMESIZE]; /**< Name of mempool handler */
> +	rte_mempool_alloc_t alloc;       /**< Allocate the external pool. */
> +	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 number of registered handlers */
> +
> +/** Structure storing the table of registered handlers. */
> +struct rte_mempool_handler_table {
> +	rte_spinlock_t sl;     /**< Spinlock for add/delete. */
> +	uint32_t num_handlers; /**< Number of handlers in the table. */
> +	/** Storage for all possible handlers. */
> +	struct rte_mempool_handler handler[RTE_MEMPOOL_MAX_HANDLER_IDX];
> +};

add __rte_cache_aligned to this structure to avoid "handler" memory
cacheline being shared with other variables

> +
> +/** Array of registered handlers */
> +extern struct rte_mempool_handler_table rte_mempool_handler_table;
> +
> +/**
> + * @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 struct rte_mempool_handler *

inline?

> +rte_mempool_handler_get(int handler_idx)
> +{
> +	return &rte_mempool_handler_table.handler[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_ext_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_ext_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	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_ext_put_bulk(struct rte_mempool *mp, void * const *obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	handler = rte_mempool_handler_get(mp->handler_idx);
> +	return handler->put(mp->pool, obj_table, n);
> +}
> +
> +/**
> + * @internal wrapper for external mempool manager get_count callback.
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @return
> + *   The number of available objects in the external pool.
> + */
> +unsigned
> +rte_mempool_ext_get_count(const struct rte_mempool *mp);
> +
> +/**
> + * @internal wrapper for external mempool manager free callback.
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + */
> +void
> +rte_mempool_ext_free(struct rte_mempool *mp);
> +
> +/**
> + * Set the handler of a mempool
> + *
> + * This can only be done on a mempool that is not populated, i.e. just after
> + * a call to rte_mempool_create_empty().
> + *
> + * @param mp
> + *   Pointer to the memory pool.
> + * @param name
> + *   Name of the handler.
> + * @return
> + *   - 0: Sucess; the new handler is configured.
> + *   - <0: Error (errno)
> + */
> +int
> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
> +
> +/**
> + * Register an external pool handler.
> + *
> + * @param h
> + *   Pointer to the external pool handler
> + * @return
> + *   - >=0: Sucess; return the index of the handler in the table.
> + *   - <0: Error (errno)
> + */
> +int rte_mempool_handler_register(struct rte_mempool_handler *h);
> +
> +/**
> + * Macro to statically register an external pool handler.
> + */
> +#define MEMPOOL_REGISTER_HANDLER(h)					\
> +	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.
>   *
> @@ -736,7 +914,7 @@ void rte_mempool_dump(FILE *f, struct rte_mempool *mp);
>   */
>  static inline void __attribute__((always_inline))
>  __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
> -		    unsigned n, int is_mp)
> +		    unsigned n, __rte_unused int is_mp)
>  {
>  	struct rte_mempool_cache *cache;
>  	uint32_t index;
> @@ -774,7 +952,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_ext_put_bulk(mp, &cache->objs[cache_size],
>  				cache->len - cache_size);
>  		cache->len = cache_size;
>  	}
> @@ -782,26 +960,10 @@ __mempool_put_bulk(struct rte_mempool *mp, void * const *obj_table,
>  	return;
>  
>  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");
> -	}
> -#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);
> -#endif
> +	rte_mempool_ext_put_bulk(mp, obj_table, n);
>  }
>  
> -
>  /**
>   * Put several objects back in the mempool (multi-producers safe).
>   *
> @@ -922,7 +1084,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)
>  {
>  	int ret;
>  	struct rte_mempool_cache *cache;
> @@ -945,7 +1107,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_ext_get_bulk(mp,

This makes inline function to a function pointer. Nothing wrong in
that. However, Do you see any performance drop with "local cache" only
use case?

http://dpdk.org/dev/patchwork/patch/12993/

> +			&cache->objs[cache->len], req);
>  		if (unlikely(ret < 0)) {
>  			/*
>  			 * In the offchance that we are buffer constrained,
> @@ -972,10 +1135,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_ext_get_bulk(mp, obj_table, n);
>  
>  	if (ret < 0)
>  		__MEMPOOL_STAT_ADD(mp, get_fail, n);
> diff --git a/lib/librte_mempool/rte_mempool_default.c b/lib/librte_mempool/rte_mempool_default.c
> new file mode 100644
> index 0000000..a6ac65a
> --- /dev/null
> +++ b/lib/librte_mempool/rte_mempool_default.c
> @@ -0,0 +1,147 @@
> +/*-
> + *   BSD LICENSE
> + *
> + *   Copyright(c) 2010-2016 Intel Corporation. All rights reserved.
> + *   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_errno.h>
> +#include <rte_ring.h>
> +#include <rte_mempool.h>
> +
> +static int
> +common_ring_mp_put(void *p, void * const *obj_table, unsigned n)
> +{
> +	return rte_ring_mp_enqueue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static int
> +common_ring_sp_put(void *p, void * const *obj_table, unsigned n)
> +{
> +	return rte_ring_sp_enqueue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static int
> +common_ring_mc_get(void *p, void **obj_table, unsigned n)
> +{
> +	return rte_ring_mc_dequeue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static int
> +common_ring_sc_get(void *p, void **obj_table, unsigned n)
> +{
> +	return rte_ring_sc_dequeue_bulk((struct rte_ring *)p, obj_table, n);
> +}
> +
> +static unsigned
> +common_ring_get_count(void *p)
> +{
> +	return rte_ring_count((struct rte_ring *)p);
> +}
> +
> +
> +static void *
> +common_ring_alloc(struct rte_mempool *mp)
> +{
> +	int rg_flags = 0, ret;
> +	char rg_name[RTE_RING_NAMESIZE];
> +	struct rte_ring *r;
> +
> +	ret = snprintf(rg_name, sizeof(rg_name),
> +		RTE_MEMPOOL_MZ_FORMAT, mp->name);
> +	if (ret < 0 || ret >= (int)sizeof(rg_name)) {
> +		rte_errno = ENAMETOOLONG;
> +		return NULL;
> +	}
> +
> +	/* ring flags */
> +	if (mp->flags & MEMPOOL_F_SP_PUT)
> +		rg_flags |= RING_F_SP_ENQ;
> +	if (mp->flags & MEMPOOL_F_SC_GET)
> +		rg_flags |= RING_F_SC_DEQ;
> +
> +	/* Allocate the ring that will be used to store objects.
> +	 * Ring functions will return appropriate errors if we are
> +	 * running as a secondary process etc., so no checks made
> +	 * in this function for that condition. */
> +	r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
> +		mp->socket_id, rg_flags);
> +
> +	return r;
> +}
> +
> +static void
> +common_ring_free(void *p)
> +{
> +	rte_ring_free((struct rte_ring *)p);
> +}
> +
> +static struct rte_mempool_handler handler_mp_mc = {
> +	.name = "ring_mp_mc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_mp_put,
> +	.get = common_ring_mc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +static struct rte_mempool_handler handler_sp_sc = {
> +	.name = "ring_sp_sc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_sp_put,
> +	.get = common_ring_sc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +static struct rte_mempool_handler handler_mp_sc = {
> +	.name = "ring_mp_sc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_mp_put,
> +	.get = common_ring_sc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +static struct rte_mempool_handler handler_sp_mc = {
> +	.name = "ring_sp_mc",
> +	.alloc = common_ring_alloc,
> +	.free = common_ring_free,
> +	.put = common_ring_sp_put,
> +	.get = common_ring_mc_get,
> +	.get_count = common_ring_get_count,
> +};
> +
> +MEMPOOL_REGISTER_HANDLER(handler_mp_mc);
> +MEMPOOL_REGISTER_HANDLER(handler_sp_sc);
> +MEMPOOL_REGISTER_HANDLER(handler_mp_sc);
> +MEMPOOL_REGISTER_HANDLER(handler_sp_mc);
> diff --git a/lib/librte_mempool/rte_mempool_handler.c b/lib/librte_mempool/rte_mempool_handler.c
> new file mode 100644
> index 0000000..78611f8
> --- /dev/null
> +++ b/lib/librte_mempool/rte_mempool_handler.c
> @@ -0,0 +1,139 @@
> +/*-
> + *   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 = {
> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
> +	.num_handlers = 0
> +};
> +
> +/* add a new handler in rte_mempool_handler_table, return its index */
> +int
> +rte_mempool_handler_register(struct rte_mempool_handler *h)
> +{
> +	struct rte_mempool_handler *handler;
> +	int16_t handler_idx;
> +
> +	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[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_ext_alloc(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	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_ext_free(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	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
> +rte_mempool_ext_get_count(const struct rte_mempool *mp)
> +{
> +	struct rte_mempool_handler *handler;
> +
> +	handler = rte_mempool_handler_get(mp->handler_idx);
> +	return handler->get_count(mp->pool);
> +}
> +
> +/* set the handler of a mempool */
> +int
> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name)
> +{
> +	struct rte_mempool_handler *handler = NULL;
> +	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[i].name)) {
> +			handler = &rte_mempool_handler_table.handler[i];
> +			break;
> +		}
> +	}
> +
> +	if (handler == NULL)
> +		return -EINVAL;
> +
> +	mp->handler_idx = i;
> +	return 0;
> +}
> diff --git a/lib/librte_mempool/rte_mempool_version.map b/lib/librte_mempool/rte_mempool_version.map
> index f63461b..a0e9aed 100644
> --- a/lib/librte_mempool/rte_mempool_version.map
> +++ b/lib/librte_mempool/rte_mempool_version.map
> @@ -19,6 +19,8 @@ DPDK_2.0 {
>  DPDK_16.7 {
>  	global:
>  
> +	rte_mempool_handler_table;
> +
>  	rte_mempool_check_cookies;
>  	rte_mempool_obj_iter;
>  	rte_mempool_mem_iter;
> @@ -29,6 +31,8 @@ DPDK_16.7 {
>  	rte_mempool_populate_default;
>  	rte_mempool_populate_anon;
>  	rte_mempool_free;
> +	rte_mempool_set_handler;
> +	rte_mempool_handler_register;
>  
>  	local: *;
>  } DPDK_2.0;
> -- 
> 2.5.5
>
  

On 5/24/2016 4:35 PM, Jerin Jacob wrote:
> On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote:
>> +	/*
>> +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
>> +	 * set the correct index into the handler table.
>> +	 */
>> +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
>> +		rte_mempool_set_handler(mp, "ring_sp_sc");
>> +	else if (flags & MEMPOOL_F_SP_PUT)
>> +		rte_mempool_set_handler(mp, "ring_sp_mc");
>> +	else if (flags & MEMPOOL_F_SC_GET)
>> +		rte_mempool_set_handler(mp, "ring_mp_sc");
>> +	else
>> +		rte_mempool_set_handler(mp, "ring_mp_mc");
> IMO, We should decouple the implementation specific flags of _a_
> external pool manager implementation from the generic rte_mempool_create_empty
> function as going further when we introduce new flags for custom HW accelerated
> external pool manager then this common code will be bloated.

These flags are only there to maintain backward compatibility for the 
default handlers. I would not
envisage adding more flags to this, I would suggest just adding a new 
handler using the new API calls.
So I would not see this code growing much in the future.


>> +/** Structure storing the table of registered handlers. */
>> +struct rte_mempool_handler_table {
>> +	rte_spinlock_t sl;     /**< Spinlock for add/delete. */
>> +	uint32_t num_handlers; /**< Number of handlers in the table. */
>> +	/** Storage for all possible handlers. */
>> +	struct rte_mempool_handler handler[RTE_MEMPOOL_MAX_HANDLER_IDX];
>> +};
> add __rte_cache_aligned to this structure to avoid "handler" memory
> cacheline being shared with other variables

Will do.

>> +
>> +/** Array of registered handlers */
>> +extern struct rte_mempool_handler_table rte_mempool_handler_table;
>> +
>> +/**
>> + * @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 struct rte_mempool_handler *
> inline?

Will do.

>>   		/* 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_ext_get_bulk(mp,
> This makes inline function to a function pointer. Nothing wrong in
> that. However, Do you see any performance drop with "local cache" only
> use case?
>
> http://dpdk.org/dev/patchwork/patch/12993/

With the latest mempool manager patch (without 12933), I see no 
performance degradation on my Haswell machine.
However, when I apply patch 12933, I'm seeing a 200-300 kpps drop.

With 12933, the mempool_perf_autotest is showing 24% more 
enqueues/dequeues, but testpmd forwarding
traffic between 2 40Gig interfaces from a hardware traffic generator  
with one core doing the forwarding
is showing a drop of 200-300kpps.

Regards,
Dave.



---snip---
  

On Fri, May 27, 2016 at 10:52:42AM +0100, Hunt, David wrote:
> 
> 
> On 5/24/2016 4:35 PM, Jerin Jacob wrote:
> > On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote:
> > > +	/*
> > > +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
> > > +	 * set the correct index into the handler table.
> > > +	 */
> > > +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
> > > +		rte_mempool_set_handler(mp, "ring_sp_sc");
> > > +	else if (flags & MEMPOOL_F_SP_PUT)
> > > +		rte_mempool_set_handler(mp, "ring_sp_mc");
> > > +	else if (flags & MEMPOOL_F_SC_GET)
> > > +		rte_mempool_set_handler(mp, "ring_mp_sc");
> > > +	else
> > > +		rte_mempool_set_handler(mp, "ring_mp_mc");
> > IMO, We should decouple the implementation specific flags of _a_
> > external pool manager implementation from the generic rte_mempool_create_empty
> > function as going further when we introduce new flags for custom HW accelerated
> > external pool manager then this common code will be bloated.
> 
> These flags are only there to maintain backward compatibility for the
> default handlers. I would not
> envisage adding more flags to this, I would suggest just adding a new
> handler using the new API calls.
> So I would not see this code growing much in the future.

IMHO, For _each_ HW accelerated external pool manager we may need to introduce
specific flag to tune to specific use cases.i.e MEMPOOL_F_* flags for
this exiting pool manager implemented in SW. For instance, when we add
a new HW external pool manager we may need to add MEMPOOL_MYHW_DONT_FREE_ON_SEND
(just a random name) to achieve certain functionally.

So I propose let "unsigned flags" in mempool create to be the opaque type and each
external pool manager can define what it makes sense to that specific
pool manager as there is NO other means to configure the pool manager.

For instance, on HW accelerated pool manager, the flag MEMPOOL_F_SP_PUT may
not make much sense as it can work with MP without any additional
settings in HW.

So instead of adding these checks in common code, IMO, lets move this
to a pool manager specific "function pointer" function and invoke
the function pointer from generic mempool create function.

What do you think?

Jerin
  

On 5/27/2016 11:33 AM, Jerin Jacob wrote:
> On Fri, May 27, 2016 at 10:52:42AM +0100, Hunt, David wrote:
>>
>> On 5/24/2016 4:35 PM, Jerin Jacob wrote:
>>> On Thu, May 19, 2016 at 02:44:59PM +0100, David Hunt wrote:
>>>> +	/*
>>>> +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
>>>> +	 * set the correct index into the handler table.
>>>> +	 */
>>>> +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
>>>> +		rte_mempool_set_handler(mp, "ring_sp_sc");
>>>> +	else if (flags & MEMPOOL_F_SP_PUT)
>>>> +		rte_mempool_set_handler(mp, "ring_sp_mc");
>>>> +	else if (flags & MEMPOOL_F_SC_GET)
>>>> +		rte_mempool_set_handler(mp, "ring_mp_sc");
>>>> +	else
>>>> +		rte_mempool_set_handler(mp, "ring_mp_mc");
>>> IMO, We should decouple the implementation specific flags of _a_
>>> external pool manager implementation from the generic rte_mempool_create_empty
>>> function as going further when we introduce new flags for custom HW accelerated
>>> external pool manager then this common code will be bloated.
>> These flags are only there to maintain backward compatibility for the
>> default handlers. I would not
>> envisage adding more flags to this, I would suggest just adding a new
>> handler using the new API calls.
>> So I would not see this code growing much in the future.
> IMHO, For _each_ HW accelerated external pool manager we may need to introduce
> specific flag to tune to specific use cases.i.e MEMPOOL_F_* flags for
> this exiting pool manager implemented in SW. For instance, when we add
> a new HW external pool manager we may need to add MEMPOOL_MYHW_DONT_FREE_ON_SEND
> (just a random name) to achieve certain functionally.
>
> So I propose let "unsigned flags" in mempool create to be the opaque type and each
> external pool manager can define what it makes sense to that specific
> pool manager as there is NO other means to configure the pool manager.
>
> For instance, on HW accelerated pool manager, the flag MEMPOOL_F_SP_PUT may
> not make much sense as it can work with MP without any additional
> settings in HW.
>
> So instead of adding these checks in common code, IMO, lets move this
> to a pool manager specific "function pointer" function and invoke
> the function pointer from generic mempool create function.
>
> What do you think?
>
> Jerin

Jerin,
      That chunk of code above would be better moved all right. I'd 
suggest moving it to the
rte_mempool_create function, as that's the one that needs the backward 
compatibility.

On the flags issue, each mempool handler can re-interpret the flags as 
needed. Maybe we
could use the upper half of the bits for different handlers, changing 
the meaning of the
bits depending on which handler is being set up. We can then keep the lower
half for bits that are common across all handlers? That way the user can 
just set the bits they
are interested in for that handler. Also, the alloc function has access 
to the flags, so maybe the
handler specific setup could be handled in the alloc function rather 
than adding a new function pointer?

Of course, that won't help if we need to pass in more data, in which 
case we'd probably need an
opaque data pointer somewhere. It would probably be most useful to pass 
it in with the
alloc, which may need the data. Any suggestions?

Regards,
Dave.
  

On Fri, May 27, 2016 at 03:44:31PM +0100, Hunt, David wrote:
> 
> 
Hi David,
[snip]
>      That chunk of code above would be better moved all right. I'd suggest
> moving it to the
> rte_mempool_create function, as that's the one that needs the backward
> compatibility.

OK

> 
> On the flags issue, each mempool handler can re-interpret the flags as
> needed. Maybe we
> could use the upper half of the bits for different handlers, changing the
> meaning of the
> bits depending on which handler is being set up. We can then keep the lower
> half for bits that are common across all handlers? That way the user can

Common lower half bit in flags looks good.

> just set the bits they
> are interested in for that handler. Also, the alloc function has access to
> the flags, so maybe the
> handler specific setup could be handled in the alloc function rather than
> adding a new function pointer?

Yes. I agree.

> 
> Of course, that won't help if we need to pass in more data, in which case
> we'd probably need an
> opaque data pointer somewhere. It would probably be most useful to pass it
> in with the
> alloc, which may need the data. Any suggestions?

But the top level rte_mempool_create() function needs to pass the data. Right?
That would be an ABI change. IMO, we need to start thinking about
passing a struct of config data to rte_mempool_create to create
backward compatibility on new argument addition to rte_mempool_create()

Other points in HW assisted pool manager perspective,

1) May be RING can be replaced with some other higher abstraction name
for the internal MEMPOOL_F_RING_CREATED flag
2) IMO, It is better to change void *pool in struct rte_mempool to
anonymous union type, something like below, so that mempool
implementation can choose the best type.
	union {
		void *pool;
		uint64_t val;
	}

3) int32_t handler_idx creates 4 byte hole in struct rte_mempool in
64 bit system. IMO it better to rearrange.(as const struct rte_memzone
*mz comes next)

4) IMO, It is better to change ext_alloc/ext_free to ext_create/ext_destroy
as their is no allocation in HW assisted pool manager case,
it will be mostly creating some HW initialization.

> 
> Regards,
> Dave.
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>
  

On 5/30/2016 10:41 AM, Jerin Jacob wrote:
--snip--
>> Of course, that won't help if we need to pass in more data, in which case
>> we'd probably need an
>> opaque data pointer somewhere. It would probably be most useful to pass it
>> in with the
>> alloc, which may need the data. Any suggestions?
> But the top level rte_mempool_create() function needs to pass the data. Right?
> That would be an ABI change. IMO, we need to start thinking about
> passing a struct of config data to rte_mempool_create to create
> backward compatibility on new argument addition to rte_mempool_create()

New mempool handlers will use rte_mempool_create_empty(), 
rte_mempool_set_handler(),
then rte_mempool_populate_*(). These three functions are new to this 
release, to no problem
to add a parameter to one of them for the config data. Also since we're 
adding some new
elements to the mempool structure, how about we add a new pointer for a 
void pointer to a
config data structure, as defined by the handler.

So, new element in rte_mempool struct alongside the *pool
     void *pool;
     void *pool_config;

Then add a param to the rte_mempool_set_handler function:
int
rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void 
*pool_config)

The function would simply set the pointer in the mempool struct, and the 
custom handler
alloc/create function would use as apporopriate as needed. Handlers that 
do not need this
data can be passed NULL.


> Other points in HW assisted pool manager perspective,
>
> 1) May be RING can be replaced with some other higher abstraction name
> for the internal MEMPOOL_F_RING_CREATED flag

Agreed. I'll change to MEMPOOL_F_POOL_CREATED, since we're already 
changing the *ring
element of the mempool struct to *pool

> 2) IMO, It is better to change void *pool in struct rte_mempool to
> anonymous union type, something like below, so that mempool
> implementation can choose the best type.
> 	union {
> 		void *pool;
> 		uint64_t val;
> 	}

Could we do this by using the union for the *pool_config suggested 
above, would that give
you what you need?

> 3) int32_t handler_idx creates 4 byte hole in struct rte_mempool in
> 64 bit system. IMO it better to rearrange.(as const struct rte_memzone
> *mz comes next)
OK, Will look at this.

> 4) IMO, It is better to change ext_alloc/ext_free to ext_create/ext_destroy
> as their is no allocation in HW assisted pool manager case,
> it will be mostly creating some HW initialization.

OK, I'll change. I think that makes more sense.


Regards,
Dave.
  

On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
> 
> New mempool handlers will use rte_mempool_create_empty(),
> rte_mempool_set_handler(),
> then rte_mempool_populate_*(). These three functions are new to this
> release, to no problem

Having separate APIs for external pool-manager create is worrisome in
application perspective. Is it possible to have rte_mempool_[xmem]_create
for the both external and existing SW pool manager and make
rte_mempool_create_empty and rte_mempool_populate_*  internal functions.

IMO, We can do that by selecting  specific rte_mempool_set_handler()
based on _flags_ encoding, something like below

bit 0 - 16   // generic bits uses across all the pool managers
bit 16 - 23  // pool handler specific flags bits
bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
pool managers, For backward compatibility, make '0'(in 24-31) to select
existing SW pool manager.

and applications can choose the handlers by selecting the flag in
rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
applications to choose the pool handler from command line etc in future.

and we can remove "mbuf: get default mempool handler from configuration"
change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.

What do you think?

> to add a parameter to one of them for the config data. Also since we're
> adding some new
> elements to the mempool structure, how about we add a new pointer for a void
> pointer to a
> config data structure, as defined by the handler.
> 
> So, new element in rte_mempool struct alongside the *pool
>     void *pool;
>     void *pool_config;
> 
> Then add a param to the rte_mempool_set_handler function:
> int
> rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void
> *pool_config)

IMO, Maybe we need to have _set_ and _get_.So I think we can have
two separate callback in external pool-manger for that if required.
IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23)
for the configuration as mentioned above and add the new callbacks for
set and get when required?

> > 2) IMO, It is better to change void *pool in struct rte_mempool to
> > anonymous union type, something like below, so that mempool
> > implementation can choose the best type.
> > 	union {
> > 		void *pool;
> > 		uint64_t val;
> > 	}
> 
> Could we do this by using the union for the *pool_config suggested above,
> would that give
> you what you need?

It would be an extra overhead for external pool manager to _alloc_ memory
and store the allocated pointer in mempool struct(as *pool) and use pool for
pointing other data structures as some implementation need only
limited bytes to store the external pool manager specific context.

In order to fix this problem, We may classify fast path and slow path
elements in struct rte_mempool and move all fast path elements in first
cache line and create an empty opaque space in the remaining bytes in the
cache line so that if the external pool manager needs only limited space
then it is not required to allocate the separate memory to save the
per core cache  in fast-path

something like below,
union {
	void *pool;
	uint64_t val;
	uint8_t extra_mem[16] // available free bytes in fast path cache line

}

Other points,

1) Is it possible to remove unused is_mp in  __mempool_put_bulk
function as it is just a internal function.

2) Considering "get" and "put" are the fast-path callbacks for
pool-manger, Is it possible to avoid the extra overhead of the following
_load_ and additional cache line on each call,
rte_mempool_handler_table.handler[handler_idx]

I understand it is for multiprocess support but I am thing can we
introduce something like ethernet API support for multiprocess and
resolve "put" and "get" functions pointer on init and store in
struct mempool. Some thinking like

file: drivers/net/ixgbe/ixgbe_ethdev.c
search for if (rte_eal_process_type() != RTE_PROC_PRIMARY) {

Jerin
  

Hi Jan,

On 5/23/2016 1:35 PM, Jan Viktorin wrote:
>> Until now, the objects stored in mempool mempool were internally stored a
> s/mempool mempool/mempool/
>
> stored _in_ a ring?

Fixed.

>
> @@ -161,7 +161,6 @@ per_lcore_mempool_test(__attribute__((unused)) void *arg)
>   							   n_get_bulk);
>   				if (unlikely(ret < 0)) {
>   					rte_mempool_dump(stdout, mp);
> -					rte_ring_dump(stdout, mp->ring);
>   					/* in this case, objects are lost... */
>   					return -1;
>   				}
> I think, this should be in a separate patch explaining the reason to remove it.

Done. Moved.

>> diff --git a/lib/librte_mempool/Makefile b/lib/librte_mempool/Makefile
>> index 43423e0..f19366e 100644
>> --- a/lib/librte_mempool/Makefile
>> +++ b/lib/librte_mempool/Makefile
>> @@ -42,6 +42,8 @@ LIBABIVER := 2
>>   
>>   # all source are stored in SRCS-y
>>   SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_handler.c
>> +SRCS-$(CONFIG_RTE_LIBRTE_MEMPOOL) +=  rte_mempool_default.c
>>   # install includes
>>   SYMLINK-$(CONFIG_RTE_LIBRTE_MEMPOOL)-include := rte_mempool.h
>>   
>> diff --git a/lib/librte_mempool/rte_mempool.c b/lib/librte_mempool/rte_mempool.c
>> index 1ab6701..6ec2b3f 100644
>> --- a/lib/librte_mempool/rte_mempool.c
>> +++ b/lib/librte_mempool/rte_mempool.c
>> @@ -148,7 +148,7 @@ mempool_add_elem(struct rte_mempool *mp, void *obj, phys_addr_t physaddr)
>>   #endif
>>   
>>   	/* enqueue in ring */
>> -	rte_ring_sp_enqueue(mp->ring, obj);
>> +	rte_mempool_ext_put_bulk(mp, &obj, 1);
> I suppose this is OK, however, replacing "enqueue" by "put" (semantically) sounds to me
> like a bug. Enqueue is put into a queue. Put is to drop a reference.

Yes, Makes sense. Changed  'put' and 'get' to 'enqueue' and 'dequeue'

>
>>   
>> -/* create the internal ring */
>> -static int
>> -rte_mempool_ring_create(struct rte_mempool *mp)
>> -{
>> -	int rg_flags = 0, ret;
>> -	char rg_name[RTE_RING_NAMESIZE];
>> -	struct rte_ring *r;
>> -
>> -	ret = snprintf(rg_name, sizeof(rg_name),
>> -		RTE_MEMPOOL_MZ_FORMAT, mp->name);
>> -	if (ret < 0 || ret >= (int)sizeof(rg_name))
>> -		return -ENAMETOOLONG;
>> -
>> -	/* ring flags */
>> -	if (mp->flags & MEMPOOL_F_SP_PUT)
>> -		rg_flags |= RING_F_SP_ENQ;
>> -	if (mp->flags & MEMPOOL_F_SC_GET)
>> -		rg_flags |= RING_F_SC_DEQ;
>> -
>> -	/* Allocate the ring that will be used to store objects.
>> -	 * Ring functions will return appropriate errors if we are
>> -	 * running as a secondary process etc., so no checks made
>> -	 * in this function for that condition.
>> -	 */
>> -	r = rte_ring_create(rg_name, rte_align32pow2(mp->size + 1),
>> -		mp->socket_id, rg_flags);
>> -	if (r == NULL)
>> -		return -rte_errno;
>> -
>> -	mp->ring = r;
>> -	mp->flags |= MEMPOOL_F_RING_CREATED;
>> -	return 0;
>> -}
> This is a big change. I suggest (if possible) to make a separate patch with
> something like "replace rte_mempool_ring_create by ...". Where is this code
> placed now?

The code is not gone away, it's now part of the default handler, which 
uses a ring. It's
in rte_mempool_default.c

>> -
>>   /* free a memchunk allocated with rte_memzone_reserve() */
>>   static void
>>   rte_mempool_memchunk_mz_free(__rte_unused struct rte_mempool_memhdr *memhdr,
>> @@ -351,7 +317,7 @@ rte_mempool_free_memchunks(struct rte_mempool *mp)
>>   	void *elt;
>>   
>>   	while (!STAILQ_EMPTY(&mp->elt_list)) {
>> -		rte_ring_sc_dequeue(mp->ring, &elt);
>> +		rte_mempool_ext_get_bulk(mp, &elt, 1);
> Similar as for put_bulk... Replacing "dequeue" by "get" (semantically) sounds to me
> like a bug. Dequeue is drop from a queue. Get is to obtain a reference.

Done

>>   		(void)elt;
>>   		STAILQ_REMOVE_HEAD(&mp->elt_list, next);
>>   		mp->populated_size--;
>> @@ -380,15 +346,18 @@ 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_ext_alloc(mp);
>> +		if (mp->pool == NULL) {
>> +			if (rte_errno == 0)
>> +				return -EINVAL;
>> +			else
>> +				return -rte_errno;
>> +		}
>>   	}
>> -
> Is this a whitespace change?

Accidental. Reverted


>> +
>> +	/*
>> +	 * Since we have 4 combinations of the SP/SC/MP/MC examine the flags to
>> +	 * set the correct index into the handler table.
>> +	 */
>> +	if (flags & (MEMPOOL_F_SP_PUT | MEMPOOL_F_SC_GET))
>> +		rte_mempool_set_handler(mp, "ring_sp_sc");
>> +	else if (flags & MEMPOOL_F_SP_PUT)
>> +		rte_mempool_set_handler(mp, "ring_sp_mc");
>> +	else if (flags & MEMPOOL_F_SC_GET)
>> +		rte_mempool_set_handler(mp, "ring_mp_sc");
>> +	else
>> +		rte_mempool_set_handler(mp, "ring_mp_mc");
>> +
> Do I understand it well that this code preserves behaviour of the previous API?
> Because otherwise it looks strange.

Yes. it's just to keep backward compatibility. It will also move to 
somewhere more sensible
in the latest patch, rte_mempool_create rather than 
rte_mempool_create_empty.

>>   struct rte_mempool {
>>   	char name[RTE_MEMPOOL_NAMESIZE]; /**< Name of mempool. */
>> -	struct rte_ring *ring;           /**< Ring to store objects. */
>> +	void *pool;                      /**< Ring or ext-pool to store objects. */
>> +	/**
>> +	 * Index into the array of structs containing callback fn 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. Any function pointers stored in the mempool
>> +	 * directly would not be valid for secondary processes.
>> +	 */
> I think, this comment should go to the rte_mempool_handler_table definition
> leaving a here a short note about it.

I've added a comment to rte_mempool_handler_table, and tweaked this 
comment somewhat.

>> +	int32_t handler_idx;
>>   	const struct rte_memzone *mz;    /**< Memzone where pool is allocated */
>>   	int flags;                       /**< Flags of the mempool. */
>>   	int socket_id;                   /**< Socket id passed at mempool creation. */
>> @@ -325,6 +334,175 @@ 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. */
>> +
>> +/** Allocate the external pool. */

Note: for the next few comments you switched to commenting above the 
code , I've moved
the comments to below the code, and replied.

>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>> +
>> +/** Free the external pool. */
> What is the purpose of this callback?
> What exactly does it allocate?
> Some rte_mempool internals?
> Or the memory?
> What does it return?

This is the main allocate function of the handler. It is up to the 
mempool handlers control.
The handler's alloc function does whatever it needs to do to grab memory 
for this handler, and places
a pointer to it in the *pool opaque pointer in the rte_mempool struct. 
In the default handler, *pool
points to a ring, in other handlers, it will mostlikely point to a 
different type of data structure. It will
be transparent to the application programmer.

>> +typedef void (*rte_mempool_free_t)(void *p);
>> +
>> +/** Put an object in the external pool. */
> Why this *_free callback does not accept the rte_mempool param?
>

We're freeing the pool opaque data here.


>> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);
> What is the *p pointer?
> What is the obj_table?
> Why is it void *?
> Why is it const?
>

The *p pointer is the opaque data for a given mempool handler (ring, 
array, linked list, etc)

> Probably, "unsigned int n" is better.
>
>> +
>> +/** Get an object from the external pool. */
>> +typedef int (*rte_mempool_get_t)(void *p, void **obj_table, unsigned n);
> Probably, "unsigned int n" is better.

Done.

>> +
>> +/** Return the number of available objects in the external pool. */
> What is the purpose of the *_get_count callback? I guess it can introduce
> race conditions...

I think it depends on the implementation of the handlers get_count 
function, it must
ensure not to return values greater than the size of the mempool.

>> +typedef unsigned (*rte_mempool_get_count)(void *p);
> unsigned int

Sure.

>> +
>> +/** Structure defining a mempool handler. */
> Later in the text, I suggested to rename rte_mempool_handler to rte_mempool_ops.
> I believe that it explains the purpose of this struct better. It would improve
> consistency in function names (the *_ext_* mark is very strange and inconsistent).

I agree. I've gone through all the code and renamed to 
rte_mempool_handler_ops.

>> +struct rte_mempool_handler {
>> +	char name[RTE_MEMPOOL_HANDLER_NAMESIZE]; /**< Name of mempool handler */
>> +	rte_mempool_alloc_t alloc;       /**< Allocate the external pool. */
>> +	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 number of registered handlers */
>> +
>> +/** Structure storing the table of registered handlers. */
>> +struct rte_mempool_handler_table {
>> +	rte_spinlock_t sl;     /**< Spinlock for add/delete. */
>> +	uint32_t num_handlers; /**< Number of handlers in the table. */
>> +	/** Storage for all possible handlers. */
>> +	struct rte_mempool_handler handler[RTE_MEMPOOL_MAX_HANDLER_IDX];
>> +};
> The handlers are implemented as an array due to multi-process access.
> Is it correct? I'd expect a note about it here.

Yes, you are correct. I've improved the comments.

>> +
>> +/** Array of registered handlers */
>> +extern struct rte_mempool_handler_table rte_mempool_handler_table;
>> +
>> +/**
>> + * @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 struct rte_mempool_handler *
>> +rte_mempool_handler_get(int handler_idx)
>> +{
>> +	return &rte_mempool_handler_table.handler[handler_idx];
> Is it always safe? Can we belive the handler_idx is inside the boundaries?
> At least some RTE_VERIFY would be nice here...

Agreed. Added:
RTE_VERIFY(handler_idx < RTE_MEMPOOL_MAX_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_ext_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.
> Should this doc be more specific about the possible failures?

This is up to the handler. We cannot know what codes will be returned at 
this stage.

>> + */
>> +static inline int
>> +rte_mempool_ext_get_bulk(struct rte_mempool *mp, void **obj_table, unsigned n)
>> +{
>> +	struct rte_mempool_handler *handler;
>> +
>> +	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.
> Should this doc be more specific about the possible failures?

This is up to the handler. We cannot know what codes will be returned at 
this stage.

>> + */
>> +static inline int
>> +rte_mempool_ext_put_bulk(struct rte_mempool *mp, void * const *obj_table,
>> +		unsigned n)
>> +{
>> +	struct rte_mempool_handler *handler;
>> +
>> +	handler = rte_mempool_handler_get(mp->handler_idx);
>> +	return handler->put(mp->pool, obj_table, n);
>> +}
>> +
>> +/**
>> + * @internal wrapper for external mempool manager get_count callback.
>> + *
>> + * @param mp
>> + *   Pointer to the memory pool.
>> + * @return
>> + *   The number of available objects in the external pool.
>> + */
>> +unsigned
> unsigned int

Done.

>> +rte_mempool_ext_get_count(const struct rte_mempool *mp);
>> +
>> +/**
>> + * @internal wrapper for external mempool manager free callback.
>> + *
>> + * @param mp
>> + *   Pointer to the memory pool.
>> + */
>> +void
>> +rte_mempool_ext_free(struct rte_mempool *mp);
>> +
>> +/**
>> + * Set the handler of a mempool
>> + *
>> + * This can only be done on a mempool that is not populated, i.e. just after
>> + * a call to rte_mempool_create_empty().
>> + *
>> + * @param mp
>> + *   Pointer to the memory pool.
>> + * @param name
>> + *   Name of the handler.
>> + * @return
>> + *   - 0: Sucess; the new handler is configured.
>> + *   - <0: Error (errno)
> Should this doc be more specific about the possible failures?
>
> The body of rte_mempool_set_handler does not set errno at all.
> It returns e.g. -EEXIST.

This is up to the handler. We cannot know what codes will be returned at 
this stage.
errno was a cut-and paste error, this is now fixed.


>> + */
>> +int
>> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
>> +
>> +/**
>> + * Register an external pool handler.
>> + *
>> + * @param h
>> + *   Pointer to the external pool handler
>> + * @return
>> + *   - >=0: Sucess; return the index of the handler in the table.
>> + *   - <0: Error (errno)
> Should this doc be more specific about the possible failures?

This is up to the handler. We cannot know what codes will be returned at 
this stage.
errno was a cut-and paste error, this is now fixed.

>> + */
>> +int rte_mempool_handler_register(struct rte_mempool_handler *h);
>> +
>> +/**
>> + * Macro to statically register an external pool handler.
>> + */
>> +#define MEMPOOL_REGISTER_HANDLER(h)					\
>> +	void mp_hdlr_init_##h(void);					\
>> +	void __attribute__((constructor, used)) mp_hdlr_init_##h(void)	\
>> +	{								\
>> +		rte_mempool_handler_register(&h);			\
>> +	}
>> +
> There might be a little catch. If there is no more room for handlers, calling the
> rte_mempool_handler_register would fail silently as the error reporting does not
> work when calling a constructor (or at least, this is my experience).
>
> Not a big deal but...

I would hope that the developer would check this when adding new 
handlers. If there is no room
for new handlers, then their new one will never work...

>>   
>>   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");
>> -	}
>> -#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);
>> -#endif
>> +	rte_mempool_ext_put_bulk(mp, obj_table, n);
> This is a big change. Does it remove the RTE_LIBRTE_MEMPOOL_DEBUG config option
> entirely? If so, I suggest to first do this in a separated patch and then
> replace the original *_enqueue_bulk by your *_ext_put_bulk (or better *_ops_put_bulk
> as I explain below).

Well spotted. I have reverted this change. The mp_enqueue and sp_enqueue 
have been replaced with the
handlers version, and I've added back in the DEBUG check with the rte_panic.

>>   
>> +
>> +static struct rte_mempool_handler handler_mp_mc = {
>> +	.name = "ring_mp_mc",
>> +	.alloc = common_ring_alloc,
>> +	.free = common_ring_free,
>> +	.put = common_ring_mp_put,
>> +	.get = common_ring_mc_get,
>> +	.get_count = common_ring_get_count,
>> +};
>> +
>> +static struct rte_mempool_handler handler_sp_sc = {
>> +	.name = "ring_sp_sc",
>> +	.alloc = common_ring_alloc,
>> +	.free = common_ring_free,
>> +	.put = common_ring_sp_put,
>> +	.get = common_ring_sc_get,
>> +	.get_count = common_ring_get_count,
>> +};
>> +
>> +static struct rte_mempool_handler handler_mp_sc = {
>> +	.name = "ring_mp_sc",
>> +	.alloc = common_ring_alloc,
>> +	.free = common_ring_free,
>> +	.put = common_ring_mp_put,
>> +	.get = common_ring_sc_get,
>> +	.get_count = common_ring_get_count,
>> +};
>> +
>> +static struct rte_mempool_handler handler_sp_mc = {
>> +	.name = "ring_sp_mc",
>> +	.alloc = common_ring_alloc,
>> +	.free = common_ring_free,
>> +	.put = common_ring_sp_put,
>> +	.get = common_ring_mc_get,
>> +	.get_count = common_ring_get_count,
>> +};
>> +
> Introducing those handlers can go as a separate patch. IMHO, that would simplify
> the review process a lot. First introduce the mechanism, then add something
> inside.
>
> I'd also note that those handlers are always available and what kind of memory
> do they use...

Done. Now added as a separate patch.

They don't use any memory unless they are used.


>> +#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 = {
>> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
>> +	.num_handlers = 0
>> +};
>> +
>> +/* add a new handler in rte_mempool_handler_table, return its index */
> It seems to me that there is no way how to put some opaque pointer into the
> handler. In such case I would expect I can do something like:
>
> struct my_handler {
> 	struct rte_mempool_handler h;
> 	...
> } handler;
>
> rte_mempool_handler_register(&handler.h);
>
> But I cannot because you copy the contents of the handler. By the way, this
> should be documented.
>
> How can I pass an opaque pointer here? The only way I see is through the
> rte_mempool.pool.

I think have addressed this in a later patch, in the discussions with 
Jerin on the list. But
rather than passing data at register time, I pass it at create time (or 
rather set_handler).
And a new config void has been added to the mempool struct for this 
purpose.

>   In that case, what about renaming the rte_mempool_handler
> to rte_mempool_ops? Because semantically, it is not a handler, it just holds
> the operations.
>
> This would improve some namings:
>
> rte_mempool_ext_alloc -> rte_mempool_ops_alloc
> rte_mempool_ext_free -> rte_mempool_ops_free
> rte_mempool_ext_get_count -> rte_mempool_ops_get_count
> rte_mempool_handler_register -> rte_mempool_ops_register
>
> seems to be more readable to me. The *_ext_* mark does not say anything valuable.
> It just scares a bit :).

Agreed. Makes sense. The ext was intended to be 'external', but that's a 
bit too generic, and not
very intuitive. the 'ops' tag seems better to me. I've change this in 
the latest patch.

>> +/* wrapper to get available objects in an external pool handler */
>> +unsigned
>> +rte_mempool_ext_get_count(const struct rte_mempool *mp)
>> +{
>> +	struct rte_mempool_handler *handler;
>> +
>> +	handler = rte_mempool_handler_get(mp->handler_idx);
>> +	return handler->get_count(mp->pool);
>> +}
>> +
>> +/* set the handler of a mempool */
> The doc comment should say "this sets a handler previously registered by
> the rte_mempool_handler_register function ...". I was confused and didn't
> understand how the handlers are inserted into the table.

Done.

--snip--

> Regards
> Jan

Thanks, Jan. Very comprehensive.  I'll hopefully be pushing the latest 
patch to the list later today (Tuesday 31st)

Regard,
Dave.
  

On Tue, 31 May 2016 10:09:42 +0100
"Hunt, David" <david.hunt@intel.com> wrote:

> Hi Jan,
> 

[...]

> 
> >> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
> >> +
> >> +/** Free the external pool. */  
> > What is the purpose of this callback?
> > What exactly does it allocate?
> > Some rte_mempool internals?
> > Or the memory?
> > What does it return?  
> 
> This is the main allocate function of the handler. It is up to the 
> mempool handlers control.
> The handler's alloc function does whatever it needs to do to grab memory 
> for this handler, and places
> a pointer to it in the *pool opaque pointer in the rte_mempool struct. 
> In the default handler, *pool
> points to a ring, in other handlers, it will mostlikely point to a 
> different type of data structure. It will
> be transparent to the application programmer.

Thanks for explanation. Please, add doc comments there.

Should the *mp be const here? It is not clear to me whether the callback is
expected to modify the mempool or not (eg. set the pool pointer).

> 
> >> +typedef void (*rte_mempool_free_t)(void *p);
> >> +
> >> +/** Put an object in the external pool. */  
> > Why this *_free callback does not accept the rte_mempool param?
> >  
> 
> We're freeing the pool opaque data here.

Add doc comments...

> 
> 
> >> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);  
> > What is the *p pointer?
> > What is the obj_table?
> > Why is it void *?
> > Why is it const?
> >  
> 
> The *p pointer is the opaque data for a given mempool handler (ring, 
> array, linked list, etc)

Again, doc comments...

I don't like the obj_table representation to be an array of void *. I could see
it already in DPDK for defining Ethernet driver queues, so, it's probably not
an issue. I just say, I would prefer some basic type safety like

struct rte_mempool_obj {
	void *p;
};

Is there somebody with different opinions?

[...]

> >> +
> >> +/** Structure defining a mempool handler. */  
> > Later in the text, I suggested to rename rte_mempool_handler to rte_mempool_ops.
> > I believe that it explains the purpose of this struct better. It would improve
> > consistency in function names (the *_ext_* mark is very strange and inconsistent).  
> 
> I agree. I've gone through all the code and renamed to 
> rte_mempool_handler_ops.

Ok. I meant rte_mempool_ops because I find the word "handler" to be redundant.

> 

[...]

> >> +/**
> >> + * Set the handler of a mempool
> >> + *
> >> + * This can only be done on a mempool that is not populated, i.e. just after
> >> + * a call to rte_mempool_create_empty().
> >> + *
> >> + * @param mp
> >> + *   Pointer to the memory pool.
> >> + * @param name
> >> + *   Name of the handler.
> >> + * @return
> >> + *   - 0: Sucess; the new handler is configured.
> >> + *   - <0: Error (errno)  
> > Should this doc be more specific about the possible failures?
> >
> > The body of rte_mempool_set_handler does not set errno at all.
> > It returns e.g. -EEXIST.  
> 
> This is up to the handler. We cannot know what codes will be returned at 
> this stage.
> errno was a cut-and paste error, this is now fixed.

I don't think so. The rte_mempool_set_handler is not handler-specific:

116 /* set the handler of a mempool */
117 int
118 rte_mempool_set_handler(struct rte_mempool *mp, const char *name)
119 {
120         struct rte_mempool_handler *handler = NULL;
121         unsigned i;
122
123         /* too late, the mempool is already populated */
124         if (mp->flags & MEMPOOL_F_RING_CREATED)
125                 return -EEXIST;

Here, it returns -EEXIST.

126
127         for (i = 0; i < rte_mempool_handler_table.num_handlers; i++) {
128                 if (!strcmp(name, rte_mempool_handler_table.handler[i].name)) {
129                         handler = &rte_mempool_handler_table.handler[i];
130                         break;
131                 }
132         }
133
134         if (handler == NULL)
135                 return -EINVAL;

And here, it returns -EINVAL.

136
137         mp->handler_idx = i;
138         return 0;
139 }

So, it is possible to define those in the doc comment.

> 
> 
> >> + */
> >> +int
> >> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
> >> +
> >> +/**
> >> + * Register an external pool handler.
> >> + *
> >> + * @param h
> >> + *   Pointer to the external pool handler
> >> + * @return
> >> + *   - >=0: Sucess; return the index of the handler in the table.
> >> + *   - <0: Error (errno)  
> > Should this doc be more specific about the possible failures?  
> 
> This is up to the handler. We cannot know what codes will be returned at 
> this stage.
> errno was a cut-and paste error, this is now fixed.

Same, here... -ENOSPC, -EINVAL are returned in certain cases. And again,
this call is not handler-specific.

>
 
[...]

> >>   
> >> +
> >> +static struct rte_mempool_handler handler_mp_mc = {
> >> +	.name = "ring_mp_mc",
> >> +	.alloc = common_ring_alloc,
> >> +	.free = common_ring_free,
> >> +	.put = common_ring_mp_put,
> >> +	.get = common_ring_mc_get,
> >> +	.get_count = common_ring_get_count,
> >> +};
> >> +
> >> +static struct rte_mempool_handler handler_sp_sc = {
> >> +	.name = "ring_sp_sc",
> >> +	.alloc = common_ring_alloc,
> >> +	.free = common_ring_free,
> >> +	.put = common_ring_sp_put,
> >> +	.get = common_ring_sc_get,
> >> +	.get_count = common_ring_get_count,
> >> +};
> >> +
> >> +static struct rte_mempool_handler handler_mp_sc = {
> >> +	.name = "ring_mp_sc",
> >> +	.alloc = common_ring_alloc,
> >> +	.free = common_ring_free,
> >> +	.put = common_ring_mp_put,
> >> +	.get = common_ring_sc_get,
> >> +	.get_count = common_ring_get_count,
> >> +};
> >> +
> >> +static struct rte_mempool_handler handler_sp_mc = {
> >> +	.name = "ring_sp_mc",
> >> +	.alloc = common_ring_alloc,
> >> +	.free = common_ring_free,
> >> +	.put = common_ring_sp_put,
> >> +	.get = common_ring_mc_get,
> >> +	.get_count = common_ring_get_count,
> >> +};
> >> +  
> > Introducing those handlers can go as a separate patch. IMHO, that would simplify
> > the review process a lot. First introduce the mechanism, then add something
> > inside.
> >
> > I'd also note that those handlers are always available and what kind of memory
> > do they use...  
> 
> Done. Now added as a separate patch.
> 
> They don't use any memory unless they are used.

Yes, that is what I've meant... What is the source of the allocations? Where does
the common_ring_sc_get get memory from?

Anyway, any documentation describing the goal of those four declarations
would be helpful.

> 
> 
> >> +#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 = {
> >> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
> >> +	.num_handlers = 0
> >> +};
> >> +
> >> +/* add a new handler in rte_mempool_handler_table, return its index */  
> > It seems to me that there is no way how to put some opaque pointer into the
> > handler. In such case I would expect I can do something like:
> >
> > struct my_handler {
> > 	struct rte_mempool_handler h;
> > 	...
> > } handler;
> >
> > rte_mempool_handler_register(&handler.h);
> >
> > But I cannot because you copy the contents of the handler. By the way, this
> > should be documented.
> >
> > How can I pass an opaque pointer here? The only way I see is through the
> > rte_mempool.pool.  
> 
> I think have addressed this in a later patch, in the discussions with 
> Jerin on the list. But
> rather than passing data at register time, I pass it at create time (or 
> rather set_handler).
> And a new config void has been added to the mempool struct for this 
> purpose.

Ok, sounds promising. It just should be well specified to avoid situations
when accessing the the pool_config before it is set.

> 
> >   In that case, what about renaming the rte_mempool_handler
> > to rte_mempool_ops? Because semantically, it is not a handler, it just holds
> > the operations.
> >
> > This would improve some namings:
> >
> > rte_mempool_ext_alloc -> rte_mempool_ops_alloc
> > rte_mempool_ext_free -> rte_mempool_ops_free
> > rte_mempool_ext_get_count -> rte_mempool_ops_get_count
> > rte_mempool_handler_register -> rte_mempool_ops_register
> >
> > seems to be more readable to me. The *_ext_* mark does not say anything valuable.
> > It just scares a bit :).  
> 
> Agreed. Makes sense. The ext was intended to be 'external', but that's a 
> bit too generic, and not
> very intuitive. the 'ops' tag seems better to me. I've change this in 
> the latest patch.

Again, note that I've suggested to avoid the word _handler_ entirely.

[...]

> 
> > Regards
> > Jan  
> 
> Thanks, Jan. Very comprehensive.  I'll hopefully be pushing the latest 
> patch to the list later today (Tuesday 31st)

Cool, please CC me.

Jan

> 
> Regard,
> Dave.
> 
>
  

On 5/31/2016 1:06 PM, Jan Viktorin wrote:
> On Tue, 31 May 2016 10:09:42 +0100
> "Hunt, David" <david.hunt@intel.com> wrote:
>
>> Hi Jan,
>>
> [...]
>
>>>> +typedef void *(*rte_mempool_alloc_t)(struct rte_mempool *mp);
>>>> +
>>>> +/** Free the external pool. */
>>> What is the purpose of this callback?
>>> What exactly does it allocate?
>>> Some rte_mempool internals?
>>> Or the memory?
>>> What does it return?
>> This is the main allocate function of the handler. It is up to the
>> mempool handlers control.
>> The handler's alloc function does whatever it needs to do to grab memory
>> for this handler, and places
>> a pointer to it in the *pool opaque pointer in the rte_mempool struct.
>> In the default handler, *pool
>> points to a ring, in other handlers, it will mostlikely point to a
>> different type of data structure. It will
>> be transparent to the application programmer.
> Thanks for explanation. Please, add doc comments there.
>
> Should the *mp be const here? It is not clear to me whether the callback is
> expected to modify the mempool or not (eg. set the pool pointer).

Comment added. Not const, as the *pool is set.

>>>> +typedef void (*rte_mempool_free_t)(void *p);
>>>> +
>>>> +/** Put an object in the external pool. */
>>> Why this *_free callback does not accept the rte_mempool param?
>>>   
>> We're freeing the pool opaque data here.
> Add doc comments...

Done.

>>
>>>> +typedef int (*rte_mempool_put_t)(void *p, void * const *obj_table, unsigned n);
>>> What is the *p pointer?
>>> What is the obj_table?
>>> Why is it void *?
>>> Why is it const?
>>>   
>> The *p pointer is the opaque data for a given mempool handler (ring,
>> array, linked list, etc)
> Again, doc comments...
>
> I don't like the obj_table representation to be an array of void *. I could see
> it already in DPDK for defining Ethernet driver queues, so, it's probably not
> an issue. I just say, I would prefer some basic type safety like
>
> struct rte_mempool_obj {
> 	void *p;
> };
>
> Is there somebody with different opinions?
>
> [...]

Comments added. I've left as a void* for the moment.

>>>> +
>>>> +/** Structure defining a mempool handler. */
>>> Later in the text, I suggested to rename rte_mempool_handler to rte_mempool_ops.
>>> I believe that it explains the purpose of this struct better. It would improve
>>> consistency in function names (the *_ext_* mark is very strange and inconsistent).
>> I agree. I've gone through all the code and renamed to
>> rte_mempool_handler_ops.
> Ok. I meant rte_mempool_ops because I find the word "handler" to be redundant.

I prefer the use of the word handler, unless others also have opinions 
either way?

>
>>>> +/**
>>>> + * Set the handler of a mempool
>>>> + *
>>>> + * This can only be done on a mempool that is not populated, i.e. just after
>>>> + * a call to rte_mempool_create_empty().
>>>> + *
>>>> + * @param mp
>>>> + *   Pointer to the memory pool.
>>>> + * @param name
>>>> + *   Name of the handler.
>>>> + * @return
>>>> + *   - 0: Sucess; the new handler is configured.
>>>> + *   - <0: Error (errno)
>>> Should this doc be more specific about the possible failures?
>>>
>>> The body of rte_mempool_set_handler does not set errno at all.
>>> It returns e.g. -EEXIST.
>> This is up to the handler. We cannot know what codes will be returned at
>> this stage.
>> errno was a cut-and paste error, this is now fixed.
> I don't think so. The rte_mempool_set_handler is not handler-specific:
[...]
> So, it is possible to define those in the doc comment.

Ah, I see now. My mistake, I assumed a different function. Added now.

>>
>>>> + */
>>>> +int
>>>> +rte_mempool_set_handler(struct rte_mempool *mp, const char *name);
>>>> +
>>>> +/**
>>>> + * Register an external pool handler.
>>>> + *
>>>> + * @param h
>>>> + *   Pointer to the external pool handler
>>>> + * @return
>>>> + *   - >=0: Sucess; return the index of the handler in the table.
>>>> + *   - <0: Error (errno)
>>> Should this doc be more specific about the possible failures?
>> This is up to the handler. We cannot know what codes will be returned at
>> this stage.
>> errno was a cut-and paste error, this is now fixed.
> Same, here... -ENOSPC, -EINVAL are returned in certain cases. And again,
> this call is not handler-specific.

Yes, Added now to comments.

>   
> [...]
>
>>>>    
>>>> +
>>>> +static struct rte_mempool_handler handler_mp_mc = {
>>>> +	.name = "ring_mp_mc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_mp_put,
>>>> +	.get = common_ring_mc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>>> +static struct rte_mempool_handler handler_sp_sc = {
>>>> +	.name = "ring_sp_sc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_sp_put,
>>>> +	.get = common_ring_sc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>>> +static struct rte_mempool_handler handler_mp_sc = {
>>>> +	.name = "ring_mp_sc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_mp_put,
>>>> +	.get = common_ring_sc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>>> +static struct rte_mempool_handler handler_sp_mc = {
>>>> +	.name = "ring_sp_mc",
>>>> +	.alloc = common_ring_alloc,
>>>> +	.free = common_ring_free,
>>>> +	.put = common_ring_sp_put,
>>>> +	.get = common_ring_mc_get,
>>>> +	.get_count = common_ring_get_count,
>>>> +};
>>>> +
>>> Introducing those handlers can go as a separate patch. IMHO, that would simplify
>>> the review process a lot. First introduce the mechanism, then add something
>>> inside.
>>>
>>> I'd also note that those handlers are always available and what kind of memory
>>> do they use...
>> Done. Now added as a separate patch.
>>
>> They don't use any memory unless they are used.
> Yes, that is what I've meant... What is the source of the allocations? Where does
> the common_ring_sc_get get memory from?
>
> Anyway, any documentation describing the goal of those four declarations
> would be helpful.

For these handlers, the allocations are as per the original code before 
this patch. For new handlers,
hardware allocators, stack allocators, etc.

I've added comments on the 4 declarations.

>>
>>>> +#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 = {
>>>> +	.sl =  RTE_SPINLOCK_INITIALIZER ,
>>>> +	.num_handlers = 0
>>>> +};
>>>> +
>>>> +/* add a new handler in rte_mempool_handler_table, return its index */
>>> It seems to me that there is no way how to put some opaque pointer into the
>>> handler. In such case I would expect I can do something like:
>>>
>>> struct my_handler {
>>> 	struct rte_mempool_handler h;
>>> 	...
>>> } handler;
>>>
>>> rte_mempool_handler_register(&handler.h);
>>>
>>> But I cannot because you copy the contents of the handler. By the way, this
>>> should be documented.
>>>
>>> How can I pass an opaque pointer here? The only way I see is through the
>>> rte_mempool.pool.
>> I think have addressed this in a later patch, in the discussions with
>> Jerin on the list. But
>> rather than passing data at register time, I pass it at create time (or
>> rather set_handler).
>> And a new config void has been added to the mempool struct for this
>> purpose.
> Ok, sounds promising. It just should be well specified to avoid situations
> when accessing the the pool_config before it is set.
>
>>>    In that case, what about renaming the rte_mempool_handler
>>> to rte_mempool_ops? Because semantically, it is not a handler, it just holds
>>> the operations.
>>>
>>> This would improve some namings:
>>>
>>> rte_mempool_ext_alloc -> rte_mempool_ops_alloc
>>> rte_mempool_ext_free -> rte_mempool_ops_free
>>> rte_mempool_ext_get_count -> rte_mempool_ops_get_count
>>> rte_mempool_handler_register -> rte_mempool_ops_register
>>>
>>> seems to be more readable to me. The *_ext_* mark does not say anything valuable.
>>> It just scares a bit :).
>> Agreed. Makes sense. The ext was intended to be 'external', but that's a
>> bit too generic, and not
>> very intuitive. the 'ops' tag seems better to me. I've change this in
>> the latest patch.
> Again, note that I've suggested to avoid the word _handler_ entirely.
>
> [...]
>
>>> Regards
>>> Jan
>> Thanks, Jan. Very comprehensive.  I'll hopefully be pushing the latest
>> patch to the list later today (Tuesday 31st)
> Cool, please CC me.

Will do.


Rgds,
Dave.
  

On 5/31/2016 9:53 AM, Jerin Jacob wrote:
> On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
>> New mempool handlers will use rte_mempool_create_empty(),
>> rte_mempool_set_handler(),
>> then rte_mempool_populate_*(). These three functions are new to this
>> release, to no problem
> Having separate APIs for external pool-manager create is worrisome in
> application perspective. Is it possible to have rte_mempool_[xmem]_create
> for the both external and existing SW pool manager and make
> rte_mempool_create_empty and rte_mempool_populate_*  internal functions.
>
> IMO, We can do that by selecting  specific rte_mempool_set_handler()
> based on _flags_ encoding, something like below
>
> bit 0 - 16   // generic bits uses across all the pool managers
> bit 16 - 23  // pool handler specific flags bits
> bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
> pool managers, For backward compatibility, make '0'(in 24-31) to select
> existing SW pool manager.
>
> and applications can choose the handlers by selecting the flag in
> rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
> applications to choose the pool handler from command line etc in future.

There might be issues with the 8-bit handler number, as we'd have to add 
an api call to
first get the index of a given hander by name, then OR it into the 
flags. That would mean
also extra API calls for the non-default external handlers. I do agree 
with the handler-specific
bits though.

Having the _empty and _set_handler  APIs seems to me to be OK for the
moment. Maybe Olivier could comment?

> and we can remove "mbuf: get default mempool handler from configuration"
> change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
> the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
>
> What do you think?

The "configuration" patch is to allow users to quickly change the 
mempool handler
by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
handler. It could just as easily be left out and use the 
rte_mempool_create.

>> to add a parameter to one of them for the config data. Also since we're
>> adding some new
>> elements to the mempool structure, how about we add a new pointer for a void
>> pointer to a
>> config data structure, as defined by the handler.
>>
>> So, new element in rte_mempool struct alongside the *pool
>>      void *pool;
>>      void *pool_config;
>>
>> Then add a param to the rte_mempool_set_handler function:
>> int
>> rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void
>> *pool_config)
> IMO, Maybe we need to have _set_ and _get_.So I think we can have
> two separate callback in external pool-manger for that if required.
> IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23)
> for the configuration as mentioned above and add the new callbacks for
> set and get when required?

OK, We'll keep the config to the 8 bits of the flags for now. That will 
also mean I won't
add the pool_config void pointer either (for the moment)

>>> 2) IMO, It is better to change void *pool in struct rte_mempool to
>>> anonymous union type, something like below, so that mempool
>>> implementation can choose the best type.
>>> 	union {
>>> 		void *pool;
>>> 		uint64_t val;
>>> 	}
>> Could we do this by using the union for the *pool_config suggested above,
>> would that give
>> you what you need?
> It would be an extra overhead for external pool manager to _alloc_ memory
> and store the allocated pointer in mempool struct(as *pool) and use pool for
> pointing other data structures as some implementation need only
> limited bytes to store the external pool manager specific context.
>
> In order to fix this problem, We may classify fast path and slow path
> elements in struct rte_mempool and move all fast path elements in first
> cache line and create an empty opaque space in the remaining bytes in the
> cache line so that if the external pool manager needs only limited space
> then it is not required to allocate the separate memory to save the
> per core cache  in fast-path
>
> something like below,
> union {
> 	void *pool;
> 	uint64_t val;
> 	uint8_t extra_mem[16] // available free bytes in fast path cache line
>
> }

Something for the future, perhaps? Will the 8-bits in the flags suffice 
for now?


> Other points,
>
> 1) Is it possible to remove unused is_mp in  __mempool_put_bulk
> function as it is just a internal function.

Fixed

> 2) Considering "get" and "put" are the fast-path callbacks for
> pool-manger, Is it possible to avoid the extra overhead of the following
> _load_ and additional cache line on each call,
> rte_mempool_handler_table.handler[handler_idx]
>
> I understand it is for multiprocess support but I am thing can we
> introduce something like ethernet API support for multiprocess and
> resolve "put" and "get" functions pointer on init and store in
> struct mempool. Some thinking like
>
> file: drivers/net/ixgbe/ixgbe_ethdev.c
> search for if (rte_eal_process_type() != RTE_PROC_PRIMARY) {

I'll look at this one before posting the next version of the patch 
(soon). :)


> Jerin
>
Thanks for your input on this, much appreciated.
Dave.
  

On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote:
> 
> 
> On 5/31/2016 9:53 AM, Jerin Jacob wrote:
> > On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
> > > New mempool handlers will use rte_mempool_create_empty(),
> > > rte_mempool_set_handler(),
> > > then rte_mempool_populate_*(). These three functions are new to this
> > > release, to no problem
> > Having separate APIs for external pool-manager create is worrisome in
> > application perspective. Is it possible to have rte_mempool_[xmem]_create
> > for the both external and existing SW pool manager and make
> > rte_mempool_create_empty and rte_mempool_populate_*  internal functions.
> > 
> > IMO, We can do that by selecting  specific rte_mempool_set_handler()
> > based on _flags_ encoding, something like below
> > 
> > bit 0 - 16   // generic bits uses across all the pool managers
> > bit 16 - 23  // pool handler specific flags bits
> > bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
> > pool managers, For backward compatibility, make '0'(in 24-31) to select
> > existing SW pool manager.
> > 
> > and applications can choose the handlers by selecting the flag in
> > rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
> > applications to choose the pool handler from command line etc in future.
> 
> There might be issues with the 8-bit handler number, as we'd have to add an
> api call to
> first get the index of a given hander by name, then OR it into the flags.
> That would mean
> also extra API calls for the non-default external handlers. I do agree with
> the handler-specific
> bits though.

That would be an internal API(upper 8 bits to handler name). Right ?
Seems to be OK for me.

> 
> Having the _empty and _set_handler  APIs seems to me to be OK for the
> moment. Maybe Olivier could comment?
> 

But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe
it is better reduce the public API in spec where ever possible ?

Maybe Olivier could comment ?


> > and we can remove "mbuf: get default mempool handler from configuration"
> > change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
> > the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
> > 
> > What do you think?
> 
> The "configuration" patch is to allow users to quickly change the mempool
> handler
> by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
> handler. It could just as easily be left out and use the rte_mempool_create.
>

Yes, I understand, but I am trying to avoid build time constant. IMO, It
would be better by default RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is not
defined in config. and for quick change developers can introduce the build 
with RTE_MBUF_DEFAULT_MEMPOOL_HANDLER="specific handler"

 
> > > to add a parameter to one of them for the config data. Also since we're
> > > adding some new
> > > elements to the mempool structure, how about we add a new pointer for a void
> > > pointer to a
> > > config data structure, as defined by the handler.
> > > 
> > > So, new element in rte_mempool struct alongside the *pool
> > >      void *pool;
> > >      void *pool_config;
> > > 
> > > Then add a param to the rte_mempool_set_handler function:
> > > int
> > > rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void
> > > *pool_config)
> > IMO, Maybe we need to have _set_ and _get_.So I think we can have
> > two separate callback in external pool-manger for that if required.
> > IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23)
> > for the configuration as mentioned above and add the new callbacks for
> > set and get when required?
> 
> OK, We'll keep the config to the 8 bits of the flags for now. That will also
> mean I won't
> add the pool_config void pointer either (for the moment)

OK to me.

> 
> > > > 2) IMO, It is better to change void *pool in struct rte_mempool to
> > > > anonymous union type, something like below, so that mempool
> > > > implementation can choose the best type.
> > > > 	union {
> > > > 		void *pool;
> > > > 		uint64_t val;
> > > > 	}
> > > Could we do this by using the union for the *pool_config suggested above,
> > > would that give
> > > you what you need?
> > It would be an extra overhead for external pool manager to _alloc_ memory
> > and store the allocated pointer in mempool struct(as *pool) and use pool for
> > pointing other data structures as some implementation need only
> > limited bytes to store the external pool manager specific context.
> > 
> > In order to fix this problem, We may classify fast path and slow path
> > elements in struct rte_mempool and move all fast path elements in first
> > cache line and create an empty opaque space in the remaining bytes in the
> > cache line so that if the external pool manager needs only limited space
> > then it is not required to allocate the separate memory to save the
> > per core cache  in fast-path
> > 
> > something like below,
> > union {
> > 	void *pool;
> > 	uint64_t val;
> > 	uint8_t extra_mem[16] // available free bytes in fast path cache line
> > 
> > }
> 
> Something for the future, perhaps? Will the 8-bits in the flags suffice for
> now?

OK. But simple anonymous union for same type should be OK add now? Not
much change I believe, If its difficult then postpone it

union {
	void *pool;
	uint64_t val;
}

> 
> 
> > Other points,
> > 
> > 1) Is it possible to remove unused is_mp in  __mempool_put_bulk
> > function as it is just a internal function.
> 
> Fixed

__mempool_get_bulk too.

 
> > 2) Considering "get" and "put" are the fast-path callbacks for
> > pool-manger, Is it possible to avoid the extra overhead of the following
> > _load_ and additional cache line on each call,
> > rte_mempool_handler_table.handler[handler_idx]
> > 
> > I understand it is for multiprocess support but I am thing can we
> > introduce something like ethernet API support for multiprocess and
> > resolve "put" and "get" functions pointer on init and store in
> > struct mempool. Some thinking like
> > 
> > file: drivers/net/ixgbe/ixgbe_ethdev.c
> > search for if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> 
> I'll look at this one before posting the next version of the patch (soon).
> :)

OK

> 
> 
> > Jerin
> > 
> Thanks for your input on this, much appreciated.
> Dave.
>
  

Hi,

On 05/31/2016 03:47 PM, Hunt, David wrote:
> On 5/31/2016 1:06 PM, Jan Viktorin wrote:
>> On Tue, 31 May 2016 10:09:42 +0100
>> "Hunt, David" <david.hunt@intel.com> wrote:
>>
>>> The *p pointer is the opaque data for a given mempool handler (ring,
>>> array, linked list, etc)
>> Again, doc comments...
>>
>> I don't like the obj_table representation to be an array of void *. I
>> could see
>> it already in DPDK for defining Ethernet driver queues, so, it's
>> probably not
>> an issue. I just say, I would prefer some basic type safety like
>>
>> struct rte_mempool_obj {
>>     void *p;
>> };
>>
>> Is there somebody with different opinions?
>>
>> [...]
> 
> Comments added. I've left as a void* for the moment.

Jan, could you please detail why you think having a
rte_mempool_obj structure brings more safety?

For now, I'm in favor of keeping the array of void *, because
that's what we use in other mempool or ring functions.


>>>>> +/** Structure defining a mempool handler. */
>>>> Later in the text, I suggested to rename rte_mempool_handler to
>>>> rte_mempool_ops.
>>>> I believe that it explains the purpose of this struct better. It
>>>> would improve
>>>> consistency in function names (the *_ext_* mark is very strange and
>>>> inconsistent).
>>> I agree. I've gone through all the code and renamed to
>>> rte_mempool_handler_ops.
>> Ok. I meant rte_mempool_ops because I find the word "handler" to be
>> redundant.
> 
> I prefer the use of the word handler, unless others also have opinions
> either way?

Well, I think rte_mempool_ops is clear enough, and shorter,
so I'd vote for it.


Regards,
Olivier
  

Hi,

On 05/31/2016 06:03 PM, Jerin Jacob wrote:
> On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote:
>>
>>
>> On 5/31/2016 9:53 AM, Jerin Jacob wrote:
>>> On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
>>>> New mempool handlers will use rte_mempool_create_empty(),
>>>> rte_mempool_set_handler(),
>>>> then rte_mempool_populate_*(). These three functions are new to this
>>>> release, to no problem
>>> Having separate APIs for external pool-manager create is worrisome in
>>> application perspective. Is it possible to have rte_mempool_[xmem]_create
>>> for the both external and existing SW pool manager and make
>>> rte_mempool_create_empty and rte_mempool_populate_*  internal functions.
>>>
>>> IMO, We can do that by selecting  specific rte_mempool_set_handler()
>>> based on _flags_ encoding, something like below
>>>
>>> bit 0 - 16   // generic bits uses across all the pool managers
>>> bit 16 - 23  // pool handler specific flags bits
>>> bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
>>> pool managers, For backward compatibility, make '0'(in 24-31) to select
>>> existing SW pool manager.
>>>
>>> and applications can choose the handlers by selecting the flag in
>>> rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
>>> applications to choose the pool handler from command line etc in future.
>>
>> There might be issues with the 8-bit handler number, as we'd have to add an
>> api call to
>> first get the index of a given hander by name, then OR it into the flags.
>> That would mean
>> also extra API calls for the non-default external handlers. I do agree with
>> the handler-specific
>> bits though.
> 
> That would be an internal API(upper 8 bits to handler name). Right ?
> Seems to be OK for me.
> 
>>
>> Having the _empty and _set_handler  APIs seems to me to be OK for the
>> moment. Maybe Olivier could comment?
>>
> 
> But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe
> it is better reduce the public API in spec where ever possible ?
> 
> Maybe Olivier could comment ?

Well, I think having 3 different functions is not a problem if the API
is clearer.

In my opinion, the following:
	rte_mempool_create_empty()
	rte_mempool_set_handler()
	rte_mempool_populate()

is clearer than:
	rte_mempool_create(15 args)

Splitting the flags into 3 groups, with one not beeing flags but a
pool handler number looks overcomplicated from a user perspective.

>>> and we can remove "mbuf: get default mempool handler from configuration"
>>> change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
>>> the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
>>>
>>> What do you think?
>>
>> The "configuration" patch is to allow users to quickly change the mempool
>> handler
>> by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
>> handler. It could just as easily be left out and use the rte_mempool_create.
>>
> 
> Yes, I understand, but I am trying to avoid build time constant. IMO, It
> would be better by default RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is not
> defined in config. and for quick change developers can introduce the build 
> with RTE_MBUF_DEFAULT_MEMPOOL_HANDLER="specific handler"

My understanding of the compile-time configuration option was
to allow a specific architecture to define a specific hw-assisted
handler by default.

Indeed, if there is no such need for now, we may remove it. But
we need a way to select another handler, at least in test-pmd
(in command line arguments?).


>>>> to add a parameter to one of them for the config data. Also since we're
>>>> adding some new
>>>> elements to the mempool structure, how about we add a new pointer for a void
>>>> pointer to a
>>>> config data structure, as defined by the handler.
>>>>
>>>> So, new element in rte_mempool struct alongside the *pool
>>>>      void *pool;
>>>>      void *pool_config;
>>>>
>>>> Then add a param to the rte_mempool_set_handler function:
>>>> int
>>>> rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void
>>>> *pool_config)
>>> IMO, Maybe we need to have _set_ and _get_.So I think we can have
>>> two separate callback in external pool-manger for that if required.
>>> IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23)
>>> for the configuration as mentioned above and add the new callbacks for
>>> set and get when required?
>>
>> OK, We'll keep the config to the 8 bits of the flags for now. That will also
>> mean I won't
>> add the pool_config void pointer either (for the moment)
> 
> OK to me.

I'm not sure I'm getting it. Does it mean having something like
this ?

rte_mempool_set_handler(struct rte_mempool *mp, const char *name,
	unsigned int flags)

Or does it mean some of the flags passed to rte_mempool_create*()
will be specific to some handlers?


Before adding handler-specific flags or config, can we ensure we
will need them? What kind of handler-specific configuration/flags
do you think we will need? Just an idea: what about having a global
configuration for all mempools using a given handler?



>>>>> 2) IMO, It is better to change void *pool in struct rte_mempool to
>>>>> anonymous union type, something like below, so that mempool
>>>>> implementation can choose the best type.
>>>>> 	union {
>>>>> 		void *pool;
>>>>> 		uint64_t val;
>>>>> 	}
>>>> Could we do this by using the union for the *pool_config suggested above,
>>>> would that give
>>>> you what you need?
>>> It would be an extra overhead for external pool manager to _alloc_ memory
>>> and store the allocated pointer in mempool struct(as *pool) and use pool for
>>> pointing other data structures as some implementation need only
>>> limited bytes to store the external pool manager specific context.
>>>
>>> In order to fix this problem, We may classify fast path and slow path
>>> elements in struct rte_mempool and move all fast path elements in first
>>> cache line and create an empty opaque space in the remaining bytes in the
>>> cache line so that if the external pool manager needs only limited space
>>> then it is not required to allocate the separate memory to save the
>>> per core cache  in fast-path
>>>
>>> something like below,
>>> union {
>>> 	void *pool;
>>> 	uint64_t val;
>>> 	uint8_t extra_mem[16] // available free bytes in fast path cache line
>>>
>>> }
>>
>> Something for the future, perhaps? Will the 8-bits in the flags suffice for
>> now?
> 
> OK. But simple anonymous union for same type should be OK add now? Not
> much change I believe, If its difficult then postpone it
> 
> union {
> 	void *pool;
> 	uint64_t val;
> }

I'm ok with the simple union with (void *) and (uint64_t).
Maybe "val" should be replaced by something more appropriate.
Is "pool_id" a better name?


Thanks David for working on this, and thanks Jerin and Jan for
the good comments and suggestions!

Regards
Olivier
  

On Tue, May 31, 2016 at 10:41:00PM +0200, Olivier MATZ wrote:
> Hi,
> 
> On 05/31/2016 06:03 PM, Jerin Jacob wrote:
> > On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote:
> >>
> >>
> >> On 5/31/2016 9:53 AM, Jerin Jacob wrote:
> >>> On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
> >>>> New mempool handlers will use rte_mempool_create_empty(),
> >>>> rte_mempool_set_handler(),
> >>>> then rte_mempool_populate_*(). These three functions are new to this
> >>>> release, to no problem
> >>> Having separate APIs for external pool-manager create is worrisome in
> >>> application perspective. Is it possible to have rte_mempool_[xmem]_create
> >>> for the both external and existing SW pool manager and make
> >>> rte_mempool_create_empty and rte_mempool_populate_*  internal functions.
> >>>
> >>> IMO, We can do that by selecting  specific rte_mempool_set_handler()
> >>> based on _flags_ encoding, something like below
> >>>
> >>> bit 0 - 16   // generic bits uses across all the pool managers
> >>> bit 16 - 23  // pool handler specific flags bits
> >>> bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
> >>> pool managers, For backward compatibility, make '0'(in 24-31) to select
> >>> existing SW pool manager.
> >>>
> >>> and applications can choose the handlers by selecting the flag in
> >>> rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
> >>> applications to choose the pool handler from command line etc in future.
> >>
> >> There might be issues with the 8-bit handler number, as we'd have to add an
> >> api call to
> >> first get the index of a given hander by name, then OR it into the flags.
> >> That would mean
> >> also extra API calls for the non-default external handlers. I do agree with
> >> the handler-specific
> >> bits though.
> > 
> > That would be an internal API(upper 8 bits to handler name). Right ?
> > Seems to be OK for me.
> > 
> >>
> >> Having the _empty and _set_handler  APIs seems to me to be OK for the
> >> moment. Maybe Olivier could comment?
> >>
> > 
> > But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe
> > it is better reduce the public API in spec where ever possible ?
> > 
> > Maybe Olivier could comment ?
> 
> Well, I think having 3 different functions is not a problem if the API
> is clearer.
> 
> In my opinion, the following:
> 	rte_mempool_create_empty()
> 	rte_mempool_set_handler()
> 	rte_mempool_populate()
> 
> is clearer than:
> 	rte_mempool_create(15 args)

But proposed scheme is not adding any new arguments to
rte_mempool_create. It just extending the existing flag.

rte_mempool_create(15 args) is still their as API for internal pool
creation.

> 
> Splitting the flags into 3 groups, with one not beeing flags but a
> pool handler number looks overcomplicated from a user perspective.

I am concerned with seem less integration with existing applications,
IMO, Its not worth having separate functions for external vs internal
pool creation for application(now each every applications has to added this
logic every where for no good reason), just my 2 cents.

> 
> >>> and we can remove "mbuf: get default mempool handler from configuration"
> >>> change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
> >>> the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
> >>>
> >>> What do you think?
> >>
> >> The "configuration" patch is to allow users to quickly change the mempool
> >> handler
> >> by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
> >> handler. It could just as easily be left out and use the rte_mempool_create.
> >>
> > 
> > Yes, I understand, but I am trying to avoid build time constant. IMO, It
> > would be better by default RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is not
> > defined in config. and for quick change developers can introduce the build 
> > with RTE_MBUF_DEFAULT_MEMPOOL_HANDLER="specific handler"
> 
> My understanding of the compile-time configuration option was
> to allow a specific architecture to define a specific hw-assisted
> handler by default.
> 
> Indeed, if there is no such need for now, we may remove it. But
> we need a way to select another handler, at least in test-pmd
> (in command line arguments?).

like txflags in testpmd, IMO, mempool flags will help to select the handlers
seamlessly as suggest above.

If we are _not_ taking the flags based selection scheme then it makes to
keep RTE_MBUF_DEFAULT_MEMPOOL_HANDLER

> 
> 
> >>>> to add a parameter to one of them for the config data. Also since we're
> >>>> adding some new
> >>>> elements to the mempool structure, how about we add a new pointer for a void
> >>>> pointer to a
> >>>> config data structure, as defined by the handler.
> >>>>
> >>>> So, new element in rte_mempool struct alongside the *pool
> >>>>      void *pool;
> >>>>      void *pool_config;
> >>>>
> >>>> Then add a param to the rte_mempool_set_handler function:
> >>>> int
> >>>> rte_mempool_set_handler(struct rte_mempool *mp, const char *name, void
> >>>> *pool_config)
> >>> IMO, Maybe we need to have _set_ and _get_.So I think we can have
> >>> two separate callback in external pool-manger for that if required.
> >>> IMO, For now, We can live with pool manager specific 8 bits(bit 16 -23)
> >>> for the configuration as mentioned above and add the new callbacks for
> >>> set and get when required?
> >>
> >> OK, We'll keep the config to the 8 bits of the flags for now. That will also
> >> mean I won't
> >> add the pool_config void pointer either (for the moment)
> > 
> > OK to me.
> 
> I'm not sure I'm getting it. Does it mean having something like
> this ?
> 
> rte_mempool_set_handler(struct rte_mempool *mp, const char *name,
> 	unsigned int flags)
> 
> Or does it mean some of the flags passed to rte_mempool_create*()
> will be specific to some handlers?
> 
> 
> Before adding handler-specific flags or config, can we ensure we
> will need them? What kind of handler-specific configuration/flags
> do you think we will need? Just an idea: what about having a global
> configuration for all mempools using a given handler?

We may need to configure external pool manager like don't free the
packets back to pool  after it has been send out(just an example of
valid external HW pool manager configuration)

> 
> 
> 
> >>>>> 2) IMO, It is better to change void *pool in struct rte_mempool to
> >>>>> anonymous union type, something like below, so that mempool
> >>>>> implementation can choose the best type.
> >>>>> 	union {
> >>>>> 		void *pool;
> >>>>> 		uint64_t val;
> >>>>> 	}
> >>>> Could we do this by using the union for the *pool_config suggested above,
> >>>> would that give
> >>>> you what you need?
> >>> It would be an extra overhead for external pool manager to _alloc_ memory
> >>> and store the allocated pointer in mempool struct(as *pool) and use pool for
> >>> pointing other data structures as some implementation need only
> >>> limited bytes to store the external pool manager specific context.
> >>>
> >>> In order to fix this problem, We may classify fast path and slow path
> >>> elements in struct rte_mempool and move all fast path elements in first
> >>> cache line and create an empty opaque space in the remaining bytes in the
> >>> cache line so that if the external pool manager needs only limited space
> >>> then it is not required to allocate the separate memory to save the
> >>> per core cache  in fast-path
> >>>
> >>> something like below,
> >>> union {
> >>> 	void *pool;
> >>> 	uint64_t val;
> >>> 	uint8_t extra_mem[16] // available free bytes in fast path cache line
> >>>
> >>> }
> >>
> >> Something for the future, perhaps? Will the 8-bits in the flags suffice for
> >> now?
> > 
> > OK. But simple anonymous union for same type should be OK add now? Not
> > much change I believe, If its difficult then postpone it
> > 
> > union {
> > 	void *pool;
> > 	uint64_t val;
> > }
> 
> I'm ok with the simple union with (void *) and (uint64_t).
> Maybe "val" should be replaced by something more appropriate.
> Is "pool_id" a better name?

How about "opaque"?

> 
> 
> Thanks David for working on this, and thanks Jerin and Jan for
> the good comments and suggestions!
> 
> Regards
> Olivier
  

On 5/31/2016 9:40 PM, Olivier MATZ wrote:

[...]

>>>>>> +/** Structure defining a mempool handler. */
>>>>> Later in the text, I suggested to rename rte_mempool_handler to
>>>>> rte_mempool_ops.
>>>>> I believe that it explains the purpose of this struct better. It
>>>>> would improve
>>>>> consistency in function names (the *_ext_* mark is very strange and
>>>>> inconsistent).
>>>> I agree. I've gone through all the code and renamed to
>>>> rte_mempool_handler_ops.
>>> Ok. I meant rte_mempool_ops because I find the word "handler" to be
>>> redundant.
>> I prefer the use of the word handler, unless others also have opinions
>> either way?
> Well, I think rte_mempool_ops is clear enough, and shorter,
> so I'd vote for it.
>

OK, I've just changed it. It will be in next revision. :)

Regards,
Dave.
  

On 5/31/2016 10:11 PM, Jerin Jacob wrote:
> On Tue, May 31, 2016 at 10:41:00PM +0200, Olivier MATZ wrote:
>> Hi,
>>
>> On 05/31/2016 06:03 PM, Jerin Jacob wrote:
>>> On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote:
>>>>
>>>> On 5/31/2016 9:53 AM, Jerin Jacob wrote:
>>>>> On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
>>>>>> New mempool handlers will use rte_mempool_create_empty(),
>>>>>> rte_mempool_set_handler(),
>>>>>> then rte_mempool_populate_*(). These three functions are new to this
>>>>>> release, to no problem
>>>>> Having separate APIs for external pool-manager create is worrisome in
>>>>> application perspective. Is it possible to have rte_mempool_[xmem]_create
>>>>> for the both external and existing SW pool manager and make
>>>>> rte_mempool_create_empty and rte_mempool_populate_*  internal functions.
>>>>>
>>>>> IMO, We can do that by selecting  specific rte_mempool_set_handler()
>>>>> based on _flags_ encoding, something like below
>>>>>
>>>>> bit 0 - 16   // generic bits uses across all the pool managers
>>>>> bit 16 - 23  // pool handler specific flags bits
>>>>> bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
>>>>> pool managers, For backward compatibility, make '0'(in 24-31) to select
>>>>> existing SW pool manager.
>>>>>
>>>>> and applications can choose the handlers by selecting the flag in
>>>>> rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
>>>>> applications to choose the pool handler from command line etc in future.
>>>> There might be issues with the 8-bit handler number, as we'd have to add an
>>>> api call to
>>>> first get the index of a given hander by name, then OR it into the flags.
>>>> That would mean
>>>> also extra API calls for the non-default external handlers. I do agree with
>>>> the handler-specific
>>>> bits though.
>>> That would be an internal API(upper 8 bits to handler name). Right ?
>>> Seems to be OK for me.
>>>
>>>> Having the _empty and _set_handler  APIs seems to me to be OK for the
>>>> moment. Maybe Olivier could comment?
>>>>
>>> But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe
>>> it is better reduce the public API in spec where ever possible ?
>>>
>>> Maybe Olivier could comment ?
>> Well, I think having 3 different functions is not a problem if the API
>> is clearer.
>>
>> In my opinion, the following:
>> 	rte_mempool_create_empty()
>> 	rte_mempool_set_handler()
>> 	rte_mempool_populate()
>>
>> is clearer than:
>> 	rte_mempool_create(15 args)
> But proposed scheme is not adding any new arguments to
> rte_mempool_create. It just extending the existing flag.
>
> rte_mempool_create(15 args) is still their as API for internal pool
> creation.
>
>> Splitting the flags into 3 groups, with one not beeing flags but a
>> pool handler number looks overcomplicated from a user perspective.
> I am concerned with seem less integration with existing applications,
> IMO, Its not worth having separate functions for external vs internal
> pool creation for application(now each every applications has to added this
> logic every where for no good reason), just my 2 cents.

I think that there is always going to be some  extra code in the 
applications
  that want to use an external mempool. The _set_handler approach does
create, set_hander, populate. The Flags method queries the handler list to
get the index, sets the flags bits, then calls create. Both methods will 
work.

But I think the _set_handler approach is more user friendly, therefore that
it the method I would lean towards.

>>>>> and we can remove "mbuf: get default mempool handler from configuration"
>>>>> change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
>>>>> the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
>>>>>
>>>>> What do you think?
>>>> The "configuration" patch is to allow users to quickly change the mempool
>>>> handler
>>>> by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
>>>> handler. It could just as easily be left out and use the rte_mempool_create.
>>>>
>>> Yes, I understand, but I am trying to avoid build time constant. IMO, It
>>> would be better by default RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is not
>>> defined in config. and for quick change developers can introduce the build
>>> with RTE_MBUF_DEFAULT_MEMPOOL_HANDLER="specific handler"
>> My understanding of the compile-time configuration option was
>> to allow a specific architecture to define a specific hw-assisted
>> handler by default.
>>
>> Indeed, if there is no such need for now, we may remove it. But
>> we need a way to select another handler, at least in test-pmd
>> (in command line arguments?).
> like txflags in testpmd, IMO, mempool flags will help to select the handlers
> seamlessly as suggest above.
>
> If we are _not_ taking the flags based selection scheme then it makes to
> keep RTE_MBUF_DEFAULT_MEMPOOL_HANDLER

see comment above

>>>>>>> 2) IMO, It is better to change void *pool in struct rte_mempool to
>>>>>>> anonymous union type, something like below, so that mempool
>>>>>>> implementation can choose the best type.
>>>>>>> 	union {
>>>>>>> 		void *pool;
>>>>>>> 		uint64_t val;
>>>>>>> 	}
>>>>>> Could we do this by using the union for the *pool_config suggested above,
>>>>>> would that give
>>>>>> you what you need?
>>>>> It would be an extra overhead for external pool manager to _alloc_ memory
>>>>> and store the allocated pointer in mempool struct(as *pool) and use pool for
>>>>> pointing other data structures as some implementation need only
>>>>> limited bytes to store the external pool manager specific context.
>>>>>
>>>>> In order to fix this problem, We may classify fast path and slow path
>>>>> elements in struct rte_mempool and move all fast path elements in first
>>>>> cache line and create an empty opaque space in the remaining bytes in the
>>>>> cache line so that if the external pool manager needs only limited space
>>>>> then it is not required to allocate the separate memory to save the
>>>>> per core cache  in fast-path
>>>>>
>>>>> something like below,
>>>>> union {
>>>>> 	void *pool;
>>>>> 	uint64_t val;
>>>>> 	uint8_t extra_mem[16] // available free bytes in fast path cache line
>>>>>
>>>>> }
>>>> Something for the future, perhaps? Will the 8-bits in the flags suffice for
>>>> now?
>>> OK. But simple anonymous union for same type should be OK add now? Not
>>> much change I believe, If its difficult then postpone it
>>>
>>> union {
>>> 	void *pool;
>>> 	uint64_t val;
>>> }
>> I'm ok with the simple union with (void *) and (uint64_t).
>> Maybe "val" should be replaced by something more appropriate.
>> Is "pool_id" a better name?
> How about "opaque"?

I think I would lean towards pool_id in this case.


Regards,
David.
  

On Wed, Jun 01, 2016 at 11:46:20AM +0100, Hunt, David wrote:
> 
> 
> On 5/31/2016 10:11 PM, Jerin Jacob wrote:
> > On Tue, May 31, 2016 at 10:41:00PM +0200, Olivier MATZ wrote:
> > > Hi,
> > > 
> > > On 05/31/2016 06:03 PM, Jerin Jacob wrote:
> > > > On Tue, May 31, 2016 at 04:37:02PM +0100, Hunt, David wrote:
> > > > > 
> > > > > On 5/31/2016 9:53 AM, Jerin Jacob wrote:
> > > > > > On Mon, May 30, 2016 at 12:27:26PM +0100, Hunt, David wrote:
> > > > > > > New mempool handlers will use rte_mempool_create_empty(),
> > > > > > > rte_mempool_set_handler(),
> > > > > > > then rte_mempool_populate_*(). These three functions are new to this
> > > > > > > release, to no problem
> > > > > > Having separate APIs for external pool-manager create is worrisome in
> > > > > > application perspective. Is it possible to have rte_mempool_[xmem]_create
> > > > > > for the both external and existing SW pool manager and make
> > > > > > rte_mempool_create_empty and rte_mempool_populate_*  internal functions.
> > > > > > 
> > > > > > IMO, We can do that by selecting  specific rte_mempool_set_handler()
> > > > > > based on _flags_ encoding, something like below
> > > > > > 
> > > > > > bit 0 - 16   // generic bits uses across all the pool managers
> > > > > > bit 16 - 23  // pool handler specific flags bits
> > > > > > bit 24 - 31  // to select the specific pool manager(Up to 256 different flavors of
> > > > > > pool managers, For backward compatibility, make '0'(in 24-31) to select
> > > > > > existing SW pool manager.
> > > > > > 
> > > > > > and applications can choose the handlers by selecting the flag in
> > > > > > rte_mempool_[xmem]_create, That way it will be easy in testpmd or any other
> > > > > > applications to choose the pool handler from command line etc in future.
> > > > > There might be issues with the 8-bit handler number, as we'd have to add an
> > > > > api call to
> > > > > first get the index of a given hander by name, then OR it into the flags.
> > > > > That would mean
> > > > > also extra API calls for the non-default external handlers. I do agree with
> > > > > the handler-specific
> > > > > bits though.
> > > > That would be an internal API(upper 8 bits to handler name). Right ?
> > > > Seems to be OK for me.
> > > > 
> > > > > Having the _empty and _set_handler  APIs seems to me to be OK for the
> > > > > moment. Maybe Olivier could comment?
> > > > > 
> > > > But need 3 APIs. Right? _empty , _set_handler and _populate ? I believe
> > > > it is better reduce the public API in spec where ever possible ?
> > > > 
> > > > Maybe Olivier could comment ?
> > > Well, I think having 3 different functions is not a problem if the API
> > > is clearer.
> > > 
> > > In my opinion, the following:
> > > 	rte_mempool_create_empty()
> > > 	rte_mempool_set_handler()
> > > 	rte_mempool_populate()
> > > 
> > > is clearer than:
> > > 	rte_mempool_create(15 args)
> > But proposed scheme is not adding any new arguments to
> > rte_mempool_create. It just extending the existing flag.
> > 
> > rte_mempool_create(15 args) is still their as API for internal pool
> > creation.
> > 
> > > Splitting the flags into 3 groups, with one not beeing flags but a
> > > pool handler number looks overcomplicated from a user perspective.
> > I am concerned with seem less integration with existing applications,
> > IMO, Its not worth having separate functions for external vs internal
> > pool creation for application(now each every applications has to added this
> > logic every where for no good reason), just my 2 cents.
> 
> I think that there is always going to be some  extra code in the
> applications
>  that want to use an external mempool. The _set_handler approach does
> create, set_hander, populate. The Flags method queries the handler list to
> get the index, sets the flags bits, then calls create. Both methods will
> work.

I was suggesting flags like TXQ in ethdev where application just
selects the mode. Not sure why application has to get the index first.

some thing like,
#define ETH_TXQ_FLAGS_NOMULTSEGS 0x0001 /**< nb_segs=1 for all mbufs */
#define ETH_TXQ_FLAGS_NOREFCOUNT 0x0002 /**< refcnt can be ignored */
#define ETH_TXQ_FLAGS_NOMULTMEMP 0x0004 /**< all bufs come from same mempool */ 

Anyway, Looks like  no one else much bothered about external pool
manger creation API being different. So, I given up. No objections from my side :-)

> 
> But I think the _set_handler approach is more user friendly, therefore that
> it the method I would lean towards.
> 
> > > > > > and we can remove "mbuf: get default mempool handler from configuration"
> > > > > > change-set OR just add if RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is defined then set
> > > > > > the same with rte_mempool_set_handler in rte_mempool_[xmem]_create.
> > > > > > 
> > > > > > What do you think?
> > > > > The "configuration" patch is to allow users to quickly change the mempool
> > > > > handler
> > > > > by changing RTE_MBUF_DEFAULT_MEMPOOL_HANDLER to another string of a known
> > > > > handler. It could just as easily be left out and use the rte_mempool_create.
> > > > > 
> > > > Yes, I understand, but I am trying to avoid build time constant. IMO, It
> > > > would be better by default RTE_MBUF_DEFAULT_MEMPOOL_HANDLER is not
> > > > defined in config. and for quick change developers can introduce the build
> > > > with RTE_MBUF_DEFAULT_MEMPOOL_HANDLER="specific handler"
> > > My understanding of the compile-time configuration option was
> > > to allow a specific architecture to define a specific hw-assisted
> > > handler by default.
> > > 
> > > Indeed, if there is no such need for now, we may remove it. But
> > > we need a way to select another handler, at least in test-pmd
> > > (in command line arguments?).
> > like txflags in testpmd, IMO, mempool flags will help to select the handlers
> > seamlessly as suggest above.
> > 
> > If we are _not_ taking the flags based selection scheme then it makes to
> > keep RTE_MBUF_DEFAULT_MEMPOOL_HANDLER
> 
> see comment above

Try to add some means to select the external handler for existing
applications so that we can test existing applications in different modes.

Thanks,
Jerin

> 
> > > > > > > > 2) IMO, It is better to change void *pool in struct rte_mempool to
> > > > > > > > anonymous union type, something like below, so that mempool
> > > > > > > > implementation can choose the best type.
> > > > > > > > 	union {
> > > > > > > > 		void *pool;
> > > > > > > > 		uint64_t val;
> > > > > > > > 	}
> > > > > > > Could we do this by using the union for the *pool_config suggested above,
> > > > > > > would that give
> > > > > > > you what you need?
> > > > > > It would be an extra overhead for external pool manager to _alloc_ memory
> > > > > > and store the allocated pointer in mempool struct(as *pool) and use pool for
> > > > > > pointing other data structures as some implementation need only
> > > > > > limited bytes to store the external pool manager specific context.
> > > > > > 
> > > > > > In order to fix this problem, We may classify fast path and slow path
> > > > > > elements in struct rte_mempool and move all fast path elements in first
> > > > > > cache line and create an empty opaque space in the remaining bytes in the
> > > > > > cache line so that if the external pool manager needs only limited space
> > > > > > then it is not required to allocate the separate memory to save the
> > > > > > per core cache  in fast-path
> > > > > > 
> > > > > > something like below,
> > > > > > union {
> > > > > > 	void *pool;
> > > > > > 	uint64_t val;
> > > > > > 	uint8_t extra_mem[16] // available free bytes in fast path cache line
> > > > > > 
> > > > > > }
> > > > > Something for the future, perhaps? Will the 8-bits in the flags suffice for
> > > > > now?
> > > > OK. But simple anonymous union for same type should be OK add now? Not
> > > > much change I believe, If its difficult then postpone it
> > > > 
> > > > union {
> > > > 	void *pool;
> > > > 	uint64_t val;
> > > > }
> > > I'm ok with the simple union with (void *) and (uint64_t).
> > > Maybe "val" should be replaced by something more appropriate.
> > > Is "pool_id" a better name?
> > How about "opaque"?
> 
> I think I would lean towards pool_id in this case.
> 
> 
> Regards,
> David.
  

On Tue, 31 May 2016 22:40:59 +0200
Olivier MATZ <olivier.matz@6wind.com> wrote:

> Hi,
> 
> On 05/31/2016 03:47 PM, Hunt, David wrote:
> > On 5/31/2016 1:06 PM, Jan Viktorin wrote:  
> >> On Tue, 31 May 2016 10:09:42 +0100
> >> "Hunt, David" <david.hunt@intel.com> wrote:
> >>  
> >>> The *p pointer is the opaque data for a given mempool handler (ring,
> >>> array, linked list, etc)  
> >> Again, doc comments...
> >>
> >> I don't like the obj_table representation to be an array of void *. I
> >> could see
> >> it already in DPDK for defining Ethernet driver queues, so, it's
> >> probably not
> >> an issue. I just say, I would prefer some basic type safety like
> >>
> >> struct rte_mempool_obj {
> >>     void *p;
> >> };
> >>
> >> Is there somebody with different opinions?
> >>
> >> [...]  
> > 
> > Comments added. I've left as a void* for the moment.  
> 
> Jan, could you please detail why you think having a
> rte_mempool_obj structure brings more safety?

First, void * does not say anything about the purpose of the argument.
So, anybody, who is not familiar with the mempool internals would be
lost for a while (as I was when studying the Ethernet queue API of DPDK).

The type safety (in C, LoL) here is in the means of messing up order of arguments.
When there are more void * args, you can accidently pass something wrong.
DPDK has quite strict compiler settings, however, I don't consider it to be
a good practice in general.

When you have a named struct or a typedef, its definition usually contains doc
comments describing its purposes. Nobody usually writes good doc comments for
void * arguments of functions.

> 
> For now, I'm in favor of keeping the array of void *, because
> that's what we use in other mempool or ring functions.

It was just a suggestion... I don't consider this to be an issue (as stated
earlier).

Jan

> 
> 
> >>>>> +/** Structure defining a mempool handler. */  
> >>>> Later in the text, I suggested to rename rte_mempool_handler to
> >>>> rte_mempool_ops.
> >>>> I believe that it explains the purpose of this struct better. It
> >>>> would improve
> >>>> consistency in function names (the *_ext_* mark is very strange and
> >>>> inconsistent).  
> >>> I agree. I've gone through all the code and renamed to
> >>> rte_mempool_handler_ops.  
> >> Ok. I meant rte_mempool_ops because I find the word "handler" to be
> >> redundant.  
> > 
> > I prefer the use of the word handler, unless others also have opinions
> > either way?  
> 
> Well, I think rte_mempool_ops is clear enough, and shorter,
> so I'd vote for it.
> 
> 
> Regards,
> Olivier