Re: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
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 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: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
> On Feb 19, 2018, at 8:13 AM, Matan Azrad wrote: > > Hi Wiles > > From: Wiles, Keith, Monday, February 19, 2018 3:56 PM >>> On Feb 19, 2018, at 12:03 AM, Matan Azrad 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: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
Hi Wiles From: Wiles, Keith, Monday, February 19, 2018 3:56 PM > > On Feb 19, 2018, at 12:03 AM, Matan Azrad 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: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
> On Feb 19, 2018, at 12:03 AM, Matan Azrad 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: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
Hi Matan, On Sun, Feb 18, 2018 at 06:11:20AM +, 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 > > --- > > 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
Re: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
> From: Wiles, Keith, Sunday, February 18, 2018 5:39 PM > > On Feb 18, 2018, at 12:11 AM, 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 > >> --- > >> 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: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
> On Feb 18, 2018, at 12:11 AM, 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 >> --- >> 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
Re: [dpdk-dev] [PATCH 1/2] eal: add API to align integer to previous power of 2
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 > --- > 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