DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] test/common: fix log2 check
@ 2019-12-04 20:52 David Marchand
  2019-12-04 21:19 ` Aaron Conole
  2019-12-20 14:01 ` [dpdk-dev] [PATCH v2] " David Marchand
  0 siblings, 2 replies; 8+ messages in thread
From: David Marchand @ 2019-12-04 20:52 UTC (permalink / raw)
  To: dev; +Cc: aconole, stable

We recently started to get random failures on the common_autotest ut with
clang on Ubuntu 16.04.6.

Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424

Wrong rte_log2_u64(0) val 0, expected ffffffff
Test Failed

The ut passes 0 to log2() to get an expected value.

Quoting log2 / log(3) manual:
If x is zero, then a pole error occurs, and the functions return
-HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.

rte_log2_uXX helpers handle 0 as a special value and return 0.
Let's have dedicated tests for this case.

Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 app/test/test_common.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 2b856f8ba5..12bd1cad90 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -216,7 +216,19 @@ test_log2(void)
 	const uint32_t max = 0x10000;
 	const uint32_t step = 1;
 
-	for (i = 0; i < max; i = i + step) {
+	compare = rte_log2_u32(0);
+	if (compare != 0) {
+		printf("Wrong rte_log2_u32(0) val %x, expected 0\n", compare);
+		return TEST_FAILED;
+	}
+
+	compare = rte_log2_u64(0);
+	if (compare != 0) {
+		printf("Wrong rte_log2_u64(0) val %x, expected 0\n", compare);
+		return TEST_FAILED;
+	}
+
+	for (i = 1; i < max; i = i + step) {
 		uint64_t i64;
 
 		/* extend range for 64-bit */
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] test/common: fix log2 check
  2019-12-04 20:52 [dpdk-dev] [PATCH] test/common: fix log2 check David Marchand
@ 2019-12-04 21:19 ` Aaron Conole
  2019-12-05  8:31   ` David Marchand
  2019-12-20 14:01 ` [dpdk-dev] [PATCH v2] " David Marchand
  1 sibling, 1 reply; 8+ messages in thread
From: Aaron Conole @ 2019-12-04 21:19 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, stable

David Marchand <david.marchand@redhat.com> writes:

> We recently started to get random failures on the common_autotest ut with
> clang on Ubuntu 16.04.6.
>
> Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424
>
> Wrong rte_log2_u64(0) val 0, expected ffffffff
> Test Failed
>
> The ut passes 0 to log2() to get an expected value.
>
> Quoting log2 / log(3) manual:
> If x is zero, then a pole error occurs, and the functions return
> -HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.
>
> rte_log2_uXX helpers handle 0 as a special value and return 0.
> Let's have dedicated tests for this case.
>
> Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Acked-by: Aaron Conole <aconole@redhat.com>

Somethings that concern me:

1.  A log2(0) should probably be an undetermined value, but this
    effectively makes log2(0) == log2(1) so that if anyone uses these
    for some mathematical work, it will have an exceptional behavior.  I
    know it's documented from a programmer perspective, but I am all for
    documenting the mathematical side effect as well.

2.  Why hasn't this been complaining for so long?  Or has it and we just
    haven't noticed?  Were some compiler flags changed recently? (maybe
    -funsafe-math was always set or something?)

-Aaron


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] test/common: fix log2 check
  2019-12-04 21:19 ` Aaron Conole
@ 2019-12-05  8:31   ` David Marchand
  2019-12-05 14:39     ` Aaron Conole
  0 siblings, 1 reply; 8+ messages in thread
From: David Marchand @ 2019-12-05  8:31 UTC (permalink / raw)
  To: Aaron Conole; +Cc: dev, dpdk stable

On Wed, Dec 4, 2019 at 10:20 PM Aaron Conole <aconole@redhat.com> wrote:
>
> David Marchand <david.marchand@redhat.com> writes:
>
> > We recently started to get random failures on the common_autotest ut with
> > clang on Ubuntu 16.04.6.
> >
> > Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424
> >
> > Wrong rte_log2_u64(0) val 0, expected ffffffff
> > Test Failed
> >
> > The ut passes 0 to log2() to get an expected value.
> >
> > Quoting log2 / log(3) manual:
> > If x is zero, then a pole error occurs, and the functions return
> > -HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.
> >
> > rte_log2_uXX helpers handle 0 as a special value and return 0.
> > Let's have dedicated tests for this case.
> >
> > Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
>
> Acked-by: Aaron Conole <aconole@redhat.com>
>
> Somethings that concern me:
>
> 1.  A log2(0) should probably be an undetermined value, but this
>     effectively makes log2(0) == log2(1) so that if anyone uses these
>     for some mathematical work, it will have an exceptional behavior.  I
>     know it's documented from a programmer perspective, but I am all for
>     documenting the mathematical side effect as well.

What do you have in mind, adding a big warning in the doxygen "THIS IS
NOT REAL MATH STUFF" ? :-)


>
> 2.  Why hasn't this been complaining for so long?  Or has it and we just
>     haven't noticed?  Were some compiler flags changed recently? (maybe
>     -funsafe-math was always set or something?)

