On 2024-02-27 16:05, Morten Brørup wrote:
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.)
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 <m...@smartsharesystems.com>
Thanks for all help.
Thank you for the detailed consideration of my feedback.
