diff mbox series

[v4,01/10] eal: introduce macros for getting value for bit

Message ID 20200703134641.386297-2-parav@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Raslan Darawsheh
Headers
Series Improve mlx5 PMD driver framework for multiple classes |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Parav Pandit July 3, 2020, 1:46 p.m. UTC 
  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>
---
Changelog:
v1->v2:
 - Addressed comments from Thomas and Gaten.
 - Avoided new file, added macro to rte_bitops.h
---
 lib/librte_eal/include/rte_bitops.h | 2 ++
 1 file changed, 2 insertions(+)

Comments

Morten Brørup July 6, 2020, 10:53 a.m. UTC | #1 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Parav Pandit
> Sent: Friday, July 3, 2020 3:47 PM
> 
> 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>
> ---
> Changelog:
> v1->v2:
>  - Addressed comments from Thomas and Gaten.
>  - Avoided new file, added macro to rte_bitops.h
> ---
>  lib/librte_eal/include/rte_bitops.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_eal/include/rte_bitops.h
> b/lib/librte_eal/include/rte_bitops.h
> index 740927f3b..d72c7cd93 100644
> --- a/lib/librte_eal/include/rte_bitops.h
> +++ b/lib/librte_eal/include/rte_bitops.h
> @@ -17,6 +17,8 @@
>  #include <rte_debug.h>
>  #include <rte_compat.h>
> 
> +#define RTE_BIT(bit_num)	(1UL << (bit_num))

Is the return value 32 or 64 bit, or is intended to depend on the target architecture?

Please be explicit by using UINT32_C(1) or UINT64_C(1) instead of 1UL, if you want a specific size.

It could be a static inline __attribute__((__pure__)) function instead of a macro, but it's not important for me.

The macro/function needs a description for the documentation.

I'm also concerned about the name of the macro being too generic. But the effort of changing all the drivers where it is being used already could be too big if the name changes too.

And the macro/function is new, so shouldn't it - in theory - be marked as experimental?