Yes, I wondered too how we did not see this earlier.
Even now, it happens randomly.

libc log2(0) is not undefined itself, it returns -infinity.
Looking at the test code, ceilf (I would expect ceil) returns
-infinity when getting it passed.
So I'd say the problem lies in the cast to uint32_t.

Both gcc and clang do not seem to bother with standard compilation
flags, and the cast gives 0 on my RHEL.

#include <stdio.h>
#include <inttypes.h>
#include <math.h>

int main(int argc, char *argv[])
{
    printf("%lf %f %"PRIu32"\n", log2(0), (float)log2(0), (uint32_t)log2(0));
    return 0;
}

$ ./log2
-inf -inf 0


Now, if I use UBSAN with clang, I get a complaint at runtime:
$ ./log2
(/home/dmarchan/log2+0x41dfa5): runtime error: value -inf is outside
the range of representable values of type 'unsigned int'
-inf -inf 0

Not sure if it explains the random failures, but won't undefined
behavior eat your babies?


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [PATCH] test/common: fix log2 check
  2019-12-05  8:31   ` David Marchand
@ 2019-12-05 14:39     ` Aaron Conole
  0 siblings, 0 replies; 8+ messages in thread
From: Aaron Conole @ 2019-12-05 14:39 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, dpdk stable

David Marchand <david.marchand@redhat.com> writes:

> On Wed, Dec 4, 2019 at 10:20 PM Aaron Conole <aconole@redhat.com> wrote:
>>
>> David Marchand <david.marchand@redhat.com> writes:
>>
>> > We recently started to get random failures on the common_autotest ut with
>> > clang on Ubuntu 16.04.6.
>> >
>> > Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424
>> >
>> > Wrong rte_log2_u64(0) val 0, expected ffffffff
>> > Test Failed
>> >
>> > The ut passes 0 to log2() to get an expected value.
>> >
>> > Quoting log2 / log(3) manual:
>> > If x is zero, then a pole error occurs, and the functions return
>> > -HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.
>> >
>> > rte_log2_uXX helpers handle 0 as a special value and return 0.
>> > Let's have dedicated tests for this case.
>> >
>> > Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
>> > Cc: stable@dpdk.org
>> >
>> > Signed-off-by: David Marchand <david.marchand@redhat.com>
>> > ---
>>
>> Acked-by: Aaron Conole <aconole@redhat.com>
>>
>> Somethings that concern me:
>>
>> 1.  A log2(0) should probably be an undetermined value, but this
>>     effectively makes log2(0) == log2(1) so that if anyone uses these
>>     for some mathematical work, it will have an exceptional behavior.  I
>>     know it's documented from a programmer perspective, but I am all for
>>     documenting the mathematical side effect as well.
>
> What do you have in mind, adding a big warning in the doxygen "THIS IS
> NOT REAL MATH STUFF" ? :-)

Is such a warning not reasonable?  :-)

