[dpdk-dev] [PATCH v10 01/10] eal: introduce macro for bit definition

[Morten Brørup]

+ Ray and Neil as ABI Policy maintainers.

> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli at arm.com]
> Sent: Tuesday, July 28, 2020 4:19 AM
> 
> <snip>
> > >
> > > > Subject: [dpdk-dev] [PATCH v10 01/10] eal: introduce macro for
> bit
> > > definition
> > > >
> > > > There are several drivers which duplicate bit generation macro.
> > > > Introduce a generic bit macros so that such drivers avoid
> redefining
> > > same in
> > > > multiple drivers.
> > > >
> > > > Signed-off-by: Parav Pandit <parav at mellanox.com>
> > > > Acked-by: Matan Azrad <matan at mellanox.com>
> > > > Acked-by: Morten Brørup <mb at smartsharesystems.com>
> > > > ---
> > > > Changelog:
> > > > v4->v5:
> > > >  - Addressed comments from Morten Brørup
> > > >  - Renamed newly added macro to RTE_BIT64
> > > >  - Added doxygen comment section for the macro
> > > > v1->v2:
> > > >  - Addressed comments from Thomas and Gaten.
> > > >  - Avoided new file, added macro to rte_bitops.h
> > > > ---
> > > >  lib/librte_eal/include/rte_bitops.h | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/lib/librte_eal/include/rte_bitops.h
> > > > b/lib/librte_eal/include/rte_bitops.h
> > > > index 740927f3b..ca46a110f 100644
> > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > @@ -17,6 +17,14 @@
> > > >  #include <rte_debug.h>
> > > >  #include <rte_compat.h>
> > > >
> > > > +/**
> > > > + * Get the uint64_t value for a specified bit set.
> > > > + *
> > > > + * @param nr
> > > > + *   The bit number in range of 0 to 63.
> > > > + */
> > > > +#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> > > In general, the macros have been avoided in this file. Suggest
> > > changing this to an inline function.
> >
> > That has been discussed already, and rejected for good reasons:
> > http://inbox.dpdk.org/dev/AM0PR05MB4866823B0170B90F679A2765D1640@
> > AM0PR05MB4866.eurprd05.prod.outlook.com/
> Thank you for the link.
> In this patch series, I see the macro being used in enum initialization
> (7/10 in v11) as well as in functions (8/10 in v11). Does it make sense
> to introduce use inline functions and use the inline functions for
> 8/10?
> If we do this, we should document in rte_bitops.h that inline functions
> should be used wherever possible.

I would agree, but only in theory. I disagree in reality, and argue that there should only be macros for this. Here is why:

rte_byteorder.h has both RTE_BEnn() macros and rte_cpu_to_be_nn() functions, for doing the same thing at compile time or at run time. There are no compile time warnings if the wrong one is being used, so I am certain that we can find code that uses the macro where the function should be used, or vice versa.

Which opens another, higher level, question: Would it be possible to add a compile time check macro in rte_common.h for these and similar?

Furthermore: For the RTE_BITnn() operations in this patch set, I expect the compiler to generate perfectly efficient code using the macro for run time use. I.e. there would be no performance advantage by also implementing the macros as functions for run time use.

> >
> > > Also, this file has uses of this macro, it would be good to replace
> > > them with the new inline function.
> >
> > Makes sense.
> > And for consistency, it would require adding an RTE_BIT32() macro
> too.