Re: [Qemu-devel] [PATCH] migration: fix rate limiting

[Juan Quintela]

Paolo Bonzini <address@hidden> wrote:
> 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.

this is the whole definition of rate-limiting O:-)
Remember how this works, with callbacks.

This was "another" of the reasons to move to a thread, to have finer
control about that.  If you look at latest patches you can see that I
pass the "amount" of space in the buffer to the producer, and plan to
just remove this calls (make completely no sense once that you moved to
a thread).


>> 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.

Later, Juan.