27/10/2020 11:15, Olivier Matz: > On Mon, Oct 26, 2020 at 11:20:04PM +0100, Thomas Monjalon wrote: > > The test worker_loopback used the deprecated mbuf field udata64. > > It is moved to a dynamic field in order to allow removal of udata64. > > > > Signed-off-by: Thomas Monjalon <tho...@monjalon.net> > > Reviewed-by: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru> > > --- [...] > > +static int counter_dynfield_offset; > > In general, I wonder if we shouldn't initialize offset to -1.
Yes good idea. > > +#define COUNTER_FIELD_TYPE uint8_t > > Another general comment, I suggest to use a typedef instead of > a define when relevant. Yes > > +#define COUNTER_FIELD(mbuf) (*RTE_MBUF_DYNFIELD(mbuf, \ > > + counter_dynfield_offset, COUNTER_FIELD_TYPE *)) > > + > > I'm not sure this comment applies here, but since it's a simple example, > it's a good place for another general comment. The RTE_MBUF_DYNFIELD() > macro is convenient because it can be used to set or get a value of any > type, but in my opinion it is not always easy to read: > > RTE_MBUF_DYNFIELD(m, off, type) = value; > > In some situations, having wrappers may make the code more readable: > > static inline void mbuf_set_counter(struct rte_mbuf *m, counter_field_t > counter); > static inline counter_field_t mbuf_get_counter(struct rte_mbuf *m); > static inline void mbuf_inc_counter(struct rte_mbuf *m); I agree, I will add some wrapper functions.
