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