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

quoted

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

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.

The existing API i.e. rte_align32pow2() behaves in similar manner i.e. returns
0 when 0 is passed.

quoted

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

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)?
	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.

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?

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