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

[Juan Quintela]

Paolo Bonzini <address@hidden> wrote:
> buffered_rate_limit is called to prevent the RAM migration callback
> from putting too much data in the buffer.  So it has to check against
> the amount of data currently in the buffer, not against the amount
> of data that has been transferred so far.
>
> s->bytes_xfer is used to communicate between successive calls of
> buffered_put_buffer.  Buffered_rate_tick resets it every now and
> then to prevent moving too much buffered data to the socket at
> once.  However, its value does not matter for the producer of the
> data.
>
> Here is the result for migrating an idle guest with 3GB of memory
> and ~360MB of non-zero memory:
>
>   migrate_set_speed       Before                After
>   ------------------------------------------------------------
>   default (32MB/sec)      ~3 sec                ~13 sec
>   infinite (10GB/sec)     ~3 sec                ~3 sec
>
> Note that before this patch, QEMU is transferring of 100 MB/sec
> despite the rate limiting.


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.

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.

Note that the migration_thread code changes all this code, and there,
the statistics are handled much better.

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.

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;