>>
>> 2.  Why hasn't this been complaining for so long?  Or has it and we just
>>     haven't noticed?  Were some compiler flags changed recently? (maybe
>>     -funsafe-math was always set or something?)
>
> Yes, I wondered too how we did not see this earlier.
> Even now, it happens randomly.
>
> libc log2(0) is not undefined itself, it returns -infinity.
> Looking at the test code, ceilf (I would expect ceil) returns
> -infinity when getting it passed.
> So I'd say the problem lies in the cast to uint32_t.
>
> Both gcc and clang do not seem to bother with standard compilation
> flags, and the cast gives 0 on my RHEL.
>
> #include <stdio.h>
> #include <inttypes.h>
> #include <math.h>
>
> int main(int argc, char *argv[])
> {
>     printf("%lf %f %"PRIu32"\n", log2(0), (float)log2(0), (uint32_t)log2(0));
>     return 0;
> }
>
> $ ./log2
> -inf -inf 0
>
>
> Now, if I use UBSAN with clang, I get a complaint at runtime:
> $ ./log2
> (/home/dmarchan/log2+0x41dfa5): runtime error: value -inf is outside
> the range of representable values of type 'unsigned int'
> -inf -inf 0
>
> Not sure if it explains the random failures, but won't undefined
> behavior eat your babies?

Possibly.  I would still expect it to be consistent when it eats babies,
but maybe it doesn't have to be.


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [PATCH v2] test/common: fix log2 check
  2019-12-04 20:52 [dpdk-dev] [PATCH] test/common: fix log2 check David Marchand
  2019-12-04 21:19 ` Aaron Conole
@ 2019-12-20 14:01 ` David Marchand
  2019-12-20 14:06   ` [dpdk-dev] [dpdk-stable] " David Marchand
  2019-12-20 14:43   ` David Marchand
  1 sibling, 2 replies; 8+ messages in thread
From: David Marchand @ 2019-12-20 14:01 UTC (permalink / raw)
  To: dev; +Cc: aconole, stable

We recently started to get random failures on the common_autotest ut with
clang on Ubuntu 16.04.6.

Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424

Wrong rte_log2_u64(0) val 0, expected ffffffff
Test Failed

The ut passes 0 to log2() to get an expected value.

Quoting log2 / log(3) manual:
If x is zero, then a pole error occurs, and the functions return
-HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.

rte_log2_uXX helpers handle 0 as a special value and return 0.
Let's have dedicated tests for this case.

Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Aaron Conole <aconole@redhat.com>
---
Changelog since v1:
- added comment in rte_log2_uXX API descriptions,

---
 app/test/test_common.c                     | 14 +++++++++++++-
 lib/librte_eal/common/include/rte_common.h |  6 ++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/app/test/test_common.c b/app/test/test_common.c
index 2b856f8ba5..12bd1cad90 100644
--- a/app/test/test_common.c
+++ b/app/test/test_common.c
@@ -216,7 +216,19 @@ test_log2(void)
 	const uint32_t max = 0x10000;
 	const uint32_t step = 1;
 
