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

[Thomas Monjalon <thomas@monjalon.net>]

14/06/2021 14:22, Morten Brørup:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > Sent: Monday, 14 June 2021 12.59
> > 
> > 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.
> 
> I get the purpose and overall intention of this library.
> 
> I probably already mentioned that I prefer
> "embedded style programming" with fixed size arrays,
> rather than runtime configurability.
> It's my personal opinion, and the DPDK Tech Board clearly prefers
> reducing the amount of compile time configurability,
> so there is no way for me to stop this progress,
> and I do not intend to oppose to this library. :-)

Embedded-style is highly customized and limited.
DPDK is more used in standard servers where
deployment must be easy and dynamically configurable.
That's my view on where we go, but I understand
some can have opposite goals. Thus the discussion :)

> This library is likely to become a core library of DPDK,
> so I think it is important getting it right.
> Could you please mention a few examples where
> you think this internal library should be used,

It could be used for device arrays which are managed
(alloc/free) in the main thread as part of init
and hotplug operations.
Other threads should be readers only.

> and where it should not be used.
> Then it is easier to discuss if the border line
> between control path and data plane is correct.
> E.g. this library is not intended to be used for dynamically
> sized packet queues that grow and shrink in the fast path.

Correct.
If fast path threads need to alloc/free, this is not the right API.
That's not a queue, just a growing array where each element has an index.

> If the library becomes a core DPDK library,
> it should probably be public instead of internal.
> E.g. if the library is used to make RTE_MAX_ETHPORTS dynamic
> instead of compile time fixed,
> then some applications might also need dynamically sized arrays
> for their application specific per-port runtime data,
> and this library could serve that purpose too.

It could be convenient but risky if users don't understand well
the limitations. I am not sure what to do.

> [snip]
> 
> > +
> > +/** Main object representing a dynamic array of pointers. */
> > +struct rte_parray {
> > +	/** Array of pointer to dynamically allocated struct. */
> > +	void **array;
> > +	/** Old array before resize, freed on next resize. */
> > +	void **old_array;
> > +	/* Lock for alloc/free operations. */
> > +	pthread_mutex_t mutex;
> > +	/** Current size of the full array. */
> > +	int32_t size;
> > +	/** Number of allocated elements. */
> > +	int32_t count;
> > +	/** Last allocated element. */
> > +	int32_t last;
> > +};
> 
> Why not uint32_t for size, count and last?

2 reasons:
1/ anyway we are limited to int32_t for the index.
2/ having the same type for all avoid compiler complaining
when comparing values.

> Consider if the hot members of the struct should be moved
> closer together, for increasing the probability
> that they end up in the same cache line
> if the structure is not cache line aligned. Probably not important,
> just wanted to mention it.

The only hot member is the array itself.
Depending on mutex implementation, all could be in a single cacheline.