> From: Mattias Rönnblom [mailto:hof...@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.ronnb...@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.)
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).
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)), ...);
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
>
> > With or without suggested changes...
> >
> > For the series,
> > Acked-by: Morten Brørup <m...@smartsharesystems.com>
> >
>
> Thanks for all help.
Thank you for the detailed consideration of my feedback.
