[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz.
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH] utils: Reduce chance of rounding inaccuracy in qemu_strtosz. |
Date: |
Mon, 15 Mar 2021 11:28:42 +0000 |
User-agent: |
Mutt/2.0.5 (2021-01-21) |
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 :|