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

["Mattias Rönnblom" <hofors@lysator.liu.se>]

On 2024-09-17 18:11, Konstantin Ananyev wrote:
>>>> +
>>>> +/**
>>>> + * 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))
>>>> +
>>>> +/**
>>>> + * 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)
>>>
>>> Would it make sense to check that rte_lcore_id() !=  LCORE_ID_ANY?
>>> After all if people do not want this extra check, they can probably use
>>> RTE_LCORE_VAR_LCORE_VALUE(rte_lcore_id(), handle)
>>> explicitly.
>>>
>>
>> It would make sense, if it was an RTE_ASSERT(). Otherwise, I don't think
>> so. Attempting to gracefully handle API violations is bad practice, imo.
> 
> Ok, RTE_ASSERT() might be a good compromise.
> As I said in another mail for that thread, I wouldn't insist here.
> 

After a having a closer look at this issue, I'm not so sure any more. 
Such an assertion would disallow the use of the macros to retrieve a 
potentially-invalid pointer, which is then never used, in case it is 
invalid.

>>
>>>> +
>>>> +/**
>>>> + * Iterate over each lcore id's value for an lcore variable.
>>>> + *
>>>> + * @param value
>>>> + *   A pointer successively set to point to lcore variable value
>>>> + *   corresponding to every lcore id (up to @c RTE_MAX_LCORE).
>>>> + * @param handle
>>>> + *   The lcore variable handle.
>>>> + */
>>>> +#define RTE_LCORE_VAR_FOREACH_VALUE(value, handle)			\
>>>> +	for (unsigned int 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, handle))
>>>
>>> Might be a bit better (and safer) to make lcore_id a macro parameter?
>>> I.E.:
>>> define RTE_LCORE_VAR_FOREACH_VALUE(value, handle, lcore_id) \
>>> for ((lcore_id) = ...
>>>
>>
>> Why?
> 
> Variable with the same name (and type) can be defined by user before the loop,
> With the intention to use it inside the loop.
> Just like it happens here (in patch #2):
> +	unsigned int lcore_id;
> .....
> +	/* take the opportunity to test the foreach macro */
> +	int *v;
> +	lcore_id = 0;
> +	RTE_LCORE_VAR_FOREACH_VALUE(v, test_int) {
> +		TEST_ASSERT_EQUAL(states[lcore_id].new_value, *v,
> +				  "Unexpected value on lcore %d during "
> +				  "iteration", lcore_id);
> +		lcore_id++;
> +	}
> +
>   
> 

Indeed. I'll change it. I suppose you could also have issues if you 
nested the macro, although those could be solved by using something like 
__COUNTER__ to create a unique name.

Supplying the variable name does defeat part of the purpose of the 
RTE_LCORE_VAR_FOREACH_VALUE.

> 
>