qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Juan Quintela
Subject: Re: [Qemu-devel] [PATCH 7/7] migration: do floating-point division
Date: Mon, 26 Jan 2015 19:15:06 +0100
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.4 (gnu/linux)

"Dr. David Alan Gilbert" <address@hidden> wrote:
> * Paolo Bonzini (address@hidden) 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 <address@hidden>
>> ---
>>  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) /

Reviewed-by: Juan Quintela <address@hidden>

we had that fix on RHEL, but I forgot to push it upstream (it was not
needed as it optimized the calculations at the time).

>
> 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);

I think we can do something like this on the normal tree, or just go for
sane units left and right?


> 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 / address@hidden / Manchester, UK



reply via email to

[Prev in Thread] Current Thread [Next in Thread]