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