Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[Pavan Nikhilesh Bhagavatula]

>Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> writes:
>
>>>> >
>>>> >Any review?
>>>> >
>>>> >Are we missing an update in the unit test to catch this issue?
>>>> >Thanks.
>>>>
>>>> We actually caught it in a unit test
>>>> >test_reciprocal_division
>>>
>>>We had this problem since the very start then.
>>>
>>>Both reciprocal_division and reciprocal_division_perf are in the
>"perf"
>>>list.
>>>Can they be promoted to the standard list?
>>
>> Both of these tests take >=5 min depending on the platform in use
>which is why
>> they are placed in perf tests.
>
>Is it possible to write a smaller set of tests that can be put in the
>standard list?
>

Currently we have MAX_ITERATIONS  (1ULL << 32) for both the tests
maybe we can reduce it to 1 << 16 for the normal case?

>>>
>>>
>>>--
>>>David Marchand

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[Aaron Conole]

Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> writes:

>>Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> writes:
>>
>>>>> >
>>>>> >Any review?
>>>>> >
>>>>> >Are we missing an update in the unit test to catch this issue?
>>>>> >Thanks.
>>>>>
>>>>> We actually caught it in a unit test
>>>>> >test_reciprocal_division
>>>>
>>>>We had this problem since the very start then.
>>>>
>>>>Both reciprocal_division and reciprocal_division_perf are in the
>>"perf"
>>>>list.
>>>>Can they be promoted to the standard list?
>>>
>>> Both of these tests take >=5 min depending on the platform in use
>>which is why
>>> they are placed in perf tests.
>>
>>Is it possible to write a smaller set of tests that can be put in the
>>standard list?
>>
>
> Currently we have MAX_ITERATIONS  (1ULL << 32) for both the tests
> maybe we can reduce it to 1 << 16 for the normal case?

Sounds good to me.  If you submit a patch, we can look at the run time
from the robot submission as well.

>>>>
>>>>
>>>>--
>>>>David Marchand

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[Pavan Nikhilesh Bhagavatula]

>Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> writes:
>
>>>Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> writes:
>>>
>>>>>> >
>>>>>> >Any review?
>>>>>> >
>>>>>> >Are we missing an update in the unit test to catch this issue?
>>>>>> >Thanks.
>>>>>>
>>>>>> We actually caught it in a unit test
>>>>>> >test_reciprocal_division
>>>>>
>>>>>We had this problem since the very start then.
>>>>>
>>>>>Both reciprocal_division and reciprocal_division_perf are in the
>>>"perf"
>>>>>list.
>>>>>Can they be promoted to the standard list?
>>>>
>>>> Both of these tests take >=5 min depending on the platform in use
>>>which is why
>>>> they are placed in perf tests.
>>>
>>>Is it possible to write a smaller set of tests that can be put in the
>>>standard list?
>>>
>>
>> Currently we have MAX_ITERATIONS  (1ULL << 32) for both the tests
>> maybe we can reduce it to 1 << 16 for the normal case?
>
>Sounds good to me.  If you submit a patch, we can look at the run time
>from the robot submission as well.
>

Yup, I'll send a patch.

>>>>>
>>>>>
>>>>>--
>>>>>David Marchand

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[Pavan Nikhilesh Bhagavatula]

>> >
>> >Any review?
>> >
>> >Are we missing an update in the unit test to catch this issue?
>> >Thanks.
>>
>> We actually caught it in a unit test
>> >test_reciprocal_division
>
>We had this problem since the very start then.
>
>Both reciprocal_division and reciprocal_division_perf are in the "perf"
>list.
>Can they be promoted to the standard list?

Both of these tests take >=5 min depending on the platform in use which is why
they are placed in perf tests.

>
>
>--
>David Marchand

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[Aaron Conole]

Pavan Nikhilesh Bhagavatula <pbhagavatula@marvell.com> writes:

>>> >
>>> >Any review?
>>> >
>>> >Are we missing an update in the unit test to catch this issue?
>>> >Thanks.
>>>
>>> We actually caught it in a unit test
>>> >test_reciprocal_division
>>
>>We had this problem since the very start then.
>>
>>Both reciprocal_division and reciprocal_division_perf are in the "perf"
>>list.
>>Can they be promoted to the standard list?
>
> Both of these tests take >=5 min depending on the platform in use which is why
> they are placed in perf tests.

Is it possible to write a smaller set of tests that can be put in the
standard list?

>>
>>
>>--
>>David Marchand

[dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[pbhagavatula]

From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Fix off by one error in 64bit reciprocal division when divisor is 32bit.

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

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
---

Example:
   Division failed, 17358247066007716387/244 =
   		expected 71140356827900476 result 71140356827900477
   Division failed, 17541123788887206374/41475 =
   		expected 422932460250444 result 422932460250445

 lib/librte_eal/common/rte_reciprocal.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/rte_reciprocal.c b/lib/librte_eal/common/rte_reciprocal.c
index f017d0c28..1c6d10e73 100644
--- a/lib/librte_eal/common/rte_reciprocal.c
+++ b/lib/librte_eal/common/rte_reciprocal.c
@@ -133,12 +133,15 @@ rte_reciprocal_value_u64(uint64_t d)
 {
 	struct rte_reciprocal_u64 R;
 	uint64_t m;
+	uint64_t r;
 	int l;

 	l = 63 - __builtin_clzll(d);

-	m = divide_128_div_64_to_64((1ULL << l), 0, d, NULL) << 1;
-	m = (1ULL << l) - d ? m + 2 : 1;
+	m = divide_128_div_64_to_64((1ULL << l), 0, d, &r) << 1;
+	if (r << 1 < r || r << 1 >= d)
+		m++;
+	m = (1ULL << l) - d ? m + 1 : 1;
 	R.m = m;

 	R.sh1 = l > 1 ? 1 : l;
--
2.23.0

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[David Marchand]

On Tue, Sep 3, 2019 at 9:17 PM <pbhagavatula@marvell.com> wrote:
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Fix off by one error in 64bit reciprocal division when divisor is 32bit.
>
> Fixes: 6d45659eacb8 ("eal: add u64-bit variant for reciprocal divide")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

Any review?

Are we missing an update in the unit test to catch this issue?
Thanks.

-- 
David Marchand

Re: [dpdk-dev] [PATCH] eal/reciprocal: fix off by one when divisor is 32bit

[David Marchand]

On Tue, Sep 3, 2019 at 9:17 PM <pbhagavatula@marvell.com> wrote:
>
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>
> Fix off by one error in 64bit reciprocal division when divisor is 32bit.

Added log from unit test:

    RTE>>reciprocal_division
    Validating unsigned 32bit division.
    Validating unsigned 64bit division.
    Validating unsigned 64bit division with 32bit divisor.
    Division failed, 16983222950483802557/819 = expected 20736535959076681
    result 20736535959076682
    Validating division by power of 2.
    Test Failed

>
> Fixes: 6d45659eacb8 ("eal: add u64-bit variant for reciprocal divide")
> Cc: stable@dpdk.org
>
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>

Please continue working on enhancing the functional test so that it
can be part of the standard unit test list.
Thanks.


-- 
David Marchand