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

[Morten Brørup]

> From: Gaëtan Rivet [mailto:grive at u256.net]
> Sent: Tuesday, July 28, 2020 11:29 AM
> 
> On 28/07/20 10:24 +0200, Morten Brørup wrote:
> > + 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.
> >
> 
> Hi,
> 
> It is not clear to me, reading this thread, what is the motivation to
> enforce use of inline functions? Is it perf, compiler type checking, or
> usage checks?
> 
> Macros are checked at compile time when possible, though it can be
> improved upon. But I agree with Morten, proposing two forms ensures
> devs
> will sometimes use the wrong one, and we would need a practical way to
> check usages.
> 
> > 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?
> >
> 
> Can you clarify your idea? Is is something similar to:
> 
>     #define _BIT64(n) (UINT64_C(1) << (n))
>     static inline uint64_t
>     bit64(uint64_t n)
>     {
>             assert(n < 64);
>             return (UINT64_C(1) << n);
>     }
>     /* Integer Constant Expression? */
>     #define ICE_P(x) (sizeof(int) == sizeof(*(1 ? ((void*)((x) * 0l)) :
> (int*)1)))
>     #define BIT64(n) (ICE_P(n) ? _BIT64(n) : bit64(n))
> 
> I don't think so, but this is as close as automatic compile-time check
> and automatic use of proper macro vs. function I know of, did you have
> something else in mind?

I was only thinking of adding a compile time warning if the function was being used where the macro should be used, and vice versa.

Your proposed solution for automatic use of the function or macro is even better. Thank you! And it could be used in rte_byteorder.h too.

But as I mentioned, it is a higher level discussion, so for this patch, let's settle with the macro as already provided by Parav. And the higher level discussion about how to do this generally in DPDK libraries, where both macros and functions for the same calculation are provided, can be resumed later.

> 
> In this kind of code:
> 
>    #include <stdio.h>
>    #include <stdint.h>
>    #include <inttypes.h>
>    #include <assert.h>
> 
>    enum vals {
>            ZERO = 0,
>            ONE = BIT64(1),
>            TWO = BIT64(2),
>            THREE = BIT64(3),
>    };
> 
>    int main(void)
>    {
>            uint64_t x = ONE;
> 
>            x = BIT64(0);
>            x = BIT64(1);
>            x = BIT64(60);
>            x = BIT64(64);
>            x = BIT64(x);
> 
>            printf("x: 0x%" PRIx64 "\n", x);
> 
>            return 0;
>    }
> 
> The enum is defined using the macro, x = BIT64(64); triggers the
> following warning with GCC:
> 
> constant_bitop.c:6:32: warning: left shift count >= width of type [-
> Wshift-count-overflow]
>     6 | #define _BIT64(n) (UINT64_C(1) << (n))
> 
> and x = BIT64(x); triggers the assert() at runtime.
> 
> > 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.
> >
> 
> Regards,
> --
> Gaëtan