Re: [dpdk-dev] [PATCH 1/7] stack: introduce rte stack library

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

Hi Gage,

Please find few comments below.

On Fri, Feb 22, 2019 at 10:06:49AM -0600, Gage Eads wrote:
> The rte_stack library provides an API for configuration and use of a
> bounded stack of pointers. Push and pop operations are MT-safe, allowing
> concurrent access, and the interface supports pushing and popping multiple
> pointers at a time.
> 
> The library's interface is modeled after another DPDK data structure,
> rte_ring, and its lock-based implementation is derived from the stack
> mempool handler. An upcoming commit will migrate the stack mempool handler
> to rte_stack.
> 
> Signed-off-by: Gage Eads <gage.eads@intel.com>

[...]

> --- /dev/null
> +++ b/doc/guides/prog_guide/stack_lib.rst
> @@ -0,0 +1,26 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright(c) 2019 Intel Corporation.
> +
> +Stack Library
> +=============
> +
> +DPDK's stack library provides an API for configuration and use of a bounded stack of
> +pointers.
> +
> +The stack library provides the following basic operations:
> +
> +*  Create a uniquely named stack of a user-specified size and using a user-specified socket.
> +
> +*  Push and pop a burst of one or more stack objects (pointers). These function are multi-threading safe.
> +
> +*  Free a previously created stack.
> +
> +*  Lookup a pointer to a stack by its name.
> +
> +*  Query a stack's current depth and number of free entries.

It seems the 80-cols limitation also applies to documentation:
https://mails.dpdk.org/archives/dev/2019-February/124917.html

[...]

> --- /dev/null
> +++ b/lib/librte_stack/rte_stack.h
> @@ -0,0 +1,277 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 Intel Corporation
> + */
> +
> +/**
> + * @file rte_stack.h
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * RTE Stack.
> + * librte_stack provides an API for configuration and use of a bounded stack of
> + * pointers. Push and pop operations are MT-safe, allowing concurrent access,
> + * and the interface supports pushing and popping multiple pointers at a time.
> + */
> +
> +#ifndef _RTE_STACK_H_
> +#define _RTE_STACK_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_errno.h>
> +#include <rte_memzone.h>
> +#include <rte_spinlock.h>
> +
> +#define RTE_TAILQ_STACK_NAME "RTE_STACK"
> +#define RTE_STACK_MZ_PREFIX "STK_"
> +/**< The maximum length of a stack name. */
> +#define RTE_STACK_NAMESIZE (RTE_MEMZONE_NAMESIZE - \
> +			   sizeof(RTE_STACK_MZ_PREFIX) + 1)
> +
> +/* Structure containing the LIFO, its current length, and a lock for mutual
> + * exclusion.
> + */
> +struct rte_lifo {
> +	rte_spinlock_t lock; /**< LIFO lock */
> +	uint32_t len; /**< LIFO len */
> +	void *objs[]; /**< LIFO pointer table */
> +};
> +
> +/* The RTE stack structure contains the LIFO structure itself, plus metadata
> + * such as its name and memzone pointer.
> + */
> +struct rte_stack {
> +	/** Name of the stack. */
> +	char name[RTE_STACK_NAMESIZE] __rte_cache_aligned;
> +	/** Memzone containing the rte_stack structure */
> +	const struct rte_memzone *memzone;
> +	uint32_t capacity; /**< Usable size of the stack */
> +	uint32_t flags; /**< Flags supplied at creation */
> +	struct rte_lifo lifo; /**< LIFO structure */
> +} __rte_cache_aligned;
> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * @internal Push several objects on the stack (MT-safe)
> + *
> + * @param s
> + *   A pointer to the stack structure.
> + * @param obj_table
> + *   A pointer to a table of void * pointers (objects).
> + * @param n
> + *   The number of objects to push on the stack from the obj_table.
> + * @return
> + *   Actual number of objects pushed (either 0 or *n*).
> + */

Minor: a dot is missing at the end of the title. There are few in this
patch, and maybe in next ones.

[...]

> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Return the number of used entries in a stack.
> + *
> + * @param s
> + *   A pointer to the stack structure.
> + * @return
> + *   The number of used entries in the stack.
> + */
> +static __rte_always_inline unsigned int __rte_experimental
> +rte_stack_count(struct rte_stack *s)
> +{
> +	return (unsigned int)s->lifo.len;
> +}

The argument can be const.

> +
> +/**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
> + *
> + * Return the number of free entries in a stack.
> + *
> + * @param s
> + *   A pointer to the stack structure.
> + * @return
> + *   The number of free entries in the stack.
> + */
> +static __rte_always_inline unsigned int __rte_experimental
> +rte_stack_free_count(struct rte_stack *s)
> +{
> +	return s->capacity - rte_stack_count(s);
> +}

Same here.