Re: [PATCH v3] migration: Fix possible division by zero on calc expected downtime

[Fabiano Rosas <farosas@suse.de>]

Peter Xu <peterx@redhat.com> writes:

> Commit dd4fe8844b changed the reporting of expected downtime behavior, so
> that the value will be calculated on-demand.  One side effect on the change
> is QEMU will allow the calculation to happen anytime even if there's no
> transfer happening for a short while.
>
> PeterM reported an ubsan report from clang when running migration-test with
> aarch64 binary on x86_64 hosts.  I can also reproduce if I run the test
> concurrently so some of the src QEMU may not get chance to push any data,
> causing mbps to be 0:
>
> ../migration/migration.c:1051:12: runtime error: -nan is outside the range of representable values of type 'long'
>
> Fix it by properly handle both Inf and Nan to return INT64_MAX.
>
> Add a rich comment, per PeterM's suggestion.
>
> Link: https://lore.kernel.org/r/CAFEAcA-MYH6C39xO0OLx4-M5pKurJpurwRsMqZe9q=W-NShAbw@mail.gmail.com
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Fixes: dd4fe8844b ("migration: Calculate expected downtime on demand")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index b6f78eb3ac..05f10e4576 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -63,6 +63,7 @@
>  #include "system/dirtylimit.h"
>  #include "qemu/sockets.h"
>  #include "system/kvm.h"
> +#include "math.h"
>  
>  #define NOTIFIER_ELEM_INIT(array, elem)    \
>      [elem] = NOTIFIER_WITH_RETURN_LIST_INITIALIZER((array)[elem])
> @@ -1044,12 +1045,29 @@ static bool migrate_show_downtime(MigrationState *s)
>  /* Return expected downtime (unit: milliseconds) */
>  int64_t migration_downtime_calc_expected(MigrationState *s)
>  {
> +    double expected_ms;
> +
>      if (mig_stats.dirty_sync_count <= 1) {
>          return migrate_downtime_limit();
>      }
>  
> -    return mig_stats.dirty_bytes_last_sync /
> +    expected_ms = mig_stats.dirty_bytes_last_sync /
>          migration_get_switchover_bw(s) * 1000;
> +
> +    /*
> +     * If we haven't been able to transfer any data, the result here could
> +     * be NaN (for 0 / 0) or infinity (something else / 0).
> +     *
> +     * Return INT64_MAX as our best approximation to "this will take
> +     * forever to complete". If the problem is transient (e.g. we just
> +     * haven't started to transfer yet) we'll recalculate to a more
> +     * accurate figure later.
> +     */
> +    if (isnan(expected_ms) || expected_ms >= (double)INT64_MAX) {
> +        return INT64_MAX;
> +    }
> +
> +    return (int64_t) expected_ms;
>  }
>  
>  static void populate_time_info(MigrationInfo *info, MigrationState *s)

Reviewed-by: Fabiano Rosas <farosas@suse.de>