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 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. 

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


> 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.