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

["Eads, Gage" <gage.eads@intel.com>]

@Thomas: I expect I can address Honnappa's feedback within a day or two. Since today is the 19.05 merge deadline, what do you think about these options for merging?
1. Merge V4 now and address these comments during RC1.
2. Delay merge until RC2, with all comments addressed.

In terms of risk, Honnappa identified an incorrect memory ordering argument (patch 6/8), but that doesn't affect the one platform (x86-64) that can (currently) use this library. His other comments address readability, error-checking, and performance, but aren't critical.  Beyond that, this patchset is isolated from the rest of DPDK. So, I think the risk to the project is very low.

(Also, note that I accidentally left off Olivier's Reviewed-by tag in V4's patches 1, 3, 5, and 6 -- I'll address that as well)

> -----Original Message-----
> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> Sent: Thursday, March 28, 2019 6:27 PM
> To: Eads, Gage <gage.eads@intel.com>; dev@dpdk.org
> Cc: olivier.matz@6wind.com; arybchenko@solarflare.com; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; nd <nd@arm.com>; thomas@monjalon.net; nd
> <nd@arm.com>
> Subject: RE: [PATCH v3 1/8] stack: introduce rte stack library
> 
> Hi Gage,
> 	Apologies for the late comments.
> 

No problem, I appreciate the feedback.

[snip]

> > +static ssize_t
> > +rte_stack_get_memsize(unsigned int count) {
> > +	ssize_t sz = sizeof(struct rte_stack);
> > +
> > +	/* Add padding to avoid false sharing conflicts */
> > +	sz += RTE_CACHE_LINE_ROUNDUP(count * sizeof(void *)) +
> > +		2 * RTE_CACHE_LINE_SIZE;
> I did not understand how the false sharing is caused and how this padding is
> solving the issue. Verbose comments would help.

The additional padding (beyond the CACHE_LINE_ROUNDUP) is to prevent false sharing caused by adjacent/next-line hardware prefetchers. I'll address this.

[snip]

> > +struct rte_stack *
> > +rte_stack_create(const char *name, unsigned int count, int socket_id,
> > +		 uint32_t flags)
> > +{
> > +	char mz_name[RTE_MEMZONE_NAMESIZE];
> > +	struct rte_stack_list *stack_list;
> > +	const struct rte_memzone *mz;
> > +	struct rte_tailq_entry *te;
> > +	struct rte_stack *s;
> > +	unsigned int sz;
> > +	int ret;
> > +
> > +	RTE_SET_USED(flags);
> > +
> > +	sz = rte_stack_get_memsize(count);
> > +
> > +	ret = snprintf(mz_name, sizeof(mz_name), "%s%s",
> > +		       RTE_STACK_MZ_PREFIX, name);
> > +	if (ret < 0 || ret >= (int)sizeof(mz_name)) {
> > +		rte_errno = ENAMETOOLONG;
> > +		return NULL;
> > +	}
> > +
> > +	te = rte_zmalloc("STACK_TAILQ_ENTRY", sizeof(*te), 0);
> > +	if (te == NULL) {
> > +		STACK_LOG_ERR("Cannot reserve memory for tailq\n");
> > +		rte_errno = ENOMEM;
> > +		return NULL;
> > +	}
> > +
> > +	rte_rwlock_write_lock(RTE_EAL_TAILQ_RWLOCK);
> > +
> I think there is a need to check if a stack with the same name exists already.

rte_memzone_reserve_aligned() does just that. This behavior is tested in the function test_stack_name_reuse(), added in commit " test/stack: add stack test".

> > +	mz = rte_memzone_reserve_aligned(mz_name, sz, socket_id,
> > +					 0, __alignof__(*s));
> > +	if (mz == NULL) {
> > +		STACK_LOG_ERR("Cannot reserve stack memzone!\n");
> > +		rte_rwlock_write_unlock(RTE_EAL_TAILQ_RWLOCK);
> > +		rte_free(te);
> > +		return NULL;
> > +	}

[snip]

> > +void
> > +rte_stack_free(struct rte_stack *s)
> > +{
> > +	struct rte_stack_list *stack_list;
> > +	struct rte_tailq_entry *te;
> > +
> > +	if (s == NULL)
> > +		return;
> > +
> Adding a check to make sure the length of the stack is 0 would help catch
> issues?

My preference is to leave that check to the user, for any apps that want to/can safely free non-empty stacks.

[snip]

> > +#define RTE_TAILQ_STACK_NAME "RTE_STACK"
> > +#define RTE_STACK_MZ_PREFIX "STK_"
> Nit, "STACK_" would be easier to debug

Since RTE_MEMZONE_NAMESIZE (32) doesn't give us a lot of space, I kept the prefix short. Adding 2 more characters *probably* won't make a difference...but I'd prefer the shortened name.

> > +/** The maximum length of a stack name. */ #define
> RTE_STACK_NAMESIZE
> > +(RTE_MEMZONE_NAMESIZE - \
> > +			   sizeof(RTE_STACK_MZ_PREFIX) + 1)
> > +

[snip]

> > +/**
> > + * @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*).
> > + */
> > +static __rte_always_inline unsigned int __rte_experimental
> This is an internal function. Is '__rte_experimental' tag required?

I don't think so, but I erred on the side of caution. I don't think the tag causes any problems.

> 
> > +rte_stack_std_push(struct rte_stack *s, void * const *obj_table,
> > +unsigned int n) {
> Since this is an internal function, does it make sense to add '__' to the
> beginning of the function name (similar to what is done in rte_ring?).

Makes sense. I'll address this.

[snip]

> > +/**
> > + * @internal Pop several objects from 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 pull from the stack.
> > + * @return
> > + *   Actual number of objects popped (either 0 or *n*).
> > + */
> > +static __rte_always_inline unsigned int __rte_experimental
> This is an internal function. Is '__rte_experimental' tag required?

(see above)

[snip]

> > +static __rte_always_inline unsigned int __rte_experimental
> > +rte_stack_pop(struct rte_stack *s, void **obj_table, unsigned int n) {
> > +	if (unlikely(n == 0 || obj_table == NULL))
> > +		return 0;
> 's == NULL' can be added as well. Similar check is missing in 'rte_stack_push'.
> Since these are data-path APIs, RTE_ASSERT would be better.
> 

Good point. I'll add RTE_ASSERT for obj_table and s. That won't work for  "n == 0" -- the pop code assumes n > 0, so we can't allow that check to be compiled out.

[snip]

> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Create a new stack named *name* in memory.
> > + *
> > + * This function uses ``memzone_reserve()`` to allocate memory for a
> > +stack of
> > + * size *count*. The behavior of the stack is controlled by the *flags*.
> > + *
> > + * @param name
> > + *   The name of the stack.
> > + * @param count
> > + *   The size of the stack.
> > + * @param socket_id
> > + *   The *socket_id* argument is the socket identifier in case of
> > + *   NUMA. The value can be *SOCKET_ID_ANY* if there is no NUMA
> > + *   constraint for the reserved zone.
> > + * @param flags
> > + *   Reserved for future use.
> > + * @return
> > + *   On success, the pointer to the new allocated stack. NULL on error with
> > + *    rte_errno set appropriately. Possible errno values include:
> > + *    - ENOSPC - the maximum number of memzones has already been
> > allocated
> > + *    - EEXIST - a stack with the same name already exists
> This is not implemented currently

It is -- see above.

Thanks,
Gage