[PATCH 1/2] eal: add API to align integer to previous power of 2

[PATCH 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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>
---
 lib/librte_eal/common/include/rte_common.h | 36 ++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index c7803e41c..126914f07 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -259,6 +259,24 @@ rte_align32pow2(uint32_t 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_align32lowpow2(uint32_t x)
+{
+	x = rte_align32pow2(x);
+	x--;
+
+	return x - (x >> 1);
+}
+
 /**
  * Aligns 64b input parameter to the next power of 2
  *
@@ -282,6 +300,24 @@ rte_align64pow2(uint64_t 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_align64lowpow2(uint64_t v)
+{
+	v = rte_align64pow2(v);
+	v--;
+
+	return v - (v >> 1);
+}
+
 /*********** Macros for calculating min and max **********/
 
 /**
-- 
2.16.1

[PATCH 2/2] test: update common auto test

[Pavan Nikhilesh]

Update common auto test to include test for previous power of 2 for both
32 and 64bit integers.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 test/test/test_common.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/test/test_common.c b/test/test/test_common.c
index d0342430f..16e6b585a 100644
--- a/test/test/test_common.c
+++ b/test/test/test_common.c
@@ -80,6 +80,7 @@ test_align(void)
 			val / pow != (i / pow) + 1)		/* if not aligned, hence +1 */
 
 	uint32_t i, p, val;
+	uint64_t j, q;
 
 	for (i = 1, p = 1; i <= MAX_NUM; i ++) {
 		if (rte_align32pow2(i) != p)
@@ -88,6 +89,29 @@ test_align(void)
 			p <<= 1;
 	}
 
+	for (i = 1, p = 0; i <= MAX_NUM; i++) {
+		if (rte_align32lowpow2(i) != p)
+			FAIL_ALIGN("rte_align32lowpow2", i, p);
+		if (rte_is_power_of_2(i))
+			p = p ? p << 1 : 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64pow2(j) != q)
+			printf("rte_align64pow2() test failed: %lu %lu\n", j,
+					q);
+		if (j == q)
+			q <<= 1;
+	}
+
+	for (j = 1, q = 0; j <= MAX_NUM; j++) {
+		if (rte_align64lowpow2(j) != q)
+			printf("rte_align64lowpow2() test failed: %lu %lu\n", j,
+					q);
+		if (rte_is_power_of_2(j))
+			q = q ? q << 1 : 1;
+	}
+
 	for (p = 2; p <= MAX_NUM; p <<= 1) {
 
 		if (!rte_is_power_of_2(p))
-- 
2.16.1

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

Hi Pavan

Please see some comments below.

 From: Pavan Nikhilesh, Saturday, February 17, 2018 12:50 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>
> ---
>  lib/librte_eal/common/include/rte_common.h | 36
> ++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/rte_common.h
> b/lib/librte_eal/common/include/rte_common.h
> index c7803e41c..126914f07 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -259,6 +259,24 @@ rte_align32pow2(uint32_t 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

I think the zero case(x=0) result should be documented.

> + */
> +static inline uint32_t
> +rte_align32lowpow2(uint32_t x)

What do you think about " rte_align32prevpow2"?

> +{
> +	x = rte_align32pow2(x);

	In case of  x is power of 2 number(already aligned), looks like the result here is x and the final result is (x >> 1)?
	Is it as you expect?

> +	x--;
> +
> +	return x - (x >> 1);

Why can't the implementation just be:
return  rte_align32pow2(x) >> 1;

If the above is correct, Are you sure we need this API?

> +}
> +
>  /**
>   * Aligns 64b input parameter to the next power of 2
>   *
> @@ -282,6 +300,24 @@ rte_align64pow2(uint64_t 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_align64lowpow2(uint64_t v)
> +{
> +	v = rte_align64pow2(v);
> +	v--;
> +
> +	return v - (v >> 1);
> +}
> +

Same comments for 64b API.

>  /*********** Macros for calculating min and max **********/
> 
>  /**
> --
> 2.16.1


If it is a new API, I think it should be added to the map file and to be tagged as experimental. No?

Matan

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

> From: Wiles, Keith, Sunday, February 18, 2018 5:39 PM
> > On Feb 18, 2018, at 12:11 AM, Matan Azrad <matan@mellanox.com>
> wrote:
> >
> > Hi Pavan
> >
> > Please see some comments below.
> >
> > From: Pavan Nikhilesh, Saturday, February 17, 2018 12:50 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>
> >> ---
> >> lib/librte_eal/common/include/rte_common.h | 36
> >> ++++++++++++++++++++++++++++++
> >> 1 file changed, 36 insertions(+)
> >>
> >> diff --git a/lib/librte_eal/common/include/rte_common.h
> >> b/lib/librte_eal/common/include/rte_common.h
> >> index c7803e41c..126914f07 100644
> >> --- a/lib/librte_eal/common/include/rte_common.h
> >> +++ b/lib/librte_eal/common/include/rte_common.h
> >> @@ -259,6 +259,24 @@ rte_align32pow2(uint32_t 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
> >
> > I think the zero case(x=0) result should be documented.
> >
> >> + */
> >> +static inline uint32_t
> >> +rte_align32lowpow2(uint32_t x)
> >
> > What do you think about " rte_align32prevpow2"?
> >
> >> +{
> >> +	x = rte_align32pow2(x);
> >
> > 	In case of  x is power of 2 number(already aligned), looks like the
> result here is x and the final result is (x >> 1)?
> > 	Is it as you expect?
> >
> >> +	x--;
> >> +
> >> +	return x - (x >> 1);
> >
> > Why can't the implementation just be:
> > return  rte_align32pow2(x) >> 1;
> >
> > If the above is correct, Are you sure we need this API?
> >
> >> +}
> >> +
> >> /**
> >>  * Aligns 64b input parameter to the next power of 2
> >>  *
> >> @@ -282,6 +300,24 @@ rte_align64pow2(uint64_t 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_align64lowpow2(uint64_t v)
> >> +{
> >> +	v = rte_align64pow2(v);
> >> +	v--;
> >> +
> >> +	return v - (v >> 1);
> >> +}
> >> +
> >
> > Same comments for 64b API.
> >
> >> /*********** Macros for calculating min and max **********/
> >>
> >> /**
> >> --
> >> 2.16.1
> >
> >
> > If it is a new API, I think it should be added to the map file and to be tagged
> as experimental. No?
> >
> 
> Is this the type of API that needs to be marked experimental,

I think it is relevant to any exposed API(not only for internal libraries).

> we should be able to prove these functions, correct?

Don't we need to prove any function in DPDK?
What is your point?

> > Matan
> 
> Regards,
> Keith

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Wiles, Keith]

> On Feb 19, 2018, at 12:03 AM, Matan Azrad <matan@mellanox.com> wrote:
> 
> 
>> 
>> Is this the type of API that needs to be marked experimental,
> 
> I think it is relevant to any exposed API(not only for internal libraries).
> 
>> we should be able to prove these functions, correct?
> 
> Don't we need to prove any function in DPDK?
> What is your point?


My point is this is a inline function and can not be placed in the .map file as a external API. These simple type of APIs are easy to prove and making them experimental seems to just cause an extra step. If the functions are not required that is a different problem or if the API is really only ever used by a single function or module of files then it should be moved to the module/file and made locate to the module/file.

> 
>>> Matan
>> 
>> Regards,
>> Keith

Regards,
Keith

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

Hi Wiles

From: Wiles, Keith, Monday, February 19, 2018 3:56 PM
> > On Feb 19, 2018, at 12:03 AM, Matan Azrad <matan@mellanox.com> wrote:
> >> Is this the type of API that needs to be marked experimental,
> >
> > I think it is relevant to any exposed API(not only for internal libraries).
> >
> >> we should be able to prove these functions, correct?
> >
> > Don't we need to prove any function in DPDK?
> > What is your point?
> 
> 
> My point is this is a inline function and can not be placed in the .map file as a
> external API.

Doesn't each API in .h file external? Why not?
If it shouldn't be external and should be in .h file, I think it should be marked as internal, no?

> These simple type of APIs are easy to prove and making them
> experimental seems to just cause an extra step.

As Thomas mentioned here:
https://dpdk.org/ml/archives/dev/2018-January/087719.html

Any new API should be experimental.

Thomas, Is it different for .h file inline APIs?
 
> If the functions are not required that is a different problem or if the API is really only ever used by a
> single function or module of files then it should be moved to the module/file
> and made locate to the module/file.

Agree.
Looks like this function makes sense and may be used by other modules later.

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Wiles, Keith]

> On Feb 19, 2018, at 8:13 AM, Matan Azrad <matan@mellanox.com> wrote:
> 
> Hi Wiles
> 
> From: Wiles, Keith, Monday, February 19, 2018 3:56 PM
>>> On Feb 19, 2018, at 12:03 AM, Matan Azrad <matan@mellanox.com> wrote:
>>>> Is this the type of API that needs to be marked experimental,
>>> 
>>> I think it is relevant to any exposed API(not only for internal libraries).
>>> 
>>>> we should be able to prove these functions, correct?
>>> 
>>> Don't we need to prove any function in DPDK?
>>> What is your point?
>> 
>> 
>> My point is this is a inline function and can not be placed in the .map file as a
>> external API.
> 
> Doesn't each API in .h file external? Why not?
> If it shouldn't be external and should be in .h file, I think it should be marked as internal, no?

We do not do a great job of using private headers as most of our headers are public ones and in the case or private they would not be external.

> 
>> These simple type of APIs are easy to prove and making them
>> experimental seems to just cause an extra step.
> 
> As Thomas mentioned here:
> https://dpdk.org/ml/archives/dev/2018-January/087719.html
> 
> Any new API should be experimental.
> 
> Thomas, Is it different for .h file inline APIs?
> 
>> If the functions are not required that is a different problem or if the API is really only ever used by a
>> single function or module of files then it should be moved to the module/file
>> and made locate to the module/file.
> 
> Agree.
> Looks like this function makes sense and may be used by other modules later.
> 

Regards,
Keith

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Thomas Monjalon]

19/02/2018 15:13, Matan Azrad:
> Hi Wiles
> 
> From: Wiles, Keith, Monday, February 19, 2018 3:56 PM
> > > On Feb 19, 2018, at 12:03 AM, Matan Azrad <matan@mellanox.com> wrote:
> > >> Is this the type of API that needs to be marked experimental,
> > >
> > > I think it is relevant to any exposed API(not only for internal libraries).
> > >
> > >> we should be able to prove these functions, correct?
> > >
> > > Don't we need to prove any function in DPDK?
> > > What is your point?
> > 
> > 
> > My point is this is a inline function and can not be placed in the .map file as a
> > external API.
> 
> Doesn't each API in .h file external? Why not?
> If it shouldn't be external and should be in .h file, I think it should be marked as internal, no?
> 
> > These simple type of APIs are easy to prove and making them
> > experimental seems to just cause an extra step.
> 
> As Thomas mentioned here:
> https://dpdk.org/ml/archives/dev/2018-January/087719.html
> 
> Any new API should be experimental.
> 
> Thomas, Is it different for .h file inline APIs?

No, it is not different for inline functions.
If we are discussing the policy for every function, it is not
a policy anymore :)

It was agreed to notify the users of the new functions,
so it gives time to confirm the function before making it "stable".
Even if the function looks obvious, I think we should follow the policy.

So, yes, please add the experimental tag.

Re: [PATCH 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

Hi Matan,

On Sun, Feb 18, 2018 at 06:11:20AM +0000, Matan Azrad wrote:
> Hi Pavan
>
> Please see some comments below.
>
>  From: Pavan Nikhilesh, Saturday, February 17, 2018 12:50 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>
> > ---
> >  lib/librte_eal/common/include/rte_common.h | 36
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/rte_common.h
> > b/lib/librte_eal/common/include/rte_common.h
> > index c7803e41c..126914f07 100644
> > --- a/lib/librte_eal/common/include/rte_common.h
> > +++ b/lib/librte_eal/common/include/rte_common.h
> > @@ -259,6 +259,24 @@ rte_align32pow2(uint32_t 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
>
> I think the zero case(x=0) result should be documented.

The existing API i.e. rte_align32pow2() behaves in similar manner i.e. returns
0 when 0 is passed.

>
> > + */
> > +static inline uint32_t
> > +rte_align32lowpow2(uint32_t x)
>
> What do you think about " rte_align32prevpow2"?

I think rte_align32prevpow2() fits better will modify and send v2.

>
> > +{
> > +	x = rte_align32pow2(x);
>
> 	In case of  x is power of 2 number(already aligned), looks like the result here is x and the final result is (x >> 1)?
> 	Is it as you expect?

I overlooked that bit while trying to make use of the existing API, will modify
the implementation to return x if its already a power of 2.

>
> > +	x--;
> > +
> > +	return x - (x >> 1);
>
> Why can't the implementation just be:
> return  rte_align32pow2(x) >> 1;
>
> If the above is correct, Are you sure we need this API?
>
> > +}
> > +
> >  /**
> >   * Aligns 64b input parameter to the next power of 2
> >   *
> > @@ -282,6 +300,24 @@ rte_align64pow2(uint64_t 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_align64lowpow2(uint64_t v)
> > +{
> > +	v = rte_align64pow2(v);
> > +	v--;
> > +
> > +	return v - (v >> 1);
> > +}
> > +
>
> Same comments for 64b API.
>
> >  /*********** Macros for calculating min and max **********/
> >
> >  /**
> > --
> > 2.16.1
>
>
> If it is a new API, I think it should be added to the map file and to be tagged as experimental. No?

Static inline functions need not be a part of map files, as for experimental
tag I don't think its needed for a math API. I don't have a strong opinion
tagging it experimental, if it is really needed I will send a re-do the patch
marking it experimental.

>
> Matan

Thanks,
Pavan

[PATCH v2 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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>
---
 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 | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index c7803e41c..b2017ee5c 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -259,6 +259,27 @@ rte_align32pow2(uint32_t 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 |= x >> 1;
+	x |= x >> 2;
+	x |= x >> 4;
+	x |= x >> 8;
+	x |= x >> 16;
+
+	return x - (x >> 1);
+}
+
 /**
  * Aligns 64b input parameter to the next power of 2
  *
@@ -282,6 +303,28 @@ rte_align64pow2(uint64_t 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 |= v >> 1;
+	v |= v >> 2;
+	v |= v >> 4;
+	v |= v >> 8;
+	v |= v >> 16;
+	v |= v >> 32;
+
+	return v - (v >> 1);
+}
+
 /*********** Macros for calculating min and max **********/

 /**
--
2.16.1

[PATCH v2 2/2] test: update common auto test

[Pavan Nikhilesh]

Update common auto test to include test for previous power of 2 for both
32 and 64bit integers.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 test/test/test_common.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/test/test_common.c b/test/test/test_common.c
index d0342430f..2115cc78f 100644
--- a/test/test/test_common.c
+++ b/test/test/test_common.c
@@ -80,6 +80,7 @@ test_align(void)
 			val / pow != (i / pow) + 1)		/* if not aligned, hence +1 */
 
 	uint32_t i, p, val;
+	uint64_t j, q;
 
 	for (i = 1, p = 1; i <= MAX_NUM; i ++) {
 		if (rte_align32pow2(i) != p)
@@ -88,6 +89,29 @@ test_align(void)
 			p <<= 1;
 	}
 
+	for (i = 1, p = 1; i <= MAX_NUM; i++) {
+		if (rte_align32prevpow2(i) != p)
+			FAIL_ALIGN("rte_align32prevpow2", i, p);
+		if (rte_is_power_of_2(i + 1))
+			p = i + 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64pow2(j) != q)
+			printf("rte_align64pow2() test failed: %lu %lu\n", j,
+					q);
+		if (j == q)
+			q <<= 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64prevpow2(j) != q)
+			printf("rte_align64prevpow2() test failed: %lu %lu\n",
+					j, q);
+		if (rte_is_power_of_2(j + 1))
+			q = j + 1;
+	}
+
 	for (p = 2; p <= MAX_NUM; p <<= 1) {
 
 		if (!rte_is_power_of_2(p))
-- 
2.16.1

Re: [PATCH v2 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

Hi Pavan
> From: Pavan Nikhilesh, Monday, February 19, 2018 1:37 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>
> ---
>  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 | 43
> ++++++++++++++++++++++++++++++
>  1 file changed, 43 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/rte_common.h
> b/lib/librte_eal/common/include/rte_common.h
> index c7803e41c..b2017ee5c 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -259,6 +259,27 @@ rte_align32pow2(uint32_t 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 |= x >> 1;
> +	x |= x >> 2;
> +	x |= x >> 4;
> +	x |= x >> 8;
> +	x |= x >> 16;
> +
> +	return x - (x >> 1);
> +}

Nice.

Since you are using the same 5 lines from the rte_align32pow2() function, I think this part can be in a separate function to do reuse.
Also the "fill ones 32" function can be used for other purpose.
What do you think?
 

>  /**
>   * Aligns 64b input parameter to the next power of 2
>   *
> @@ -282,6 +303,28 @@ rte_align64pow2(uint64_t 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 |= v >> 1;
> +	v |= v >> 2;
> +	v |= v >> 4;
> +	v |= v >> 8;
> +	v |= v >> 16;
> +	v |= v >> 32;
> +
> +	return v - (v >> 1);
> +}
> +
>  /*********** Macros for calculating min and max **********/
> 
>  /**
> --
> 2.16.1

Re: [PATCH v2 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

Hi Matan,

On Mon, Feb 19, 2018 at 12:09:46PM +0000, Matan Azrad wrote:
>
> Hi Pavan
> > From: Pavan Nikhilesh, Monday, February 19, 2018 1:37 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>
> > ---
> >  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 | 43
> > ++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/rte_common.h
> > b/lib/librte_eal/common/include/rte_common.h
> > index c7803e41c..b2017ee5c 100644
> > --- a/lib/librte_eal/common/include/rte_common.h
> > +++ b/lib/librte_eal/common/include/rte_common.h
> > @@ -259,6 +259,27 @@ rte_align32pow2(uint32_t 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 |= x >> 1;
> > +	x |= x >> 2;
> > +	x |= x >> 4;
> > +	x |= x >> 8;
> > +	x |= x >> 16;
> > +
> > +	return x - (x >> 1);
> > +}
>
> Nice.
>
> Since you are using the same 5 lines from the rte_align32pow2() function, I think this part can be in a separate function to do reuse.
> Also the "fill ones 32" function can be used for other purpose.
> What do you think?

I do agree that it would be cleaner to have a common function for both, but not
able to decide on a appropriate function name "fill ones 32" doesn't convey
what the function truly does. If you have a cleaner name do suggest, i will
roll up a v3 adding the function and experimental tag.

Thanks,
Pavan

>
>

Re: [PATCH v2 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

Hi Pavan

From: Pavan Nikhilesh, Monday, February 26, 2018 9:10 PM
> Hi Matan,
> 
> On Mon, Feb 19, 2018 at 12:09:46PM +0000, Matan Azrad wrote:
> > Since you are using the same 5 lines from the rte_align32pow2() function, I
>> think this part can be in a separate function to do reuse.
> > Also the "fill ones 32" function can be used for other purpose.
> > What do you think?
> 
> I do agree that it would be cleaner to have a common function for both, but
> not able to decide on a appropriate function name "fill ones 32" doesn't
> convey what the function truly does. If you have a cleaner name do suggest, i
> will roll up a v3 adding the function and experimental tag.

Sure.

I'm suggesting next names:
rte_combine32ms1b(register uint32_t x)
rte_combine64ms1b(register uint64_t x)

The description may be something like:
combine the upper bits into the LSBs to construct a value with the same most significant 1 as x but all 1's under it.

I would add the register keyword for each variables in all the align functions here just to hint for the compiler that it's better to save this variables in register and not in memory.  

Matan.
> Thanks,
> Pavan
> 
> >
> >

[PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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

[PATCH v3 2/2] test: update common auto test

[Pavan Nikhilesh]

Update common auto test to include test for previous power of 2 for both
32 and 64bit integers.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 test/test/test_common.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/test/test/test_common.c b/test/test/test_common.c
index d0342430f..2115cc78f 100644
--- a/test/test/test_common.c
+++ b/test/test/test_common.c
@@ -80,6 +80,7 @@ test_align(void)
 			val / pow != (i / pow) + 1)		/* if not aligned, hence +1 */
 
 	uint32_t i, p, val;
+	uint64_t j, q;
 
 	for (i = 1, p = 1; i <= MAX_NUM; i ++) {
 		if (rte_align32pow2(i) != p)
@@ -88,6 +89,29 @@ test_align(void)
 			p <<= 1;
 	}
 
+	for (i = 1, p = 1; i <= MAX_NUM; i++) {
+		if (rte_align32prevpow2(i) != p)
+			FAIL_ALIGN("rte_align32prevpow2", i, p);
+		if (rte_is_power_of_2(i + 1))
+			p = i + 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64pow2(j) != q)
+			printf("rte_align64pow2() test failed: %lu %lu\n", j,
+					q);
+		if (j == q)
+			q <<= 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64prevpow2(j) != q)
+			printf("rte_align64prevpow2() test failed: %lu %lu\n",
+					j, q);
+		if (rte_is_power_of_2(j + 1))
+			q = j + 1;
+	}
+
 	for (p = 2; p <= MAX_NUM; p <<= 1) {
 
 		if (!rte_is_power_of_2(p))
-- 
2.16.3

Re: [PATCH v3 2/2] test: update common auto test

[Thomas Monjalon]

04/04/2018 12:16, Pavan Nikhilesh:
> Update common auto test to include test for previous power of 2 for both
> 32 and 64bit integers.

This patch can be merged with previous one (related lib change).

[...]
> +	for (i = 1, p = 1; i <= MAX_NUM; i++) {
> +		if (rte_align32prevpow2(i) != p)
> +			FAIL_ALIGN("rte_align32prevpow2", i, p);
> +		if (rte_is_power_of_2(i + 1))
> +			p = i + 1;
> +	}
> +
> +	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
> +		if (rte_align64pow2(j) != q)

You could create FAIL_ALIGN64 for consistency.

> +			printf("rte_align64pow2() test failed: %lu %lu\n", j,

%lu does not work on 32-bit machines.
Please use PRIu64.

See http://dpdk.org/ml/archives/dev/2018-February/090882.html

Re: [PATCH v3 2/2] test: update common auto test

[Pavan Nikhilesh]

On Wed, Apr 04, 2018 at 02:49:28PM +0200, Thomas Monjalon wrote:
> 04/04/2018 12:16, Pavan Nikhilesh:
> > Update common auto test to include test for previous power of 2 for both
> > 32 and 64bit integers.
>
> This patch can be merged with previous one (related lib change).
>
> [...]
> > +	for (i = 1, p = 1; i <= MAX_NUM; i++) {
> > +		if (rte_align32prevpow2(i) != p)
> > +			FAIL_ALIGN("rte_align32prevpow2", i, p);
> > +		if (rte_is_power_of_2(i + 1))
> > +			p = i + 1;
> > +	}
> > +
> > +	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
> > +		if (rte_align64pow2(j) != q)
>
> You could create FAIL_ALIGN64 for consistency.
>
> > +			printf("rte_align64pow2() test failed: %lu %lu\n", j,
>
> %lu does not work on 32-bit machines.
> Please use PRIu64.
>
> See http://dpdk.org/ml/archives/dev/2018-February/090882.html
>

Thanks for the headsup will send out next version with suggested changes.

Pavan.

>
>

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

Hi Pavan

Shouldn't the new APIs be tagged with the experimental tag as agreed?

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

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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).
Besides it's a simple proven math API I don't think it will change anytime
soon.

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
>

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

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?

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

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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://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
> > >

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

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? 

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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.

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

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.

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

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.

Re: [PATCH v3 1/2] eal: add API to align integer to previous power of 2

[Matan Azrad]

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.

[PATCH v4] eal: add API to align integer to previous power of 2

[Pavan Nikhilesh]

Add 32b and 64b API's to align the given integer to the previous power
of 2. Update common auto test to include test for previous power of 2 for
both 32 and 64bit integers.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>
---
 v4 Changes:
 - Squash patchset into a single patch.
 - Use %PRIu64 instead of %lu

 lib/librte_eal/common/include/rte_common.h | 92 ++++++++++++++++++++++++++----
 test/test/test_common.c                    | 26 +++++++++
 2 files changed, 107 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 **********/

 /**
diff --git a/test/test/test_common.c b/test/test/test_common.c
index d0342430f..7361693b8 100644
--- a/test/test/test_common.c
+++ b/test/test/test_common.c
@@ -3,6 +3,7 @@
  */

 #include <stdio.h>
+#include <inttypes.h>
 #include <string.h>
 #include <math.h>
 #include <rte_common.h>
@@ -70,6 +71,9 @@ test_align(void)
 #define FAIL_ALIGN(x, i, p)\
 	{printf(x "() test failed: %u %u\n", i, p);\
 	return -1;}
+#define FAIL_ALIGN64(x, j, q)\
+	{printf(x "() test failed: %"PRIu64" %"PRIu64"\n", j, q);\
+	return -1; }
 #define ERROR_FLOOR(res, i, pow) \
 		(res % pow) || 						/* check if not aligned */ \
 		((res / pow) != (i / pow))  		/* check if correct alignment */
@@ -80,6 +84,7 @@ test_align(void)
 			val / pow != (i / pow) + 1)		/* if not aligned, hence +1 */

 	uint32_t i, p, val;
+	uint64_t j, q;

 	for (i = 1, p = 1; i <= MAX_NUM; i ++) {
 		if (rte_align32pow2(i) != p)
@@ -88,6 +93,27 @@ test_align(void)
 			p <<= 1;
 	}

+	for (i = 1, p = 1; i <= MAX_NUM; i++) {
+		if (rte_align32prevpow2(i) != p)
+			FAIL_ALIGN("rte_align32prevpow2", i, p);
+		if (rte_is_power_of_2(i + 1))
+			p = i + 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64pow2(j) != q)
+			FAIL_ALIGN64("rte_align64pow2", j, q);
+		if (j == q)
+			q <<= 1;
+	}
+
+	for (j = 1, q = 1; j <= MAX_NUM ; j++) {
+		if (rte_align64prevpow2(j) != q)
+			FAIL_ALIGN64("rte_align64prevpow2", j, q);
+		if (rte_is_power_of_2(j + 1))
+			q = j + 1;
+	}
+
 	for (p = 2; p <= MAX_NUM; p <<= 1) {

 		if (!rte_is_power_of_2(p))
--
2.16.3

Re: [PATCH v4] eal: add API to align integer to previous power of 2

[Thomas Monjalon]

04/04/2018 15:20, Pavan Nikhilesh:
> Add 32b and 64b API's to align the given integer to the previous power
> of 2. Update common auto test to include test for previous power of 2 for
> both 32 and 64bit integers.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@caviumnetworks.com>

Applied, thanks