[dpdk-dev,1/2] eal: add API to align integer to previous power of 2

  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(+)
  

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
  

> 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, we should be able to prove these functions, correct?

> Matan

Regards,
Keith
  

> 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
  

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
  

> 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
  

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.
  

> 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
  

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.