Re: [dpdk-dev] [PATCH 1/2] mempool: add stack (fifo) mempool handler

[Stephen Hemminger <stephen@networkplumber.org>]

Overall, this is ok, but why can't it be the default?
Lots of little things should be cleaned up


> +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

Useless remove it

> +};
> +
> +static void *
> +common_stack_alloc(struct rte_mempool *mp)
> +{
> +	struct rte_mempool_common_stack *s;
> +	char stack_name[RTE_RING_NAMESIZE];
> +	unsigned n = mp->size;
> +	int size = sizeof(*s) + (n+16)*sizeof(void*);
> +
> +	/* Allocate our local memory structure */
> +	snprintf(stack_name, sizeof(stack_name), "%s-common-stack", mp->name);
> +	s = rte_zmalloc_socket(stack_name,
The name for zmalloc is ignored in current code, why bother making it unique.

> +			size,
> +			RTE_CACHE_LINE_SIZE,
> +			mp->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->pool = (void *) s;
Since pool is void *, no need for a cast here

> +	rte_mempool_set_handler(mp, "stack");
> +
> +	return (void *) s;
> +}
> +
> +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;
> +	void **cache_objs;
> +	unsigned index;
> +
> +	/* Acquire lock */
Useless obvious comment, about as good a classic /* Add one to i */
>
> +static int common_stack_get(void *p, void **obj_table,
> +		unsigned n)
> +{
> +	struct rte_mempool_common_stack * s = (struct rte_mempool_common_stack *)p;

Unnecessary cast, in C void * can be assigned to any type.

> +	void **cache_objs;
> +	unsigned index, len;
> +
> +	/* Acquire lock */
Yet another useless comment.

> +	rte_spinlock_lock(&s->sl);
> +
> +	if(unlikely(n > s->len)) {
> +		rte_spinlock_unlock(&s->sl);
> +		return -ENOENT;
> +	}
> +
> +	cache_objs = s->objs;
> +
> +	for (index = 0, len = s->len - 1; index < n; ++index, len--, obj_table++)
> +		*obj_table = cache_objs[len];
> +
> +	s->len -= n;
> +	rte_spinlock_unlock(&s->sl);
> +	return n;
> +}
> +
> +static unsigned common_stack_get_count(void *p)
> +{
> +	struct rte_mempool_common_stack * s = (struct rte_mempool_common_stack *)p;

Another useless cast.

> +	return s->len;
> +}
> +
> +static void
> +common_stack_free(void *p)
> +{
> +	rte_free((struct rte_mempool_common_stack *)p);
Yet another useless cast

> +}
> +
> +static struct rte_mempool_handler handler_stack = {
For security, any data structure with function pointers should be const.

> +	.name = "stack",
> +	.alloc = common_stack_alloc,
> +	.free = common_stack_free,
> +	.put = common_stack_put,
> +	.get = common_stack_get,
> +	.get_count = common_stack_get_count
> +};
> +
> +MEMPOOL_REGISTER_HANDLER(handler_stack);