Re: [dpdk-dev] [PATCH] mempool: accept user flags only

[Olivier Matz <olivier.matz@6wind.com>]

On Mon, Oct 18, 2021 at 11:37:32AM +0300, Andrew Rybchenko wrote:
> On 10/18/21 11:26 AM, David Marchand wrote:
> > As reported by Dmitry, MEMPOOL_F_POOL_CREATED is a flag only manipulated
> > internally.
> > This flag is not supposed to be requested from an application and would
> > probably result in an incorrect behavior if an application did pass it.
> > 
> > Other internal flags may be introduced later.
> > 
> > Rework the check and export a mask of valid user flags for use in the
> > unit test.
> > 
> > Fixes: b240af8b10f9 ("mempool: enforce valid flags at creation")
> > 
> > Reported-by: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
> > Signed-off-by: David Marchand <david.marchand@redhat.com>

(...)

> > --- a/lib/mempool/rte_mempool.h
> > +++ b/lib/mempool/rte_mempool.h
> > @@ -258,6 +258,15 @@ struct rte_mempool {
> >  #define MEMPOOL_F_POOL_CREATED   0x0010 /**< Internal: pool is created. */
> >  #define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**< Don't need IOVA contiguous objs. */
> >  
> > +/**
> > + * This macro lists all the mempool flags an application may request.
> > + */
> > +#define MEMPOOL_VALID_USER_FLAGS (MEMPOOL_F_NO_SPREAD \
> 
> I think RTE_ prefix is missing here since it is in a public
> header now.

I discussed about this offline with David. I was ok to omit the
RTE_ prefix for consistency.

> > +	| MEMPOOL_F_NO_CACHE_ALIGN \
> > +	| MEMPOOL_F_SP_PUT \
> > +	| MEMPOOL_F_SC_GET \
> > +	| MEMPOOL_F_NO_IOVA_CONTIG \
> > +	)
> >  /**
> >   * @internal When debug is enabled, store some statistics.
> >   *
> > 
> 
> Should we make a patch to add defines with RTE_ prefix and
> add a deprecation for old flags without RTE_ prefix?
> 
> Olivier, what do you think? If you have time to care about it,
> it would be great. (I'm unfamiliar with coccinelle yet).

I was joking with David about keeping some work for 2022 :)

I will try to have a look. As a side note, I was not that convinced by
coccinelle for simple replacements like this, because it does not
update the comments or documentation, it can (rarely) break the
indentation, it is slow, and the syntax is quite complex.