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

[Honnappa Nagarahalli]

<snip>

> 
> 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.
Agree, there is not a suitable way to enforce the use of one over the other (other than code review).

When the APIs in rte_bitops.h were introduced, there was a discussion around using the macros. I was for using macros as it would have kept the code as well as number of APIs smaller. However, there was a decision made not to use macros and instead provide inline functions. It was nothing to do with performance. So, I am just saying that we need to follow the same principles at least for this file.

> >
> 
> 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?
> 
> 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