From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Hemminger <stephen@networkplumber.org>
Subject: Re: [PATCH 1/2] mempool: add stack (fifo) mempool handler
Date: Thu, 5 May 2016 14:28:35 -0700
Message-ID: <20160505142835.7df35079@xeon-e3>
References: <1462472982-49782-1-git-send-email-david.hunt@intel.com>
 <1462472982-49782-2-git-send-email-david.hunt@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Cc: olivier.matz@6wind.com, dev@dpdk.org
To: David Hunt <david.hunt@intel.com>
Return-path: <dev-bounces@dpdk.org>
Received: from mail-pf0-f175.google.com (mail-pf0-f175.google.com
 [209.85.192.175]) by dpdk.org (Postfix) with ESMTP id AB8F4595E
 for <dev@dpdk.org>; Thu,  5 May 2016 23:28:24 +0200 (CEST)
Received: by mail-pf0-f175.google.com with SMTP id c189so43124248pfb.3
 for <dev@dpdk.org>; Thu, 05 May 2016 14:28:24 -0700 (PDT)
In-Reply-To: <1462472982-49782-2-git-send-email-david.hunt@intel.com>
List-Id: patches and discussions about DPDK <dev.dpdk.org>
List-Unsubscribe: <http://dpdk.org/ml/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://dpdk.org/ml/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <http://dpdk.org/ml/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.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);