[dpdk-stable] [dpdk-dev] [PATCH] eal: fix large multiple calculation in reciprocal division

[dpdk-stable] [dpdk-dev] [PATCH] eal: fix large multiple calculation in reciprocal division

[pbhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Fix large multiple calculation in 64 bit reciprocal division.

Fixes: 6d45659eacb8 ("eal: add u64-bit variant for reciprocal divide")
Cc: stable@dpdk.org

Reported-by: Stefan Kanthak <stefan.kanthak@nexgo.de>
Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---
 lib/librte_eal/common/rte_reciprocal.c | 76 ++++++--------------------
 1 file changed, 16 insertions(+), 60 deletions(-)

diff --git a/lib/librte_eal/common/rte_reciprocal.c b/lib/librte_eal/common/rte_reciprocal.c
index f017d0c28..920fbcdbe 100644
--- a/lib/librte_eal/common/rte_reciprocal.c
+++ b/lib/librte_eal/common/rte_reciprocal.c
@@ -60,72 +60,28 @@ struct rte_reciprocal rte_reciprocal_value(uint32_t d)
 }
 
 /*
- * Code taken from Hacker's Delight:
- * http://www.hackersdelight.org/hdcodetxt/divlu.c.txt
+ * Code based on from Hacker's Delight:
+ * http://www.hackersdelight.org/hdcodetxt/divluh.c.txt
  * License permits inclusion here per:
  * http://www.hackersdelight.org/permissions.htm
  */
 static uint64_t
-divide_128_div_64_to_64(uint64_t u1, uint64_t u0, uint64_t v, uint64_t *r)
+divlluh(uint64_t x, uint64_t y, uint64_t z)
 {
-	const uint64_t b = (1ULL << 32); /* Number base (16 bits). */
-	uint64_t un1, un0,           /* Norm. dividend LSD's. */
-		 vn1, vn0,           /* Norm. divisor digits. */
-		 q1, q0,             /* Quotient digits. */
-		 un64, un21, un10,   /* Dividend digit pairs. */
-		 rhat;               /* A remainder. */
-	int s;                       /* Shift amount for norm. */
-
-	/* If overflow, set rem. to an impossible value. */
-	if (u1 >= v) {
-		if (r != NULL)
-			*r = (uint64_t) -1;
-		return (uint64_t) -1;
-	}
-
-	/* Count leading zeros. */
-	s = __builtin_clzll(v);
-	if (s > 0) {
-		v = v << s;
-		un64 = (u1 << s) | ((u0 >> (64 - s)) & (-s >> 31));
-		un10 = u0 << s;
-	} else {
-
-		un64 = u1 | u0;
-		un10 = u0;
-	}
-
-	vn1 = v >> 32;
-	vn0 = v & 0xFFFFFFFF;
-
-	un1 = un10 >> 32;
-	un0 = un10 & 0xFFFFFFFF;
-
-	q1 = un64/vn1;
-	rhat = un64 - q1*vn1;
-again1:
-	if (q1 >= b || q1*vn0 > b*rhat + un1) {
-		q1 = q1 - 1;
-		rhat = rhat + vn1;
-		if (rhat < b)
-			goto again1;
-	}
-
-	un21 = un64*b + un1 - q1*v;
-
-	q0 = un21/vn1;
-	rhat = un21 - q0*vn1;
-again2:
-	if (q0 >= b || q0*vn0 > b*rhat + un0) {
-		q0 = q0 - 1;
-		rhat = rhat + vn1;
-		if (rhat < b)
-			goto again2;
+	int64_t i;
+	uint64_t t;
+
+	for (i = 1; i <= 64; i++) {
+		t = x >> 63;
+		x = (x << 1) | (y >> 63);
+		y = y << 1;
+		if ((x | t) >= z) {
+			x = x - z;
+			y = y + 1;
+		}
 	}
 
-	if (r != NULL)
-		*r = (un21*b + un0 - q0*v) >> s;
-	return q1*b + q0;
+	return y;
 }
 
 struct rte_reciprocal_u64
@@ -137,7 +93,7 @@ rte_reciprocal_value_u64(uint64_t d)
 
 	l = 63 - __builtin_clzll(d);
 
-	m = divide_128_div_64_to_64((1ULL << l), 0, d, NULL) << 1;
+	m = divlluh((1ULL << l), 0, d) << 1;
 	m = (1ULL << l) - d ? m + 2 : 1;
 	R.m = m;
 
-- 
2.21.0

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix large multiple calculation in reciprocal division

[Stephen Hemminger]

On Sun, 14 Apr 2019 10:52:59 +0530
<pbhagavatula@marvell.com> wrote:

> +	int64_t i;
Since i only does 1..64 it can be int.

> +	uint64_t t;
> +
> +	for (i = 1; i <= 64; i++) {
> +		t = x >> 63;
> +		x = (x << 1) | (y >> 63);
> +		y = y << 1;
> +		if ((x | t) >= z) {
> +			x = x - z;
> +			y = y + 1;

Re: [dpdk-stable] [dpdk-dev] [PATCH] eal: fix large multiple calculation in reciprocal division

[Stefan Kanthak]

Pavan Nikhilesh <pbhagavatula@marvell.com> wrote Sunday, April 14, 2019 7:22 AM:


> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Fix large multiple calculation in 64 bit reciprocal division.

This "fix" has a SERIOUS bug, it needs yet another fix.
I recommend to understand code first before you copy it!

[...]

> + int64_t i;

uint i = 64;

> + uint64_t t;
> +
> + for (i = 1; i <= 64; i++) {

do {

> + t = x >> 63;

t = (int64_t) x >> 63; // t is either 0ULL or ~0ULL

> + x = (x << 1) | (y >> 63);
> + y = y << 1;
> + if ((x | t) >= z) {
> + x = x - z;
> + y = y + 1;
> + }

} while (--i > 0);

Stefan Kanthak