[PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Eric Blake]

Not all floating point fractions are precise.  For example, the two
nearest 32-bit IEEE float values for 0.345 are 0.344999998808 and
0.34500002861, with the lower one being closer.  When our scaling unit
is 1000, that in turn can produce instances of double rounding (our
first when truncating to get the floating point fraction compared to
what the user typed, the second in converting the result of the
multiplication back to an integer), resulting in a final result 1 byte
smaller than the intuitive integer.

For the actual test failure encountered on gitlab cross-i386-user, we
can clean things up by adding in DBL_EPSILON (with IEEE double, that
is 2^-53) for all values on a scale smaller than Petabytes (that is
2^50), where our introduced error is not enough to add a full byte,
but will be enough to cause the subsequent multiplication to overshoot
rather than undershoot the nearest integer.  And ultimately, we've
already documented that fractional values are for human convenience:
if a user is worried about byte-level precision when specifying more
than 50 bits of sizing, they should already be specifying things in
bytes rather than fractions.

Fixes: cf923b783efd5 (utils: Improve qemu_strtosz() to have 64 bits of precision)
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Eric Blake <eblake@redhat.com>
---

I'm not actually sure how to kick off a gitlab CI run of this to see if
it fixes the failure originally reported at
https://gitlab.com/qemu-project/qemu/-/jobs/1090685134
Pointers welcome!

An alternative patch might be writing (uint64_t)(fraction * mul + 0.5)
(that is, introduce the fudge factor after the multiplication instead
of before).  Preferences?

 util/cutils.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/util/cutils.c b/util/cutils.c
index d89a40a8c325..c124d8165f57 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -25,6 +25,7 @@
 #include "qemu/osdep.h"
 #include "qemu/host-utils.h"
 #include <math.h>
+#include <float.h>

 #include "qemu-common.h"
 #include "qemu/sockets.h"
@@ -329,6 +330,15 @@ static int do_strtosz(const char *nptr, const char **end,
                         "is deprecated: %s", nptr);
         }
         endptr++;
+        /*
+         * Add in a fudge-factor (2^53 when double is IEEE format) for
+         * all scales less than P (2^50), so that things like
+         * 12.345M with unit 1000 produce 12345000 instead of
+         * 12344999.
+         */
+        if (mul > 1e49) {
+            fraction += DBL_EPSILON;
+        }
     } else {
         mul = suffix_mul(default_suffix, unit);
         assert(mul > 0);
-- 
2.30.1

Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Richard Henderson]

On 3/11/21 2:07 PM, Eric Blake wrote:
> +        /*
> +         * Add in a fudge-factor (2^53 when double is IEEE format) for
> +         * all scales less than P (2^50), so that things like
> +         * 12.345M with unit 1000 produce 12345000 instead of
> +         * 12344999.
> +         */
> +        if (mul > 1e49) {

The comment says less than, the code says greater than.


> An alternative patch might be writing (uint64_t)(fraction * mul + 0.5)
> (that is, introduce the fudge factor after the multiplication instead
> of before).  Preferences?

I think I would prefer this, or for further rounding error reduction, 
fma(fraction, mul, 0.5).


r~

Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Eric Blake]

On 3/13/21 3:48 PM, Richard Henderson wrote:
> On 3/11/21 2:07 PM, Eric Blake wrote:
>> +        /*
>> +         * Add in a fudge-factor (2^53 when double is IEEE format) for
>> +         * all scales less than P (2^50), so that things like
>> +         * 12.345M with unit 1000 produce 12345000 instead of
>> +         * 12344999.
>> +         */
>> +        if (mul > 1e49) {
> 
> The comment says less than, the code says greater than.

Shoot. A demonstration that I did not have an environment that actually
reproduced the bug (and my request for help in figuring out how to kick
off a gitlab CI run that would catch it).

> 
> 
>> An alternative patch might be writing (uint64_t)(fraction * mul + 0.5)
>> (that is, introduce the fudge factor after the multiplication instead
>> of before).  Preferences?
> 
> I think I would prefer this, or for further rounding error reduction,
> fma(fraction, mul, 0.5).

Indeed, fma() sounds a bit nicer at minimizing the chance for double
rounding errors.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Richard Henderson]

