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

[Wiles, Keith]

> On Feb 18, 2018, at 12:11 AM, Matan Azrad <matan at 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 at 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