Re: [PATCH 2/5] memool: add stack (lifo) based external mempool handler

[Olivier MATZ <olivier.matz@6wind.com>]

Hi,

> [PATCH 2/5] memool: add stack (lifo) based external mempool handler

typo in the patch title: memool -> mempool


On 01/26/2016 06:25 PM, David Hunt wrote:
> adds a simple stack based mempool handler
>
> Signed-off-by: David Hunt <david.hunt@intel.com>

What is the purpose of this mempool handler?

Is it an example or is it something that could be useful for
dpdk applications?

If it's just an example, I think we could move this code
in app/test.

> --- a/app/test/test_mempool_perf.c
> +++ b/app/test/test_mempool_perf.c
> @@ -52,7 +52,6 @@
>   #include <rte_lcore.h>
>   #include <rte_atomic.h>
>   #include <rte_branch_prediction.h>
> -#include <rte_ring.h>
>   #include <rte_mempool.h>
>   #include <rte_spinlock.h>
>   #include <rte_malloc.h>

Is this change related?



> +struct rte_mempool_common_stack {
> +	/* Spinlock to protect access */
> +	rte_spinlock_t sl;
> +
> +	uint32_t size;
> +	uint32_t len;
> +	void *objs[];
> +
> +#ifdef RTE_LIBRTE_MEMPOOL_DEBUG
> +#endif

There is nothing inside the #ifdef


> +static void *
> +common_stack_alloc(struct rte_mempool *mp,
> +		const char *name, unsigned n, int socket_id, unsigned flags)
> +{
> +	struct rte_mempool_common_stack *s;
> +	char stack_name[RTE_RING_NAMESIZE];
> +
> +	int size = sizeof(*s) + (n+16)*sizeof(void *);
> +
> +	flags = flags;
> +
> +	/* Allocate our local memory structure */
> +	snprintf(stack_name, sizeof(stack_name), "%s-common-stack", name);
> +	s = rte_zmalloc_socket(stack_name,
> +					size, RTE_CACHE_LINE_SIZE, socket_id);
> +	if (s == NULL) {
> +		RTE_LOG(ERR, MEMPOOL, "Cannot allocate stack!\n");
> +		return NULL;
> +	}
> +
> +	/* And the spinlock we use to protect access */
> +	rte_spinlock_init(&s->sl);
> +
> +	s->size = n;
> +	mp->rt_pool = (void *) s;
> +	mp->handler_idx = rte_get_mempool_handler("stack");
> +
> +	return (void *) s;
> +}

The explicit casts could be removed I think.


> +
> +static int common_stack_put(void *p, void * const *obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_common_stack *s =
> +				(struct rte_mempool_common_stack *)p;

indent issue (same in get() and count())

> +	void **cache_objs;
> +	unsigned index;
> +
> +	/* Acquire lock */
> +	rte_spinlock_lock(&s->sl);
> +	cache_objs = &s->objs[s->len];
> +
> +	/* Is there sufficient space in the stack ? */
> +	if ((s->len + n) > s->size) {
> +		rte_spinlock_unlock(&s->sl);
> +		return -ENOENT;
> +	}

I think this cannot happen as there is a check in the get().
I wonder if a rte_panic() wouldn't be better here.



Regards,
Olivier