> From: Konstantin Ananyev [mailto:konstantin.anan...@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.
