[PATCH v11 1/7] eal: add static per-lcore memory allocation facility

[Mattias Rönnblom]

On 2024-10-14 10:17, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:mattias.ronnblom at ericsson.com]
>> Sent: Monday, 14 October 2024 09.44
> 
> 
>> +struct lcore_var_buffer {
>> +	char data[RTE_MAX_LCORE_VAR * RTE_MAX_LCORE];
>> +	struct lcore_var_buffer *prev;
>> +};
> 
> In relation to Jerin's request for using hugepages when available, the "data" field should be a pointer to the memory allocated from either the heap or through rte_malloc. You would also need to add a flag to indicate which it is, so the correct deallocation function can be used to free it on cleanup.
> 

The typing (glibc heap or DPDK heap) could be in the buffers themselves, no?

> <feature creep>
> Here's another (nice to have) idea, which does not need to be part of this series, but can be implemented in a separate patch:
> If you move "offset" into this structure, new lcore variables can be allocated from any buffer, instead of only the most recently allocated buffer.
> There might even be gains by picking the "optimal" buffer to allocate different size variables from.
> </feature creep>
> 

If the max lcore variable size is much greater than the actual variable 
sizes, the amount of fragmentation (i.e., the space at the end) will be 
very small.

I don't think we should use huge pages for this facility, since they 
don't support demand paging.

The day we have a DPDK heap which support lcore-affinitized allocations, 
then potentially eal_common_lcore_var.c could use that, provided it's 
available (and there is a proper way to check [or get notified] if it is 
available or not).

>> +
>> +static struct lcore_var_buffer *current_buffer;
>> +
>> +/* initialized to trigger buffer allocation on first allocation */
>> +static size_t offset = RTE_MAX_LCORE_VAR;
> 
> 
>> +void *
>> +rte_lcore_var_alloc(size_t size, size_t align)
>> +{
>> +	/* Having the per-lcore buffer size aligned on cache lines
>> +	 * assures as well as having the base pointer aligned on cache
>> +	 * size assures that aligned offsets also translate to alipgned
>> +	 * pointers across all values.
>> +	 */
>> +	RTE_BUILD_BUG_ON(RTE_MAX_LCORE_VAR % RTE_CACHE_LINE_SIZE != 0);
>> +	RTE_ASSERT(align <= RTE_CACHE_LINE_SIZE);
>> +	RTE_ASSERT(size <= RTE_MAX_LCORE_VAR);
> 
> This is very slow path, please RTE_VERIFY instead of RTE_ASSERT in this function.
> 

Sure. (I think I rejected that before, but now I don't agree with my old 
self.)

> 
>> +/**
>> + * Get pointer to lcore variable instance with the specified lcore id.
>> + *
>> + * @param lcore_id
>> + *   The lcore id specifying which of the @c RTE_MAX_LCORE value
>> + *   instances should be accessed. The lcore id need not be valid
>> + *   (e.g., may be @ref LCORE_ID_ANY), but in such a case, the pointer
>> + *   is also not valid (and thus should not be dereferenced).
>> + * @param handle
>> + *   The lcore variable handle.
>> + */
>> +#define RTE_LCORE_VAR_LCORE_VALUE(lcore_id, handle)			\
>> +	((typeof(handle))rte_lcore_var_lcore_ptr(lcore_id, handle))
> 
> Please remove the _VALUE suffix.
> 

You changed your mind? I'm missing the rationale here.

>> +
>> +/**
>> + * Get pointer to lcore variable instance of the current thread.
>> + *
>> + * May only be used by EAL threads and registered non-EAL threads.
>> + */
>> +#define RTE_LCORE_VAR_VALUE(handle) \
>> +	RTE_LCORE_VAR_LCORE_VALUE(rte_lcore_id(), handle)
> 
> Please remove the _VALUE suffix.
> 
>> +
>> +/**
>> + * Iterate over each lcore id's value for an lcore variable.
>> + *
>> + * @param lcore_id
>> + *   An <code>unsigned int</code> variable successively set to the
>> + *   lcore id of every valid lcore id (up to @c RTE_MAX_LCORE).
>> + * @param value
>> + *   A pointer variable successively set to point to lcore variable
>> + *   value instance of the current lcore id being processed.
>> + * @param handle
>> + *   The lcore variable handle.
>> + */
>> +#define RTE_LCORE_VAR_FOREACH_VALUE(lcore_id, value, handle)
> 
> Please remove the _VALUE suffix.
> 
>> 	\
>> +	for ((lcore_id) =						\
>> +		     (((value) = RTE_LCORE_VAR_LCORE_VALUE(0, handle)), 0);
>> \
>> +	     (lcore_id) < RTE_MAX_LCORE;				\
>> +	     (lcore_id)++, (value) = RTE_LCORE_VAR_LCORE_VALUE(lcore_id,
>> \
>