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

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> >> +
> >> +/**
> >> + * 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.

> 
> >> +
> >> +/**
> >> + * 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++;
+	}
+