From mboxrd@z Thu Jan 1 00:00:00 1970 From: Pavan Nikhilesh Subject: Re: [PATCH 1/2] eal: add API to align integer to previous power of 2 Date: Mon, 19 Feb 2018 14:06:08 +0530 Message-ID: <20180219083607.GA2841@ltp-pvn> References: <20180217104934.17291-1-pbhagavatula@caviumnetworks.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org To: Matan Azrad , "jerin.jacob@caviumnetworks.com" , Thomas Monjalon Return-path: Received: from NAM03-BY2-obe.outbound.protection.outlook.com (mail-by2nam03on0070.outbound.protection.outlook.com [104.47.42.70]) by dpdk.org (Postfix) with ESMTP id 080F62BF4 for ; Mon, 19 Feb 2018 09:36:34 +0100 (CET) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > --- > > 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