Re: [dpdk-dev] [PATCH v2 06/15] event/sw: switch test counter to dynamic mbuf field

[Thomas Monjalon <thomas@monjalon.net>]

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 <thomas@monjalon.net>
> > Reviewed-by: Andrew Rybchenko <andrew.rybchenko@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.