-	for (i = 0; i < max; i = i + step) {
+	compare = rte_log2_u32(0);
+	if (compare != 0) {
+		printf("Wrong rte_log2_u32(0) val %x, expected 0\n", compare);
+		return TEST_FAILED;
+	}
+
+	compare = rte_log2_u64(0);
+	if (compare != 0) {
+		printf("Wrong rte_log2_u64(0) val %x, expected 0\n", compare);
+		return TEST_FAILED;
+	}
+
+	for (i = 1; i < max; i = i + step) {
 		uint64_t i64;
 
 		/* extend range for 64-bit */
diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
index 459d082d14..7a98071ffe 100644
--- a/lib/librte_eal/common/include/rte_common.h
+++ b/lib/librte_eal/common/include/rte_common.h
@@ -538,6 +538,9 @@ rte_bsf32_safe(uint64_t v, uint32_t *pos)
 /**
  * Return the rounded-up log2 of a integer.
  *
+ * @note Contrary to the logarithm mathematical operation,
+ * rte_log2_u32(0) == 0 and not -inf.
+ *
  * @param v
  *     The input parameter.
  * @return
@@ -632,6 +635,9 @@ rte_fls_u64(uint64_t x)
 /**
  * Return the rounded-up log2 of a 64-bit integer.
  *
+ * @note Contrary to the logarithm mathematical operation,
+ * rte_log2_u32(0) == 0 and not -inf.
+ *
  * @param v
  *     The input parameter.
  * @return
-- 
2.23.0


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/common: fix log2 check
  2019-12-20 14:01 ` [dpdk-dev] [PATCH v2] " David Marchand
@ 2019-12-20 14:06   ` David Marchand
  2019-12-20 14:43   ` David Marchand
  1 sibling, 0 replies; 8+ messages in thread
From: David Marchand @ 2019-12-20 14:06 UTC (permalink / raw)
  To: dev; +Cc: Aaron Conole, dpdk stable

On Fri, Dec 20, 2019 at 3:02 PM David Marchand
<david.marchand@redhat.com> wrote:
> diff --git a/lib/librte_eal/common/include/rte_common.h b/lib/librte_eal/common/include/rte_common.h
> index 459d082d14..7a98071ffe 100644
> --- a/lib/librte_eal/common/include/rte_common.h
> +++ b/lib/librte_eal/common/include/rte_common.h
> @@ -538,6 +538,9 @@ rte_bsf32_safe(uint64_t v, uint32_t *pos)
>  /**
>   * Return the rounded-up log2 of a integer.
>   *
> + * @note Contrary to the logarithm mathematical operation,
> + * rte_log2_u32(0) == 0 and not -inf.
> + *
>   * @param v
>   *     The input parameter.
>   * @return
> @@ -632,6 +635,9 @@ rte_fls_u64(uint64_t x)
>  /**
>   * Return the rounded-up log2 of a 64-bit integer.
>   *
> + * @note Contrary to the logarithm mathematical operation,
> + * rte_log2_u32(0) == 0 and not -inf.

_u64*
Will fix while applying.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/common: fix log2 check
  2019-12-20 14:01 ` [dpdk-dev] [PATCH v2] " David Marchand
  2019-12-20 14:06   ` [dpdk-dev] [dpdk-stable] " David Marchand
@ 2019-12-20 14:43   ` David Marchand
  2019-12-20 14:52     ` Aaron Conole
  1 sibling, 1 reply; 8+ messages in thread
From: David Marchand @ 2019-12-20 14:43 UTC (permalink / raw)
  To: dev; +Cc: Aaron Conole, dpdk stable

On Fri, Dec 20, 2019 at 3:02 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> We recently started to get random failures on the common_autotest ut with
> clang on Ubuntu 16.04.6.
>
> Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424
>
> Wrong rte_log2_u64(0) val 0, expected ffffffff
> Test Failed
>
> The ut passes 0 to log2() to get an expected value.
>
> Quoting log2 / log(3) manual:
> If x is zero, then a pole error occurs, and the functions return
> -HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.
>
> rte_log2_uXX helpers handle 0 as a special value and return 0.
> Let's have dedicated tests for this case.
>
> Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Aaron Conole <aconole@redhat.com>

Applied with mentioned fix.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [dpdk-stable] [PATCH v2] test/common: fix log2 check
  2019-12-20 14:43   ` David Marchand
@ 2019-12-20 14:52     ` Aaron Conole
  0 siblings, 0 replies; 8+ messages in thread
From: Aaron Conole @ 2019-12-20 14:52 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, dpdk stable

David Marchand <david.marchand@redhat.com> writes:

> On Fri, Dec 20, 2019 at 3:02 PM David Marchand
> <david.marchand@redhat.com> wrote:
>>
>> We recently started to get random failures on the common_autotest ut with
>> clang on Ubuntu 16.04.6.
>>
>> Example: https://travis-ci.com/DPDK/dpdk/jobs/263177424
>>
>> Wrong rte_log2_u64(0) val 0, expected ffffffff
>> Test Failed
>>
>> The ut passes 0 to log2() to get an expected value.
>>
>> Quoting log2 / log(3) manual:
>> If x is zero, then a pole error occurs, and the functions return
>> -HUGE_VAL, -HUGE_VALF, or -HUGE_VALL, respectively.
>>
>> rte_log2_uXX helpers handle 0 as a special value and return 0.
>> Let's have dedicated tests for this case.
>>
>> Fixes: 05c4345ef5c2 ("test: add unit test for integer log2 function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: David Marchand <david.marchand@redhat.com>
>> Acked-by: Aaron Conole <aconole@redhat.com>
>
> Applied with mentioned fix.

Thanks!


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-12-20 14:52 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-04 20:52 [dpdk-dev] [PATCH] test/common: fix log2 check David Marchand
2019-12-04 21:19 ` Aaron Conole
2019-12-05  8:31   ` David Marchand
2019-12-05 14:39     ` Aaron Conole
2019-12-20 14:01 ` [dpdk-dev] [PATCH v2] " David Marchand
2019-12-20 14:06   ` [dpdk-dev] [dpdk-stable] " David Marchand
2019-12-20 14:43   ` David Marchand
2019-12-20 14:52     ` Aaron Conole

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).