[dpdk-dev,v3,1/2] eal: add API to align integer to previous power of 2
Checks
Commit Message
On Wed, Apr 04, 2018 at 05:11:22PM +0000, Matan Azrad wrote:
>
>
> From: Pavan Nikhilesh, Wednesday, April 4, 2018 7:42 PM
> > Hi Matan,
> >
> > On Wed, Apr 04, 2018 at 04:10:36PM +0000, Matan Azrad wrote:
> > > Hi Pavan
> > >
> > > Shouldn't the new APIs be tagged with the experimental tag as agreed?
> >
> > Can't tag it experimental as it causes cyclic dependency (need to include
> > rte_compact.h).
>
> You probably mean rte_compat.h.
>
> It is ok to add it, what is the issue with that?
Change set:
>
> > Besides it's a simple proven math API I don't think it will change anytime
> > soon.
>
> It has already discussed:
> https://dpdk.org/dev/patchwork/patch/35211/
>
> I think you need to add it anyway.
>
> > Thanks,
> > Pavan
> >
> > >
> > > Besides that,
> > > Acked-by: Matan Azrad <matan@mellanox.com>
> > >
> > > From: Pavan Nikhilesh, Wednesday, April 4, 2018 1:16 PM
> > > > Add 32b and 64b API's to align the given integer to the previous power of
> > 2.
> > > >
> > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
> > > > ---
> > > > v3 Changes:
> > > > - Move commonly used code to rte_combine(32/64)ms1b so that it can
> > > > be reused.
> > > >
> > > > v2 Changes:
> > > > - Modified api name to `rte_align(32/64)prevpow2` from
> > > > `rte_align(32/64)lowpow2`.
> > > > - corrected fuction to return if the integer is already aligned to power of
> > 2.
> > > >
> > > > lib/librte_eal/common/include/rte_common.h | 92
> > > > ++++++++++++++++++++++++++----
> > > > 1 file changed, 81 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/lib/librte_eal/common/include/rte_common.h
> > > > b/lib/librte_eal/common/include/rte_common.h
> > > > index c7803e41c..7e147dcf2 100644
> > > > --- a/lib/librte_eal/common/include/rte_common.h
> > > > +++ b/lib/librte_eal/common/include/rte_common.h
> > > > @@ -223,6 +223,51 @@ extern int
> > RTE_BUILD_BUG_ON_detected_error; }
> > > > while(0) #endif
> > > >
> > > > +/**
> > > > + * Combines 32b inputs most significant set bits into the least
> > > > + * significant bits to construct a value with the same MSBs as x
> > > > + * but all 1's under it.
> > > > + *
> > > > + * @param x
> > > > + * The integer whose MSBs need to be combined with its LSBs
> > > > + * @return
> > > > + * The combined value.
> > > > + */
> > > > +static inline uint32_t
> > > > +rte_combine32ms1b(register uint32_t x) {
> > > > + x |= x >> 1;
> > > > + x |= x >> 2;
> > > > + x |= x >> 4;
> > > > + x |= x >> 8;
> > > > + x |= x >> 16;
> > > > +
> > > > + return x;
> > > > +}
> > > > +
> > > > +/**
> > > > + * Combines 64b inputs most significant set bits into the least
> > > > + * significant bits to construct a value with the same MSBs as x
> > > > + * but all 1's under it.
> > > > + *
> > > > + * @param v
> > > > + * The integer whose MSBs need to be combined with its LSBs
> > > > + * @return
> > > > + * The combined value.
> > > > + */
> > > > +static inline uint64_t
> > > > +rte_combine64ms1b(register uint64_t v) {
> > > > + v |= v >> 1;
> > > > + v |= v >> 2;
> > > > + v |= v >> 4;
> > > > + v |= v >> 8;
> > > > + v |= v >> 16;
> > > > + v |= v >> 32;
> > > > +
> > > > + return v;
> > > > +}
> > > > +
> > > > /*********** Macros to work with powers of 2 ********/
> > > >
> > > > /**
> > > > @@ -250,15 +295,28 @@ static inline uint32_t rte_align32pow2(uint32_t
> > x) {
> > > > x--;
> > > > - x |= x >> 1;
> > > > - x |= x >> 2;
> > > > - x |= x >> 4;
> > > > - x |= x >> 8;
> > > > - x |= x >> 16;
> > > > + x = rte_combine32ms1b(x);
> > > >
> > > > return x + 1;
> > > > }
> > > >
> > > > +/**
> > > > + * Aligns input parameter to the previous power of 2
> > > > + *
> > > > + * @param x
> > > > + * The integer value to algin
> > > > + *
> > > > + * @return
> > > > + * Input parameter aligned to the previous power of 2
> > > > + */
> > > > +static inline uint32_t
> > > > +rte_align32prevpow2(uint32_t x)
> > > > +{
> > > > + x = rte_combine32ms1b(x);
> > > > +
> > > > + return x - (x >> 1);
> > > > +}
> > > > +
> > > > /**
> > > > * Aligns 64b input parameter to the next power of 2
> > > > *
> > > > @@ -272,16 +330,28 @@ static inline uint64_t rte_align64pow2(uint64_t
> > v) {
> > > > v--;
> > > > - v |= v >> 1;
> > > > - v |= v >> 2;
> > > > - v |= v >> 4;
> > > > - v |= v >> 8;
> > > > - v |= v >> 16;
> > > > - v |= v >> 32;
> > > > + v = rte_combine64ms1b(v);
> > > >
> > > > return v + 1;
> > > > }
> > > >
> > > > +/**
> > > > + * Aligns 64b input parameter to the previous power of 2
> > > > + *
> > > > + * @param v
> > > > + * The 64b value to align
> > > > + *
> > > > + * @return
> > > > + * Input parameter aligned to the previous power of 2
> > > > + */
> > > > +static inline uint64_t
> > > > +rte_align64prevpow2(uint64_t v)
> > > > +{
> > > > + v = rte_combine64ms1b(v);
> > > > +
> > > > + return v - (v >> 1);
> > > > +}
> > > > +
> > > > /*********** Macros for calculating min and max **********/
> > > >
> > > > /**
> > > > --
> > > > 2.16.3
> > >
Comments
Hi Pavan
From: Pavan Nikhilesh, Wednesday, April 4, 2018 8:52 PM
> On Wed, Apr 04, 2018 at 05:11:22PM +0000, Matan Azrad wrote:
> >
> >
> > From: Pavan Nikhilesh, Wednesday, April 4, 2018 7:42 PM
> > > Hi Matan,
> > >
> > > On Wed, Apr 04, 2018 at 04:10:36PM +0000, Matan Azrad wrote:
> > > > Hi Pavan
> > > >
> > > > Shouldn't the new APIs be tagged with the experimental tag as agreed?
> > >
> > > Can't tag it experimental as it causes cyclic dependency (need to
> > > include rte_compact.h).
> >
> > You probably mean rte_compat.h.
> >
> > It is ok to add it, what is the issue with that?
>
> Change set:
>
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -23,6 +23,7 @@ extern "C" {
> #include <limits.h>
>
> #include <rte_config.h>
> +#include <rte_compat.h>
>
> #ifndef typeof
> #define typeof __typeof__
> @@ -233,7 +234,7 @@ extern int RTE_BUILD_BUG_ON_detected_error;
> * @return
> * The combined value.
> */
> -static inline uint32_t
> +static inline uint32_t __rte_experimental
> rte_combine32ms1b(register uint32_t x)
> {
> x |= x >> 1;
> @@ -245,6 +246,10 @@ rte_combine32ms1b(register uint32_t x)
> return x;
> }
>
> Causes:
>
> In file included from
> /home/pavan/Work/clean/dpdk/build/include/rte_compat.h:8:0,
> from
> /home/pavan/Work/clean/dpdk/lib/librte_eal/linuxapp/eal/eal.c:27:
> /home/pavan/Work/clean/dpdk/build/include/rte_common.h:238:1: error:
> expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘rte_combine32ms1b’
> rte_combine32ms1b(register uint32_t x)
> ^~~~~~~~~~~~~~~~~
> /home/pavan/Work/clean/dpdk/build/include/rte_common.h: In function
> ‘rte_align32pow2’:
> /home/pavan/Work/clean/dpdk/build/include/rte_common.h:303:6: error:
> implicit declaration of function ‘rte_combine32ms1b’; did you mean
> ‘rte_combine64ms1b’? [-Werror=implicit-function-declaration]
> x = rte_combine32ms1b(x);
> ^~~~~~~~~~~~~~~~~
> rte_combine64ms1b
> /home/pavan/Work/clean/dpdk/build/include/rte_common.h:303:6: error:
> nested extern declaration of ‘rte_combine32ms1b’ [-Werror=nested-
> externs]
>
> Cause:
>
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -245,6 +246,10 @@ rte_combine32ms1b(register uint32_t x)
> return x;
> }
>
> +#ifndef __rte_experimental
> +#error "__rte_experimental is not defined!"
> +#endif
> +
>
> /home/pavan/Work/clean/dpdk/build/include/rte_common.h:250:9: error:
> #error "__rte_experimental is not defined!"
>
> >
> > > Besides it's a simple proven math API I don't think it will change
> > > anytime soon.
> >
> > It has already discussed:
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdp
> d
> >
> k.org%2Fdev%2Fpatchwork%2Fpatch%2F35211%2F&data=02%7C01%7Cmata
> n%40mell
> >
> anox.com%7Cf940ba2484134961435708d59a54ce41%7Ca652971c7d2e4d9ba6
> a4d149
> >
> 256f461b%7C0%7C0%7C636584611394981007&sdata=jO7gLkRhZVv6gSqTyzsK
> K2Lb3j
> > 9vsUg2%2FPKZDs2Xdag%3D&reserved=0
> >
> > I think you need to add it anyway.
> >
> > > Thanks,
> > > Pavan
> > >
> > > >
> > > > Besides that,
> > > > Acked-by: Matan Azrad <matan@mellanox.com>
> > > >
> > > > From: Pavan Nikhilesh, Wednesday, April 4, 2018 1:16 PM
> > > > > Add 32b and 64b API's to align the given integer to the previous
> > > > > power of
> > > 2.
> > > > >
> > > > > Signed-off-by: Pavan Nikhilesh
> <pbhagavatula@caviumnetworks.com>
> > > > > ---
> > > > > v3 Changes:
> > > > > - Move commonly used code to rte_combine(32/64)ms1b so that it
> > > > > can be reused.
> > > > >
> > > > > v2 Changes:
> > > > > - Modified api name to `rte_align(32/64)prevpow2` from
> > > > > `rte_align(32/64)lowpow2`.
> > > > > - corrected fuction to return if the integer is already aligned
> > > > > to power of
> > > 2.
> > > > >
> > > > > lib/librte_eal/common/include/rte_common.h | 92
> > > > > ++++++++++++++++++++++++++----
> > > > > 1 file changed, 81 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/lib/librte_eal/common/include/rte_common.h
> > > > > b/lib/librte_eal/common/include/rte_common.h
> > > > > index c7803e41c..7e147dcf2 100644
> > > > > --- a/lib/librte_eal/common/include/rte_common.h
> > > > > +++ b/lib/librte_eal/common/include/rte_common.h
> > > > > @@ -223,6 +223,51 @@ extern int
> > > RTE_BUILD_BUG_ON_detected_error; }
> > > > > while(0) #endif
> > > > >
> > > > > +/**
> > > > > + * Combines 32b inputs most significant set bits into the least
> > > > > + * significant bits to construct a value with the same MSBs as
> > > > > +x
> > > > > + * but all 1's under it.
> > > > > + *
> > > > > + * @param x
> > > > > + * The integer whose MSBs need to be combined with its LSBs
> > > > > + * @return
> > > > > + * The combined value.
> > > > > + */
> > > > > +static inline uint32_t
> > > > > +rte_combine32ms1b(register uint32_t x) {
> > > > > + x |= x >> 1;
> > > > > + x |= x >> 2;
> > > > > + x |= x >> 4;
> > > > > + x |= x >> 8;
> > > > > + x |= x >> 16;
> > > > > +
> > > > > + return x;
> > > > > +}
> > > > > +
> > > > > +/**
> > > > > + * Combines 64b inputs most significant set bits into the least
> > > > > + * significant bits to construct a value with the same MSBs as
> > > > > +x
> > > > > + * but all 1's under it.
> > > > > + *
> > > > > + * @param v
> > > > > + * The integer whose MSBs need to be combined with its LSBs
> > > > > + * @return
> > > > > + * The combined value.
> > > > > + */
> > > > > +static inline uint64_t
> > > > > +rte_combine64ms1b(register uint64_t v) {
> > > > > + v |= v >> 1;
> > > > > + v |= v >> 2;
> > > > > + v |= v >> 4;
> > > > > + v |= v >> 8;
> > > > > + v |= v >> 16;
> > > > > + v |= v >> 32;
> > > > > +
> > > > > + return v;
> > > > > +}
> > > > > +
> > > > > /*********** Macros to work with powers of 2 ********/
> > > > >
> > > > > /**
> > > > > @@ -250,15 +295,28 @@ static inline uint32_t
> > > > > rte_align32pow2(uint32_t
> > > x) {
> > > > > x--;
> > > > > - x |= x >> 1;
> > > > > - x |= x >> 2;
> > > > > - x |= x >> 4;
> > > > > - x |= x >> 8;
> > > > > - x |= x >> 16;
> > > > > + x = rte_combine32ms1b(x);
> > > > >
> > > > > return x + 1;
> > > > > }
> > > > >
> > > > > +/**
> > > > > + * Aligns input parameter to the previous power of 2
> > > > > + *
> > > > > + * @param x
> > > > > + * The integer value to algin
> > > > > + *
> > > > > + * @return
> > > > > + * Input parameter aligned to the previous power of 2
> > > > > + */
> > > > > +static inline uint32_t
> > > > > +rte_align32prevpow2(uint32_t x) {
> > > > > + x = rte_combine32ms1b(x);
> > > > > +
> > > > > + return x - (x >> 1);
> > > > > +}
> > > > > +
> > > > > /**
> > > > > * Aligns 64b input parameter to the next power of 2
> > > > > *
> > > > > @@ -272,16 +330,28 @@ static inline uint64_t
> > > > > rte_align64pow2(uint64_t
> > > v) {
> > > > > v--;
> > > > > - v |= v >> 1;
> > > > > - v |= v >> 2;
> > > > > - v |= v >> 4;
> > > > > - v |= v >> 8;
> > > > > - v |= v >> 16;
> > > > > - v |= v >> 32;
> > > > > + v = rte_combine64ms1b(v);
> > > > >
> > > > > return v + 1;
> > > > > }
> > > > >
> > > > > +/**
> > > > > + * Aligns 64b input parameter to the previous power of 2
> > > > > + *
> > > > > + * @param v
> > > > > + * The 64b value to align
> > > > > + *
> > > > > + * @return
> > > > > + * Input parameter aligned to the previous power of 2
> > > > > + */
> > > > > +static inline uint64_t
> > > > > +rte_align64prevpow2(uint64_t v) {
> > > > > + v = rte_combine64ms1b(v);
> > > > > +
> > > > > + return v - (v >> 1);
> > > > > +}
> > > > > +
> > > > > /*********** Macros for calculating min and max **********/
> > > > >
> > > > > /**
> > > > > --
> > > > > 2.16.3
> > > >
Got you.
Looks like you found issue here...
The experimental tag probably should be in a root .h file.
Probably, need a fix patch to move it for a different\new .h file.
What do you think?
Hi Matan,
>
> Got you.
> Looks like you found issue here...
> The experimental tag probably should be in a root .h file.
> Probably, need a fix patch to move it for a different\new .h file.
>
> What do you think?
>
Actually thats just start of the rabbit hole, if we succeed to tag a inline
function in rte_common.h as experimental every lib/driver that uses
rte_common.h (almost everything) needs to have CFLAGS set to
-DALLOW_EXPERIMENTAL_API.
Regards,
Pavan.
Hi Pavan
From: Pavan Nikhilesh, Wednesday, April 4, 2018 9:16 PM
> Hi Matan,
>
> >
> > Got you.
> > Looks like you found issue here...
> > The experimental tag probably should be in a root .h file.
> > Probably, need a fix patch to move it for a different\new .h file.
> >
> > What do you think?
> >
>
> Actually thats just start of the rabbit hole, if we succeed to tag a inline
> function in rte_common.h as experimental every lib/driver that uses
> rte_common.h (almost everything) needs to have CFLAGS set to -
> DALLOW_EXPERIMENTAL_API.
>
Isn't it relevant only for the libs which are using the new tagged APIs?
> Regards,
> Pavan.
On Wed, Apr 04, 2018 at 06:23:19PM +0000, Matan Azrad wrote:
> Hi Pavan
>
> From: Pavan Nikhilesh, Wednesday, April 4, 2018 9:16 PM
> > Hi Matan,
> >
> > >
> > > Got you.
> > > Looks like you found issue here...
> > > The experimental tag probably should be in a root .h file.
> > > Probably, need a fix patch to move it for a different\new .h file.
> > >
> > > What do you think?
> > >
> >
> > Actually thats just start of the rabbit hole, if we succeed to tag a inline
> > function in rte_common.h as experimental every lib/driver that uses
> > rte_common.h (almost everything) needs to have CFLAGS set to -
> > DALLOW_EXPERIMENTAL_API.
> >
>
> Isn't it relevant only for the libs which are using the new tagged APIs?
Static inline functions in .h files will be added to each and every .c
example preprocessor output for rte_pci.c which includes rte_common.h:
# 231 "/home/pavan/Work/clean/dpdk/build/include/rte_common.h"
extern int RTE_BUILD_BUG_ON_detected_error;
# 249 "/home/pavan/Work/clean/dpdk/build/include/rte_common.h"
static inline uint32_t __attribute__((deprecated("Symbol is not yet part of stable ABI"), section(".text.experimental")))
rte_combine32ms1b(register uint32_t x)
{
x |= x >> 1;
x |= x >> 2;
x |= x >> 4;
x |= x >> 8;
x |= x >> 16;
return x;
}
# 271 "/home/pavan/Work/clean/dpdk/build/include/rte_common.h"
static inline uint64_t
rte_combine64ms1b(register uint64_t v)
{
v |= v >> 1;
v |= v >> 2;
v |= v >> 4;
v |= v >> 8;
v |= v >> 16;
v |= v >> 32;
return v;
}
Which causes compiler to throw error as DALLOW_EXPERIMENTAL_API is not added
to cflags.
>
> > Regards,
> > Pavan.
Hi Pavan
From: Pavan Nikhilesh, Wednesday, April 4, 2018 9:36 PM
> On Wed, Apr 04, 2018 at 06:23:19PM +0000, Matan Azrad wrote:
> > Hi Pavan
> >
> > From: Pavan Nikhilesh, Wednesday, April 4, 2018 9:16 PM
> > > Hi Matan,
> > >
> > > >
> > > > Got you.
> > > > Looks like you found issue here...
> > > > The experimental tag probably should be in a root .h file.
> > > > Probably, need a fix patch to move it for a different\new .h file.
> > > >
> > > > What do you think?
> > > >
> > >
> > > Actually thats just start of the rabbit hole, if we succeed to tag a
> > > inline function in rte_common.h as experimental every lib/driver
> > > that uses rte_common.h (almost everything) needs to have CFLAGS set
> > > to - DALLOW_EXPERIMENTAL_API.
> > >
> >
> > Isn't it relevant only for the libs which are using the new tagged APIs?
>
> Static inline functions in .h files will be added to each and every .c example
> preprocessor output for rte_pci.c which includes rte_common.h:
>
> # 231 "/home/pavan/Work/clean/dpdk/build/include/rte_common.h"
> extern int RTE_BUILD_BUG_ON_detected_error; # 249
> "/home/pavan/Work/clean/dpdk/build/include/rte_common.h"
> static inline uint32_t __attribute__((deprecated("Symbol is not yet part of
> stable ABI"), section(".text.experimental"))) rte_combine32ms1b(register
> uint32_t x) { x |= x >> 1; x |= x >> 2; x |= x >> 4; x |= x >> 8; x |= x >> 16;
>
> return x;
> }
> # 271 "/home/pavan/Work/clean/dpdk/build/include/rte_common.h"
> static inline uint64_t
> rte_combine64ms1b(register uint64_t v)
> {
> v |= v >> 1;
> v |= v >> 2;
> v |= v >> 4;
> v |= v >> 8;
> v |= v >> 16;
> v |= v >> 32;
>
> return v;
> }
>
> Which causes compiler to throw error as DALLOW_EXPERIMENTAL_API is not
> added to cflags.
>
Are you sure?
I added the next code and the compilation passed:
static inline uint32_t
__attribute__((deprecated("Symbol is not yet part of stable ABI"), \
section(".text.experimental")))
rte_combine32ms1b(register uint32_t x)
{
x |= x >> 1;
x |= x >> 2;
x |= x >> 4;
x |= x >> 8;
x |= x >> 16;
return x;
}
Actually, the combine functions should not be experimental (already used in the existed code).
It also will prevent us to add the cflag in every lib which uses the old align functions.
Only the new align functions should be tagged.
And then, you need to add the cflag only in the places which use these functions.
Am I missing something?
> >
> > > Regards,
> > > Pavan.
@@ -23,6 +23,7 @@ extern "C" {
#include <limits.h>
#include <rte_config.h>
+#include <rte_compat.h>
#ifndef typeof
#define typeof __typeof__
@@ -233,7 +234,7 @@ extern int RTE_BUILD_BUG_ON_detected_error;
* @return
* The combined value.
*/
-static inline uint32_t
+static inline uint32_t __rte_experimental
rte_combine32ms1b(register uint32_t x)
{
x |= x >> 1;
@@ -245,6 +246,10 @@ rte_combine32ms1b(register uint32_t x)
return x;
}
Causes:
In file included from /home/pavan/Work/clean/dpdk/build/include/rte_compat.h:8:0,
from /home/pavan/Work/clean/dpdk/lib/librte_eal/linuxapp/eal/eal.c:27:
/home/pavan/Work/clean/dpdk/build/include/rte_common.h:238:1: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘rte_combine32ms1b’
rte_combine32ms1b(register uint32_t x)
^~~~~~~~~~~~~~~~~
/home/pavan/Work/clean/dpdk/build/include/rte_common.h: In function ‘rte_align32pow2’:
/home/pavan/Work/clean/dpdk/build/include/rte_common.h:303:6: error: implicit declaration of function ‘rte_combine32ms1b’; did you mean ‘rte_combine64ms1b’? [-Werror=implicit-function-declaration]
x = rte_combine32ms1b(x);
^~~~~~~~~~~~~~~~~
rte_combine64ms1b
/home/pavan/Work/clean/dpdk/build/include/rte_common.h:303:6: error: nested extern declaration of ‘rte_combine32ms1b’ [-Werror=nested-externs]
Cause:
@@ -245,6 +246,10 @@ rte_combine32ms1b(register uint32_t x)
return x;
}
+#ifndef __rte_experimental
+#error "__rte_experimental is not defined!"
+#endif
+
/home/pavan/Work/clean/dpdk/build/include/rte_common.h:250:9: error: #error "__rte_experimental is not defined!"