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

From: Wiles, Keith, Sunday, February 18, 2018 5:39 PM

quoted

On Feb 18, 2018, at 12:11 AM, Matan Azrad [off-list ref]

wrote:

quoted

Hi Pavan

Please see some comments below.

From: Pavan Nikhilesh, Saturday, February 17, 2018 12:50 PM

quoted

Add 32b and 64b API's to align the given integer to the previous power of

2.

quoted

quoted

Signed-off-by: Pavan Nikhilesh <redacted>
---
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.

quoted

+ */
+static inline uint32_t
+rte_align32lowpow2(uint32_t x)

What do you think about " rte_align32prevpow2"?

quoted

+{
+	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)?

quoted

	Is it as you expect?

quoted

+	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?

quoted

+}
+
/**
 * 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.

quoted

/*********** 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?

quoted

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?

quoted

Matan

Regards,
Keith