[dpdk-dev] [PATCH v4 01/10] eal: introduce macros for getting valuefor bit

[Parav Pandit]

> From: Morten Brørup <mb at smartsharesystems.com>
> Sent: Thursday, July 9, 2020 12:46 PM
> 
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Parav Pandit
> > Sent: Thursday, July 9, 2020 8:24 AM
> >
> > Hi Morten,
> >
> > > From: Morten Brørup <mb at 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 at monjalon.net]
> > > > Sent: Tuesday, July 7, 2020 2:13 PM
> > > >
> > > > 07/07/2020 13:38, Parav Pandit:
> > > > > From: Morten Brørup <mb at 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 at smartsharesystems.com>

Thanks Morten; adding it.