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

[Eads, Gage]

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz at 6wind.com]
> Sent: Monday, February 25, 2019 4:43 AM
> To: Eads, Gage <gage.eads at intel.com>
> Cc: dev at dpdk.org; arybchenko at solarflare.com; Richardson, Bruce
> <bruce.richardson at intel.com>; Ananyev, Konstantin
> <konstantin.ananyev at intel.com>; gavin.hu at arm.com;
> Honnappa.Nagarahalli at arm.com; nd at arm.com; thomas at monjalon.net
> Subject: Re: [PATCH 1/7] stack: introduce rte stack library
> 
> 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 at 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
> 

Sure, will fix in v2.

> [...]
> 
> > --- /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.
> 

Will fix.

> [...]
> 
> > +/**
> > + * @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.

Unfortunately the const keyword causes a discarded-qualifiers warning in the non-blocking implementation, due to the call to rte_atomic64_read().

Thanks,
Gage