Re: [Qemu-trivial] [Qemu-devel] [PATCH 7/7] migration: do floating-point division

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> Dividing integer expressions transferred_bytes and time_spent, and then converting
> the integer quotient to type double. Any remainder, or fractional part of the
> quotient, is ignored.  Fix this.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration/migration.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b3adbc6..6db75b8 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -646,7 +646,7 @@ static void *migration_thread(void *opaque)
>          if (current_time >= initial_time + BUFFER_DELAY) {
>              uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
>              uint64_t time_spent = current_time - initial_time;
> -            double bandwidth = transferred_bytes / time_spent;
> +            double bandwidth = (double)transferred_bytes / time_spent;
>              max_size = bandwidth * migrate_max_downtime() / 1000000;
>  
>              s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /

This feels like it would be better to fix this by merging it into
the s->mbps calculation just off the bottom; we currently have:

            uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
            uint64_t time_spent = current_time - initial_time;
            double bandwidth = transferred_bytes / time_spent;
            max_size = bandwidth * migrate_max_downtime() / 1000000;

            s->mbps = time_spent ? (((double) transferred_bytes * 8.0) /
                    ((double) time_spent / 1000.0)) / 1000.0 / 1000.0 : -1;

 
Note that the mbps has a check for time_spent being 0 - if that can ever happen,
how come 'bandwidth' has never triggered it?
 
  transferred_bytes    -    bytes
  time_spent           -    ms
  bandwidth            -    bytes/ms
  migrate_max_downtime -    in ns
  s->mbps              -    mbit/s

giving

  max_size = bytes/ms * time-in-ns  / 1000000
           = bytes/ms * time-in-ms
           = bytes

so how about something like:
            uint64_t transferred_bytes = qemu_ftell(s->file) - initial_bytes;
            uint64_t time_spent = current_time - initial_time;
            double   bytes_s    = (double) transferred_bytes / ((double) time_spent / 1000.0));
            s->mbps = (bytes_s * 8.0) / 1000000.0;
            max_size = bytes_s * (migrate_max_downtime() / 1000000000.0);

that also needs the trace fixing and the line a few lines below, I *think* we have
   dirty_bytes_rate is in bytes/second ? (arch_init.c)
   expected_downtime in ms ?
                s->expected_downtime = s->dirty_bytes_rate / bandwidth;
so,  bytes/s / bytes/ms    erm that's supposed to come out as time in ms

                s->expected_downtime = (int64_t)(1000 * (double)s->dirty_bytes_rate / bytes_s);

Yeuch.

Dave

> -- 
> 1.8.3.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK