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

[David Marchand <david.marchand@redhat.com>]

On Mon, Oct 18, 2021 at 11:05 AM Andrew Rybchenko
<andrew.rybchenko@oktetlabs.ru> wrote:
> >>> +/**
> >>> + * 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.
> >
>
> I dreamed to finish with renanamings in core libraries in 2021.

Sadly, you are not the only one.


> May be it is to naive :(
>
> I see that rc1 is already delayed and adding a couple of
> more patches could delay it even more. I'm ready to do it
> without coccinelle today, so, it is a question to @Thomas and
> @David if that late patch will be considered at all.

Thomas?

As usual, we must avoid unannounced API changes unless waived by Techboard.
I am not against it if we keep compatilibity (marking RTE_DEPRECATED
can wait next releases), and seeing how it only impacts a relatively
low number of places in DPDK.

Risks are mainly on compilation side, and conflicts with other patches
in mempool for rc1, but I can only think of Dmitry series and the
conflict is easy to solve.

It should be quick to do: when could you send such a patch?


What I have seen:

$ git grep -iE "(define|inline).*\<(|__)mempool" -- lib/mempool/*.h
lib/mempool/rte_mempool.h:#define       MEMPOOL_PG_SHIFT_MAX
(sizeof(uintptr_t) * CHAR_BIT - 1)
lib/mempool/rte_mempool.h:#define       MEMPOOL_PG_NUM_DEFAULT  1
^^^
bit-rot, both can be dropped.

lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_SPREAD      0x0001
lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_CACHE_ALIGN 0x0002 /**<
Do not align objs on cache lines.*/
lib/mempool/rte_mempool.h:#define MEMPOOL_F_SP_PUT         0x0004 /**<
Default put is "single-producer".*/
lib/mempool/rte_mempool.h:#define MEMPOOL_F_SC_GET         0x0008 /**<
Default get is "single-consumer".*/
lib/mempool/rte_mempool.h:#define MEMPOOL_F_POOL_CREATED   0x0010 /**<
Internal: pool is created. */
lib/mempool/rte_mempool.h:#define MEMPOOL_F_NO_IOVA_CONTIG 0x0020 /**<
Don't need IOVA contiguous objs. */
lib/mempool/rte_mempool.h:#define MEMPOOL_VALID_USER_FLAGS
(MEMPOOL_F_NO_SPREAD \

lib/mempool/rte_mempool.h:#define __MEMPOOL_STAT_ADD(mp, name, n) do {
                   \
lib/mempool/rte_mempool.h:#define __MEMPOOL_STAT_ADD(mp, name, n) do {} while(0)
lib/mempool/rte_mempool.h:#define MEMPOOL_HEADER_SIZE(mp, cs) \
lib/mempool/rte_mempool.h:static inline struct rte_mempool_objhdr
*__mempool_get_header(void *obj)
lib/mempool/rte_mempool.h:static inline struct rte_mempool_objtlr
*__mempool_get_trailer(void *obj)
lib/mempool/rte_mempool.h:#define __mempool_check_cookies(mp,
obj_table_const, n, free) \
lib/mempool/rte_mempool.h:#define __mempool_check_cookies(mp,
obj_table_const, n, free) do {} while(0)
lib/mempool/rte_mempool.h:#define
__mempool_contig_blocks_check_cookies(mp, first_obj_table_const, n, \
lib/mempool/rte_mempool.h:#define
__mempool_contig_blocks_check_cookies(mp, first_obj_table_const, n, \
^^
Internal stuff, but I would prefix in any case.

lib/mempool/rte_mempool.h:#define MEMPOOL_REGISTER_OPS(ops)
                 \


-- 
David Marchand