[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] migration: fix rate limiting
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH] migration: fix rate limiting |
Date: |
Tue, 20 Nov 2012 15:44:05 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121016 Thunderbird/16.0.1 |
Il 20/11/2012 12:03, Juan Quintela ha scritto:
> This patch is wrong O;-)
> That don't mean that current code is right.
>
> We have 4 variables:
> - xfer_limit: how much we are allowed to send each 100ms (in bytes)
> - buffer_capacity: the size of the buffer that we are using
> - buffer_size: the amount of the previous buffer that we are using
> buffer_size < buffer_capacity, or we are doing something
> wrong.
> - bytes_xfer: How many bytes we have transfered since last 100ms timer.
Note that the buffered_file places a wall between producer and consumer
(or rather tries to place it; my patch is an attempt to fix). The
consumer side is buffered_flush & bytes_xfer, and the rate-limiting
there is mandatory.
However, on the producer side, the rate-limiting is only advisory, to
avoid making the buffer_capacity too large. In principle (and leaving
MAX_WAIT aside for a moment) you could skip qemu_file_rate_limit calls
completely. It would place the whole RAM in the buffer, and still
transfer it in xfer_limit chunks on the wire.
> And how this work:
> - we have an input handler that copies stuff from RAM to buffer
> it stops when we have sent more that xfer_limit on this period (each
> 100ms)
> - we have another handler that is run each 100ms, and this one sent
> anything that is on the buffer, and reset bytes_xfer to zero.
>
> WHat you have done is just telling that in the 1st input handler, that
> we always have size on the buffer for it, so that we are not doing any
> rate limiting at all.
>
> It is very strange that buffer_capacity is bigger that xfer_limit (it
> could happen, but it is very unusual), and then what you have done there
> is just disabling rate limiting altogether.
I haven't: once s->bytes_xfer >= s->xfer_limit, buffered_flush will not
do anything, and data will accumulate in the buffer until bytes_xfer is
moved back to 0. So what happens really is that ram_save_block is
already preparing the next chunk of data to send, but only s->xfer_limit
bytes are sent on each 100ms period.
However, my patch was incomplete. The desired behavior is that
buffered_put_buffer(s, NULL, 0, 0) will restart the iteration, so it has
to check buffer_size too. In fact, the check in buffered_put_b
There is also another bug in the current code, which is an off-by-one.
Comparison is s->bytes_xfer > s->xfer_limit, but it should be >= instead.
And more somewhat broken checks. I'm testing a more complete fix.
Paolo
>
> So ....
>
> NACK
>
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>> buffered_file.c | 2 +-
>> 1 file modificato, 1 inserzione(+). 1 rimozione(-)
>>
>> diff --git a/buffered_file.c b/buffered_file.c
>> index bd0f61d..46cd591 100644
>> --- a/buffered_file.c
>> +++ b/buffered_file.c
>> @@ -193,7 +193,7 @@ static int buffered_rate_limit(void *opaque)
>> if (s->freeze_output)
>> return 1;
>>
>> - if (s->bytes_xfer > s->xfer_limit)
> o> + if (s->buffer_size > s->xfer_limit)
>> return 1;
>>
>> return 0;