[v10,01/10] eal: introduce macro for bit definition
Checks
Commit Message
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@mellanox.com>
Acked-by: Matan Azrad <matan@mellanox.com>
Acked-by: Morten Brørup <mb@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(+)
Comments
<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@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Morten Brørup <mb@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.
Also, this file has uses of this macro, it would be good to replace them with the new inline function.
> +
> /*------------------------ 32-bit relaxed operations ------------------------*/
>
> /**
> --
> 2.26.2
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Honnappa
> Nagarahalli
> Sent: Friday, July 24, 2020 8:31 PM
>
> <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@mellanox.com>
> > Acked-by: Matan Azrad <matan@mellanox.com>
> > Acked-by: Morten Brørup <mb@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/
> 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.
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, July 27, 2020 1:52 PM
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Honnappa
> > Nagarahalli
> > Sent: Friday, July 24, 2020 8:31 PM
> >
> > <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@mellanox.com>
> > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > Acked-by: Morten Brørup <mb@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:
> https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Finbox.
> dpdk.org%2Fdev%2FAM0PR05MB4866823B0170B90F679A2765D1640%40AM
> 0PR05MB4866.eurprd05.prod.outlook.com%2F&data=02%7C01%7Cpara
> v%40mellanox.com%7Cdb43e7a083ae44a063a208d8320619b6%7Ca652971c7d
> 2e4d9ba6a4d149256f461b%7C0%7C0%7C637314349075254981&sdata=k
> R3fSOickIY2HbpmodNlB3ERF%2F2Qc7th55SGF40xmB0%3D&reserved=0
>
> > 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.
Ok. Sending v12 addressing it.
<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@mellanox.com>
> > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > Acked-by: Morten Brørup <mb@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.
>
> > 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.
+ Ray and Neil as ABI Policy maintainers.
> From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@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@mellanox.com>
> > > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > > Acked-by: Morten Brørup <mb@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.
On 28/07/20 10:24 +0200, Morten Brørup wrote:
> + Ray and Neil as ABI Policy maintainers.
>
> > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@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@mellanox.com>
> > > > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > > > Acked-by: Morten Brørup <mb@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?
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,
> From: Gaëtan Rivet [mailto:grive@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@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@mellanox.com>
> > > > > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > > > > Acked-by: Morten Brørup <mb@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
<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@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@mellanox.com>
> > > > > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > > > > Acked-by: Morten Brørup <mb@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
28/07/2020 17:39, Honnappa Nagarahalli:
> > On 28/07/20 10:24 +0200, Morten Brørup wrote:
> > > > From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com]
> > > > > > > --- 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.
I think bit definitions should be simple macros.
Even macro is a bit overkill for this simple thing.
I will merge this series.
If we want to change the philosophy of macro definitions,
it may be a larger discussion.
@@ -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))
+
/*------------------------ 32-bit relaxed operations ------------------------*/
/**