On 3/15/21 5:33 AM, Eric Blake wrote:
> On 3/13/21 3:48 PM, Richard Henderson wrote:
>> On 3/11/21 2:07 PM, Eric Blake wrote:
>>> +        /*
>>> +         * Add in a fudge-factor (2^53 when double is IEEE format) for
>>> +         * all scales less than P (2^50), so that things like
>>> +         * 12.345M with unit 1000 produce 12345000 instead of
>>> +         * 12344999.
>>> +         */
>>> +        if (mul > 1e49) {
>>
>> The comment says less than, the code says greater than.
> 
> Shoot. A demonstration that I did not have an environment that actually
> reproduced the bug

Any i686 vm will do.  I had a debian 10 vm hanging around in which I could 
reproduce it.


r~

Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Eric Blake]

On 3/15/21 8:18 AM, Richard Henderson wrote:
> On 3/15/21 5:33 AM, Eric Blake wrote:
>> On 3/13/21 3:48 PM, Richard Henderson wrote:
>>> On 3/11/21 2:07 PM, Eric Blake wrote:
>>>> +        /*
>>>> +         * Add in a fudge-factor (2^53 when double is IEEE format) for
>>>> +         * all scales less than P (2^50), so that things like
>>>> +         * 12.345M with unit 1000 produce 12345000 instead of
>>>> +         * 12344999.
>>>> +         */
>>>> +        if (mul > 1e49) {
>>>
>>> The comment says less than, the code says greater than.
>>
>> Shoot. A demonstration that I did not have an environment that actually
>> reproduced the bug
> 
> Any i686 vm will do.  I had a debian 10 vm hanging around in which I
> could reproduce it.

Anyways, I've now got a gitlab CI running, and even with my patch
corrected, it still failed in a different spot:
https://gitlab.com/ebblake/qemu/-/jobs/1099334550

ERROR:../tests/unit/test-cutils.c:2096:test_qemu_strtosz_units:
assertion failed (res == EiB): (1152921504606847232 == 1152921504606846976)

so I'm liking your patch in lieu of mine.
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg05290.html

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.

[Daniel P. Berrangé]

On Thu, Mar 11, 2021 at 02:07:02PM -0600, Eric Blake wrote:
> Not all floating point fractions are precise.  For example, the two
> nearest 32-bit IEEE float values for 0.345 are 0.344999998808 and
> 0.34500002861, with the lower one being closer.  When our scaling unit
> is 1000, that in turn can produce instances of double rounding (our
> first when truncating to get the floating point fraction compared to
> what the user typed, the second in converting the result of the
> multiplication back to an integer), resulting in a final result 1 byte
> smaller than the intuitive integer.
> 
> For the actual test failure encountered on gitlab cross-i386-user, we
> can clean things up by adding in DBL_EPSILON (with IEEE double, that
> is 2^-53) for all values on a scale smaller than Petabytes (that is
> 2^50), where our introduced error is not enough to add a full byte,
> but will be enough to cause the subsequent multiplication to overshoot
> rather than undershoot the nearest integer.  And ultimately, we've
> already documented that fractional values are for human convenience:
> if a user is worried about byte-level precision when specifying more
> than 50 bits of sizing, they should already be specifying things in
> bytes rather than fractions.
> 
> Fixes: cf923b783efd5 (utils: Improve qemu_strtosz() to have 64 bits of precision)
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
> 
> I'm not actually sure how to kick off a gitlab CI run of this to see if
> it fixes the failure originally reported at
> https://gitlab.com/qemu-project/qemu/-/jobs/1090685134
> Pointers welcome!

Create a gitlab.com account, and fork the QEMU repo.

Then simply push your branch to your QEMU fork. Pipeline will run
automagically and be visible in

https://gitlab.com/<your user name>/qemu/-/pipelines

> An alternative patch might be writing (uint64_t)(fraction * mul + 0.5)
> (that is, introduce the fudge factor after the multiplication instead
> of before).  Preferences?

No pref from me if both achieve the same end result.

>  util/cutils.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index d89a40a8c325..c124d8165f57 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -25,6 +25,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu/host-utils.h"
>  #include <math.h>
> +#include <float.h>
> 
>  #include "qemu-common.h"
>  #include "qemu/sockets.h"
> @@ -329,6 +330,15 @@ static int do_strtosz(const char *nptr, const char **end,
>                          "is deprecated: %s", nptr);
>          }
>          endptr++;
> +        /*
> +         * Add in a fudge-factor (2^53 when double is IEEE format) for
> +         * all scales less than P (2^50), so that things like
> +         * 12.345M with unit 1000 produce 12345000 instead of
> +         * 12344999.
> +         */
> +        if (mul > 1e49) {
> +            fraction += DBL_EPSILON;
> +        }

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|