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

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Konstantin Ananyev [mailto:konstantin.ananyev@huawei.com]
> Sent: Tuesday, 17 September 2024 01.20
> 
> > > > +/**
> > > > + * 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.
> >
> > Not generally. I prefer keeping it brief.
> > We could add a _SAFE variant with this extra check, like LIST_FOREACH has
> LIST_FOREACH_SAFE (although for a different purpose).
> >
> > Come to think of it: In the name of brevity, consider renaming
> RTE_LCORE_VAR_VALUE to RTE_LCORE_VAR. (And
> > RTE_LCORE_VAR_FOREACH_VALUE to RTE_LCORE_VAR_FOREACH.) We want to see these
> everywhere in the code.
> 
> Well, it is not about brevity...
> I just feel  uncomfortable that our own public macro doesn't check value
> returned by rte_lcore_id() and introduce a possible out-of-bound memory
> access.

For performance reasons, we generally don't check parameter validity in fast path functions/macros; lots of code in DPDK uses ptr->array[rte_lcore_id()] without checking rte_lcore_id() validity.
We shouldn't do it here either.

There's a secondary benefit:
RTE_LCORE_VAR_VALUE() returns a pointer, so this macro can always be used.
Especially, the pointer can be initialized with other variables at the start of a function:
struct mystruct * const state = RTE_LCORE_VAR_VALUE(state_handle);
The out-of-bound memory access will occur if dereferencing the pointer.

> 
> 
> > >
> > > > +
> > > > +/**
> > > > + * 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) = ...
> >
> > The same thought have struck me, so I checked the scope of lcore_id.
> > The scope of lcore_id remains limited to the for loop, i.e. it is available
> inside the for loop, but not after it.
> 
> Variable with the same name (and type) can be defined by used 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++;
> +	}
> +
> 

You convinced me here, Konstantin.
Adding the iterator (lcore_id) as a macro parameter reduces the risk of bugs, and has no real disadvantages.

> 
> > IMO this suffices, and lcore_id doesn't need to be a macro parameter.
> > Maybe renaming lcore_id to _lcore_id would be an improvement, if lcore_id is
> already defined and used for other purposes within
> > the for loop.

PS:
We discussed the _VALUE postfix previously, Mattias, and I agreed to it. But now that I have become more familiar with the code, I think the _VALUE postfix should be dropped.
I'm usually in favor of long variable/function/macro names, arguing that they improve code readability.
But I don't think the _VALUE postfix really improves readability.
Especially when RTE_LCORE_VAR() has become widely used, and everyone is familiar with it, a long name (RTE_LCORE_VAR_VALUE()) will be more annoying than helpful.