Re: [dpdk-dev] [PATCH] parray: introduce internal API for dynamic arrays

["Burakov, Anatoly" <anatoly.burakov@intel.com>]

On 14-Jun-21 11:58 AM, Thomas Monjalon wrote:
> Performance of access in a fixed-size array is very good
> because of cache locality
> and because there is a single pointer to dereference.
> The only drawback is the lack of flexibility:
> the size of such an array cannot be increase at runtime.
> 
> An approach to this problem is to allocate the array at runtime,
> being as efficient as static arrays, but still limited to a maximum.
> 
> That's why the API rte_parray is introduced,
> allowing to declare an array of pointer which can be resized dynamically
> and automatically at runtime while keeping a good read performance.
> 
> After resize, the previous array is kept until the next resize
> to avoid crashs during a read without any lock.
> 
> Each element is a pointer to a memory chunk dynamically allocated.
> This is not good for cache locality but it allows to keep the same
> memory per element, no matter how the array is resized.
> Cache locality could be improved with mempools.
> The other drawback is having to dereference one more pointer
> to read an element.
> 
> There is not much locks, so the API is for internal use only.
> This API may be used to completely remove some compilation-time maximums.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---

<snip>

> +int32_t
> +rte_parray_find_next(struct rte_parray *obj, int32_t index)
> +{
> +	if (obj == NULL || index < 0) {
> +		rte_errno = EINVAL;
> +		return -1;
> +	}
> +
> +	pthread_mutex_lock(&obj->mutex);
> +
> +	while (index < obj->size && obj->array[index] == NULL)
> +		index++;
> +	if (index >= obj->size)
> +		index = -1;
> +
> +	pthread_mutex_unlock(&obj->mutex);
> +
> +	rte_errno = 0;
> +	return index;
> +}
> +

Just a general comment about this:

I'm not really sure i like this "kinda-sorta-threadsafe-but-not-really" 
approach. IMO something either should be thread-safe, or it should be 
explicitly not thread-safe. There's no point in locking here because any 
user of find_next() will *necessarily* race with other users, because by 
the time we exit the function, the result becomes stale - so why are we 
locking in the first place?

Would be perhaps be better to leave it as non-thread-safe at its core, 
but introduce wrappers for atomic-like access to the array? E.g. 
something like `rte_parray_find_next_free_and_set()` that will perform 
the lock-find-next-set-unlock sequence? Or, alternatively, have the 
mutex there, but provide API's for explicit locking, and put the burden 
on the user to actually do the locking correctly.

-- 
Thanks,
Anatoly