[RFC v4 1/6] eal: add static per-lcore memory allocation facility

[Mattias Rönnblom]

On 2024-02-27 16:05, Morten Brørup wrote:
>> From: Mattias Rönnblom [mailto:hofors at lysator.liu.se]
>> Sent: Tuesday, 27 February 2024 14.44
>>
>> On 2024-02-27 10:58, Morten Brørup wrote:
>>>> From: Mattias Rönnblom [mailto:mattias.ronnblom at ericsson.com]
>>>> Sent: Sunday, 25 February 2024 16.03
>>>
>>> [...]
>>>
>>>> +static void *
>>>> +lcore_var_alloc(size_t size, size_t align)
>>>> +{
>>>> +	void *handle;
>>>> +	void *value;
>>>> +
>>>> +	offset = RTE_ALIGN_CEIL(offset, align);
>>>> +
>>>> +	if (offset + size > RTE_MAX_LCORE_VAR) {
>>>
>>> This would be the usual comparison:
>>> if (lcore_buffer == NULL) {
>>>
>>>> +		lcore_buffer = aligned_alloc(RTE_CACHE_LINE_SIZE,
>>>> +					     LCORE_BUFFER_SIZE);
>>>> +		RTE_VERIFY(lcore_buffer != NULL);
>>>> +
>>>> +		offset = 0;
>>>> +	}
>>>
>>> [...]
>>>
>>>> +/**
>>>> + * Define a lcore variable handle.
>>>> + *
>>>> + * This macro defines a variable which is used as a handle to access
>>>> + * the various per-lcore id instances of a per-lcore id variable.
>>>> + *
>>>> + * The aim with this macro is to make clear at the point of
>>>> + * declaration that this is an lcore handler, rather than a regular
>>>> + * pointer.
>>>> + *
>>>> + * Add @b static as a prefix in case the lcore variable are only to
>> be
>>>> + * accessed from a particular translation unit.
>>>> + */
>>>> +#define RTE_LCORE_VAR_HANDLE(type, name)	\
>>>> +	RTE_LCORE_VAR_HANDLE_TYPE(type) name
>>>> +
>>>
>>> The parameter is "name" here, and "handle" in other macros.
>>> Just mentioning to make sure you thought about it.
>>>
>>>> +/**
>>>> + * Get pointer to lcore variable instance with the specified lcore
>> id.
>>>> + */
>>>> +#define RTE_LCORE_VAR_LCORE_PTR(lcore_id, handle)			\
>>>> +	((typeof(handle))__rte_lcore_var_lcore_ptr(lcore_id, handle))
>>>> +
>>>> +/**
>>>> + * Get value of a lcore variable instance of the specified lcore id.
>>>> + */
>>>> +#define RTE_LCORE_VAR_LCORE_GET(lcore_id, handle)	\
>>>> +	(*(RTE_LCORE_VAR_LCORE_PTR(lcore_id, handle)))
>>>> +
>>>> +/**
>>>> + * Set the value of a lcore variable instance of the specified lcore
>> id.
>>>> + */
>>>> +#define RTE_LCORE_VAR_LCORE_SET(lcore_id, handle, value)		\
>>>> +	(*(RTE_LCORE_VAR_LCORE_PTR(lcore_id, handle)) = (value))
>>>
>>> I still think RTE_LCORE_VAR[_LCORE]_PTR() suffice, and
>> RTE_LCORE_VAR[_LCORE]_GET/SET are superfluous.
>>> But I don't insist on their removal. :-)
>>>
>>
>> I'll remove them. One can always add them later. Nothing I've seen in
>> the DPDK code base so far has been called for their use.
>>
>> Should the RTE_LCORE_VAR_PTR() be renamed RTE_LCORE_VAR_VALUE() (and
>> still return a pointer, obviously)? "PTR" seems a little superfluous
>> (Hungarian). "RTE_LCORE_VAR()" would be short, but not very descriptive.
> 
> Good question...
> 
> I would try to align this name and the name of the associated foreach macro, currently RTE_LCORE_VAR_FOREACH_VALUE(var, handle).
> 
> It seems confusing to have a macro named _VALUE() returning a pointer.
> (Which is why I also dislike the foreach macro's current name and "var" parameter name.)
> 

Not sure I agree. In C, you often ask for a value and get a pointer to 
that value. I'll leave it VALUE() for now.

> If it is supposed to be frequently used, a shorter name is preferable.
> Which leans towards RTE_LCORE_VAR().
> 
> And then RTE_FOREACH_LCORE_VAR(iterator, handle) or RTE_LCORE_VAR_FOREACH(iterator, handle).
> 

RTE_LCORE_VAR_FOREACH was the original name, which was changed because 
it was confusingly close to RTE_LCORE_FOREACH(), but had a different 
semantics in regards to which lcore ids are iterated over (EAL threads 
only, versus all lcore ids).

> But then it is not obvious from the name that they operate on pointers.
> We don't use Hungarian style in DPDK, so perhaps that is acceptable.
> 
> 
> Your conclusion that GET/SET are not generally required inspired me for another idea...
> Maybe returning a pointer is not the right thing to do!
> 
> I wonder if there are any obstacles to generally dereferencing the lcore variable pointer, like this:
> 
> #define RTE_LCORE_VAR_LCORE(lcore_id, handle) \
> 	(*(typeof(handle))__rte_lcore_var_lcore_ptr(lcore_id, handle))
> 
> It would work for both get and set:
> RTE_LCORE_VAR(foo) = RTE_LCORE_VAR(bar);
> 
> And also for functions being passed the address of the variable.
> E.g. memset(&RTE_LCORE_VAR(foo), ...) would expand to:
> memset(&(*(typeof(foo))__rte_lcore_var_lcore_ptr(rte_lcore_id(), foo)), ...);
> 
> 

The value is usually accessed by means of a pointer, so no need to 
return *pointer.

> One more thought, not related to the above discussion:
> 
> The TLS per-lcore variables are built with "per_lcore_" prefix added to the names, like this:
> #define RTE_DEFINE_PER_LCORE(type, name) \
> 	__thread __typeof__(type) per_lcore_##name
> 
> Should the lcore variables have something similar, i.e.:
> #define RTE_LCORE_VAR_HANDLE(type, name) \
> 	RTE_LCORE_VAR_HANDLE_TYPE(type) lcore_var_##name
> 

I started out with a prefix, but I removed it, since you may want to 
access (copy, assign) the handler pointer directly, and thus need to 
know it's real name. Also, I didn't see why you need a prefix.

For example, consider a section of code where you want to use one of two 
variables depending on condition.

RTE_LCORE_VAR_HANDLE(actual, int);

if (something)
     actual = some_handle;
else
     actual = some_other_handle;

int *value = RTE_LCORE_VAR_VALUE(actual);

This above doesn't work if some_handle is actually named 
rte_lcore_var_some_handle or something like that.

If you want to add a prefix (for which there shouldn't be a need), you 
would need a macro RTE_LCORE_VAR_NAME() as well, so the user can derive 
the actual name (including the prefix).

> 
>>
>>> With or without suggested changes...
>>>
>>> For the series,
>>> Acked-by: Morten Brørup <mb at smartsharesystems.com>
>>>
>>
>> Thanks for all help.
> 
> Thank you for the detailed consideration of my feedback.
>