> +
>  /*------------------------ 32-bit relaxed operations -----------------
> -------*/
> 
>  /**
> --
> 2.26.2
>
Parav Pandit July 7, 2020, 11:38 a.m. UTC | #2 
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Monday, July 6, 2020 4:24 PM
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Parav Pandit
> > Sent: Friday, July 3, 2020 3:47 PM
> >
> > 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>
> > ---
> > Changelog:
> > v1->v2:
> >  - Addressed comments from Thomas and Gaten.
> >  - Avoided new file, added macro to rte_bitops.h
> > ---
> >  lib/librte_eal/include/rte_bitops.h | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/lib/librte_eal/include/rte_bitops.h
> > b/lib/librte_eal/include/rte_bitops.h
> > index 740927f3b..d72c7cd93 100644
> > --- a/lib/librte_eal/include/rte_bitops.h
> > +++ b/lib/librte_eal/include/rte_bitops.h
> > @@ -17,6 +17,8 @@
> >  #include <rte_debug.h>
> >  #include <rte_compat.h>
> >
> > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> 
> Is the return value 32 or 64 bit, or is intended to depend on the target
> architecture?
>
It should be 64-bit.
 
> Please be explicit by using UINT32_C(1) or UINT64_C(1) instead of 1UL, if you
> want a specific size.
> 
Will do UINT64_C(1).

> It could be a static inline __attribute__((__pure__)) function instead of a macro,
> but it's not important for me.
> 
> The macro/function needs a description for the documentation.
> 
In this header file or outside?

> I'm also concerned about the name of the macro being too generic. But the
> effort of changing all the drivers where it is being used already could be too big
> if the name changes too.
>
Right. Currently drivers have generic name as BIT(). Close to 3000 entries.
So doing at RTE_BIT to match other rte_ APIs.
Drivers can slowly migrate at their pace to this one.
 
> And the macro/function is new, so shouldn't it - in theory - be marked as
> experimental?

How to mark a macro as experimental?
Thomas Monjalon July 7, 2020, 12:13 p.m. UTC | #3 
07/07/2020 13:38, Parav Pandit:
> From: Morten Brørup <mb@smartsharesystems.com>
> > From: Parav Pandit
> > > --- a/lib/librte_eal/include/rte_bitops.h
> > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > @@ -17,6 +17,8 @@
> > >  #include <rte_debug.h>
> > >  #include <rte_compat.h>
> > >
> > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > 
> > Is the return value 32 or 64 bit, or is intended to depend on the target
> > architecture?
> >
> It should be 64-bit.
>  
> > Please be explicit by using UINT32_C(1) or UINT64_C(1) instead of 1UL, if you
> > want a specific size.
> > 
> Will do UINT64_C(1).
> 
> > It could be a static inline __attribute__((__pure__)) function instead of a macro,
> > but it's not important for me.
> > 
> > The macro/function needs a description for the documentation.
> > 
> In this header file or outside?

It is asked to add a doxygen comment.


> > I'm also concerned about the name of the macro being too generic. But the
> > effort of changing all the drivers where it is being used already could be too big
> > if the name changes too.
> >
> Right. Currently drivers have generic name as BIT(). Close to 3000 entries.
> So doing at RTE_BIT to match other rte_ APIs.
> Drivers can slowly migrate at their pace to this one.
>  
> > And the macro/function is new, so shouldn't it - in theory - be marked as
> > experimental?
> 
> How to mark a macro as experimental?

A macro cannot be experimental.
Morten Brørup July 7, 2020, 12:40 p.m. UTC | #4 
Adding Joyce Kong to this discussion as the rte_bitops maintainer.

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, July 7, 2020 2:13 PM
> 
> 07/07/2020 13:38, Parav Pandit:
> > From: Morten Brørup <mb@smartsharesystems.com>
> > > From: Parav Pandit
> > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > @@ -17,6 +17,8 @@
> > > >  #include <rte_debug.h>
> > > >  #include <rte_compat.h>
> > > >
> > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > >
> > > Is the return value 32 or 64 bit, or is intended to depend on the
> target
> > > architecture?
> > >
> > It should be 64-bit.
> >
> > > Please be explicit by using UINT32_C(1) or UINT64_C(1) instead of
> 1UL, if you
> > > want a specific size.
> > >
> > Will do UINT64_C(1).
> >
> > > It could be a static inline __attribute__((__pure__)) function
> instead of a macro,
> > > but it's not important for me.
> > >
> > > The macro/function needs a description for the documentation.
> > >
> > In this header file or outside?
> 
> It is asked to add a doxygen comment.
> 
> 
> > > I'm also concerned about the name of the macro being too generic.
> But the
> > > effort of changing all the drivers where it is being used already
> could be too big
> > > if the name changes too.
> > >
> > Right. Currently drivers have generic name as BIT(). Close to 3000
> entries.
> > So doing at RTE_BIT to match other rte_ APIs.
> > Drivers can slowly migrate at their pace to this one.
> >
> > > And the macro/function is new, so shouldn't it - in theory - be
> marked as
> > > experimental?
> >
> > How to mark a macro as experimental?
> 
> A macro cannot be experimental.
> 

OK. If the macro is given a future proof name, I guess it should be accepted.

If we want boundary checks, I suggest a macro like:

#define RTE_BIT64(nr)						\
	({								\
		typeof(nr) n = nr; 				\
		RTE_BUILD_BUG_ON((n > 64) || (n < 0)); 	\
		UINT64_C(1) << (n);				\
	})

Or a function:

__rte_experimental
static __rte_always_inline __attribute__((const)) uint64_t
rte_bit64(const unsigned int nr)
{
	RTE_ASSERT(nr < 64);

	return UINT64_C(1) << nr;
}
Parav Pandit July 9, 2020, 6:23 a.m. UTC | #5 
Hi Morten,

> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Tuesday, July 7, 2020 6:11 PM

> Adding Joyce Kong to this discussion as the rte_bitops maintainer.
> 
> > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > Sent: Tuesday, July 7, 2020 2:13 PM
> >
> > 07/07/2020 13:38, Parav Pandit:
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > From: Parav Pandit
> > > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > > @@ -17,6 +17,8 @@
> > > > >  #include <rte_debug.h>
> > > > >  #include <rte_compat.h>
> > > > >
> > > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > > >
> > > > Is the return value 32 or 64 bit, or is intended to depend on the
> > target
> > > > architecture?
> > > >
> > > It should be 64-bit.
> > >
> > > > Please be explicit by using UINT32_C(1) or UINT64_C(1) instead of
> > 1UL, if you
> > > > want a specific size.
> > > >
> > > Will do UINT64_C(1).
> > >
> > > > It could be a static inline __attribute__((__pure__)) function
> > instead of a macro,
> > > > but it's not important for me.
> > > >
> > > > The macro/function needs a description for the documentation.
> > > >
> > > In this header file or outside?
> >
> > It is asked to add a doxygen comment.
Ok. will add.

> >
> >
> > > > I'm also concerned about the name of the macro being too generic.
> > But the
> > > > effort of changing all the drivers where it is being used already
> > could be too big
> > > > if the name changes too.
> > > >
> > > Right. Currently drivers have generic name as BIT(). Close to 3000
> > entries.
> > > So doing at RTE_BIT to match other rte_ APIs.
> > > Drivers can slowly migrate at their pace to this one.
> > >
> > > > And the macro/function is new, so shouldn't it - in theory - be
> > marked as
> > > > experimental?
> > >
> > > How to mark a macro as experimental?
> >
> > A macro cannot be experimental.
> >
> 
> OK. If the macro is given a future proof name, I guess it should be accepted.
> 
> If we want boundary checks, I suggest a macro like:
> 
> #define RTE_BIT64(nr)						\
> 	({								\
> 		typeof(nr) n = nr; 				\
> 		RTE_BUILD_BUG_ON((n > 64) || (n < 0)); 	\
> 		UINT64_C(1) << (n);				\
> 	})
>
Compiler doesn't like it.

../lib/librte_eal/include/rte_bitops.h:21:2: error: braced-group within expression allowed only inside a function
  ({      \
  ^
 
> Or a function:
> 
> __rte_experimental
> static __rte_always_inline __attribute__((const)) uint64_t rte_bit64(const
> unsigned int nr) {
> 	RTE_ASSERT(nr < 64);
> 
> 	return UINT64_C(1) << nr;
> }
> 
Value retrieved using this macro is used an enum. Don't see how a function call like above can solve it.

For a below macro definition, compiler is already catching for negative value when RTE_BIT64(-1) is done,

../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift count is negative [-Wshift-count-negative]
 #define RTE_BIT64(nr) (UINT64_C(1) << (nr))

And when RTE_BIT64(259) is done below error is done,

../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift count >= width of type [-Wshift-count-overflow]
 #define RTE_BIT64(nr) (UINT64_C(1) << (nr))

So below definition is good covering all needed cases.

#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
Morten Brørup July 9, 2020, 7:15 a.m. UTC | #6 
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Parav Pandit
> Sent: Thursday, July 9, 2020 8:24 AM
> 
> Hi Morten,
> 
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Tuesday, July 7, 2020 6:11 PM
> 
> > Adding Joyce Kong to this discussion as the rte_bitops maintainer.
> >
> > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > Sent: Tuesday, July 7, 2020 2:13 PM
> > >
> > > 07/07/2020 13:38, Parav Pandit:
> > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > > From: Parav Pandit
> > > > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > > > @@ -17,6 +17,8 @@
> > > > > >  #include <rte_debug.h>
> > > > > >  #include <rte_compat.h>
> > > > > >
> > > > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > > > >
> > > > > Is the return value 32 or 64 bit, or is intended to depend on
> the
> > > target
> > > > > architecture?
> > > > >
> > > > It should be 64-bit.
> > > >
> > > > > Please be explicit by using UINT32_C(1) or UINT64_C(1) instead
> of
> > > 1UL, if you
> > > > > want a specific size.
> > > > >
> > > > Will do UINT64_C(1).
> > > >
> > > > > It could be a static inline __attribute__((__pure__)) function
> > > instead of a macro,
> > > > > but it's not important for me.
> > > > >
> > > > > The macro/function needs a description for the documentation.
> > > > >
> > > > In this header file or outside?
> > >
> > > It is asked to add a doxygen comment.
> Ok. will add.
> 
> > >
> > >
> > > > > I'm also concerned about the name of the macro being too
> generic.
> > > But the
> > > > > effort of changing all the drivers where it is being used
> already
> > > could be too big
> > > > > if the name changes too.
> > > > >
> > > > Right. Currently drivers have generic name as BIT(). Close to
> 3000
> > > entries.
> > > > So doing at RTE_BIT to match other rte_ APIs.
> > > > Drivers can slowly migrate at their pace to this one.
> > > >
> > > > > And the macro/function is new, so shouldn't it - in theory - be
> > > marked as
> > > > > experimental?
> > > >
> > > > How to mark a macro as experimental?
> > >
> > > A macro cannot be experimental.
> > >
> >
> > OK. If the macro is given a future proof name, I guess it should be
> accepted.
> >
> > If we want boundary checks, I suggest a macro like:
> >
> > #define RTE_BIT64(nr)						\
> > 	({								\
> > 		typeof(nr) n = nr; 				\
> > 		RTE_BUILD_BUG_ON((n > 64) || (n < 0)); 	\
> > 		UINT64_C(1) << (n);				\
> > 	})
> >
> Compiler doesn't like it.
> 
> ../lib/librte_eal/include/rte_bitops.h:21:2: error: braced-group within
> expression allowed only inside a function
>   ({      \
>   ^
> 
> > Or a function:
> >
> > __rte_experimental
> > static __rte_always_inline __attribute__((const)) uint64_t
> rte_bit64(const
> > unsigned int nr) {
> > 	RTE_ASSERT(nr < 64);
> >
> > 	return UINT64_C(1) << nr;
> > }
> >
> Value retrieved using this macro is used an enum. Don't see how a
> function call like above can solve it.
> 
> For a below macro definition, compiler is already catching for negative
> value when RTE_BIT64(-1) is done,
> 
> ../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift count
> is negative [-Wshift-count-negative]
>  #define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> 
> And when RTE_BIT64(259) is done below error is done,
> 
> ../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift count
> >= width of type [-Wshift-count-overflow]
>  #define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> 
> So below definition is good covering all needed cases.
> 
> #define RTE_BIT64(nr) (UINT64_C(1) << (nr))

Great. Then, when you have added a doxygen comment:

Acked-by: Morten Brørup <mb@smartsharesystems.com>
Parav Pandit July 9, 2020, 7:30 a.m. UTC | #7 
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Thursday, July 9, 2020 12:46 PM
> 
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Parav Pandit
> > Sent: Thursday, July 9, 2020 8:24 AM
> >
> > Hi Morten,
> >
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Tuesday, July 7, 2020 6:11 PM
> >
> > > Adding Joyce Kong to this discussion as the rte_bitops maintainer.
> > >
> > > > From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > > > Sent: Tuesday, July 7, 2020 2:13 PM
> > > >
> > > > 07/07/2020 13:38, Parav Pandit:
> > > > > From: Morten Brørup <mb@smartsharesystems.com>
> > > > > > From: Parav Pandit
> > > > > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > > > > @@ -17,6 +17,8 @@
> > > > > > >  #include <rte_debug.h>
> > > > > > >  #include <rte_compat.h>
> > > > > > >
> > > > > > > +#define RTE_BIT(bit_num)	(1UL << (bit_num))
> > > > > >
> > > > > > Is the return value 32 or 64 bit, or is intended to depend on
> > the
> > > > target
> > > > > > architecture?
> > > > > >
> > > > > It should be 64-bit.
> > > > >
> > > > > > Please be explicit by using UINT32_C(1) or UINT64_C(1) instead
> > of
> > > > 1UL, if you
> > > > > > want a specific size.
> > > > > >
> > > > > Will do UINT64_C(1).
> > > > >
> > > > > > It could be a static inline __attribute__((__pure__)) function
> > > > instead of a macro,
> > > > > > but it's not important for me.
> > > > > >
> > > > > > The macro/function needs a description for the documentation.
> > > > > >
> > > > > In this header file or outside?
> > > >
> > > > It is asked to add a doxygen comment.
> > Ok. will add.
> >
> > > >
> > > >
> > > > > > I'm also concerned about the name of the macro being too
> > generic.
> > > > But the
> > > > > > effort of changing all the drivers where it is being used
> > already
> > > > could be too big
> > > > > > if the name changes too.
> > > > > >
> > > > > Right. Currently drivers have generic name as BIT(). Close to
> > 3000
> > > > entries.
> > > > > So doing at RTE_BIT to match other rte_ APIs.
> > > > > Drivers can slowly migrate at their pace to this one.
> > > > >
> > > > > > And the macro/function is new, so shouldn't it - in theory -
> > > > > > be
> > > > marked as
> > > > > > experimental?
> > > > >
> > > > > How to mark a macro as experimental?
> > > >
> > > > A macro cannot be experimental.
> > > >
> > >
> > > OK. If the macro is given a future proof name, I guess it should be
> > accepted.
> > >
> > > If we want boundary checks, I suggest a macro like:
> > >
> > > #define RTE_BIT64(nr)						\
> > > 	({								\
> > > 		typeof(nr) n = nr; 				\
> > > 		RTE_BUILD_BUG_ON((n > 64) || (n < 0)); 	\
> > > 		UINT64_C(1) << (n);				\
> > > 	})
> > >
> > Compiler doesn't like it.
> >
> > ../lib/librte_eal/include/rte_bitops.h:21:2: error: braced-group
> > within expression allowed only inside a function
> >   ({      \
> >   ^
> >
> > > Or a function:
> > >
> > > __rte_experimental
> > > static __rte_always_inline __attribute__((const)) uint64_t
> > rte_bit64(const
> > > unsigned int nr) {
> > > 	RTE_ASSERT(nr < 64);
> > >
> > > 	return UINT64_C(1) << nr;
> > > }
> > >
> > Value retrieved using this macro is used an enum. Don't see how a
> > function call like above can solve it.
> >
> > For a below macro definition, compiler is already catching for
> > negative value when RTE_BIT64(-1) is done,
> >
> > ../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift
> > count is negative [-Wshift-count-negative]  #define RTE_BIT64(nr)
> > (UINT64_C(1) << (nr))
> >
> > And when RTE_BIT64(259) is done below error is done,
> >
> > ../lib/librte_eal/include/rte_bitops.h:36:36: warning: left shift
> > count
> > >= width of type [-Wshift-count-overflow]
> >  #define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> >
> > So below definition is good covering all needed cases.
> >
> > #define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> 
> Great. Then, when you have added a doxygen comment:
> 
> Acked-by: Morten Brørup <mb@smartsharesystems.com>

Thanks Morten; adding it.
diff mbox series

Patch

diff --git a/lib/librte_eal/include/rte_bitops.h b/lib/librte_eal/include/rte_bitops.h
index 740927f3b..d72c7cd93 100644
--- a/lib/librte_eal/include/rte_bitops.h
+++ b/lib/librte_eal/include/rte_bitops.h
@@ -17,6 +17,8 @@ 
 #include <rte_debug.h>
 #include <rte_compat.h>
 
+#define RTE_BIT(bit_num)	(1UL << (bit_num))
+
 /*------------------------ 32-bit relaxed operations ------------------------*/
 
 /**