[Qemu-devel] [PATCH v2 0/4] migration: fix rate limiting

[Qemu-devel] [PATCH v2 0/4] migration: fix rate limiting

[Paolo Bonzini]

Here is v2 of the rate limiting fix (now fixes).  The rate-limiting
condition "s->freeze_output || s->bytes_xfer > s->xfer_limit" is
changed to "s->buffer_size > s->xfer_limit", separating the limits
used by the producer (s->buffer_size) from those used by the
consumer (s->freeze_output and s->bytes_xfer).

Paolo


Paolo Bonzini (4):
  buffered_file: reset bytes_xfer on every tick
  buffered_file: do not send more than s->bytes_xfer bytes per tick
  buffered_file: rate-limit producers based on buffer size
  buffered_file: do not automatically unfreeze output on
    buffered_put_buffer

 buffered_file.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

[Qemu-devel] [PATCH v2 1/4] buffered_file: reset bytes_xfer on every tick

[Paolo Bonzini]

Even if the socket cannot send more data right now, whenever a new
tick has started we can send xfer_limit more bytes.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 buffered_file.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index bd0f61d..49e9089 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -233,11 +233,10 @@ static void buffered_rate_tick(void *opaque)
 
     qemu_mod_timer(s->timer, qemu_get_clock_ms(rt_clock) + 100);
 
+    s->bytes_xfer = 0;
     if (s->freeze_output)
         return;
 
-    s->bytes_xfer = 0;
-
     buffered_put_buffer(s, NULL, 0, 0);
 }
 
-- 
1.7.1

[Qemu-devel] [PATCH v2 2/4] buffered_file: do not send more than s->bytes_xfer bytes per tick

[Paolo Bonzini]

Sending more was possible if the buffer was large.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 buffered_file.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 49e9089..edead5c 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -66,9 +66,9 @@ static ssize_t buffered_flush(QEMUFileBuffered *s)
     DPRINTF("flushing %zu byte(s) of data\n", s->buffer_size);
 
     while (s->bytes_xfer < s->xfer_limit && offset < s->buffer_size) {
-
+        size_t to_send = MIN(s->buffer_size - offset, s->xfer_limit - s->bytes_xfer);
         ret = migrate_fd_put_buffer(s->migration_state, s->buffer + offset,
-                                    s->buffer_size - offset);
+                                    to_send);
         if (ret == -EAGAIN) {
             DPRINTF("backend not ready, freezing\n");
             ret = 0;
-- 
1.7.1

[Qemu-devel] [PATCH v2 3/4] buffered_file: rate-limit producers based on buffer size

[Paolo Bonzini]

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.

Also fix an off-by-one error, where > was used instead of >=.  With this
fix, the condition in buffered_put_buffer is really the opposite of
"rate limit reached", so write it explicitly like that.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 buffered_file.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index edead5c..2dac99a 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -125,7 +125,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
 
     if (pos == 0 && size == 0) {
         DPRINTF("file is ready\n");
-        if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
+        if (!qemu_file_rate_limit(s->file)) {
             DPRINTF("notifying client\n");
             migrate_fd_put_ready(s->migration_state);
         }
@@ -190,10 +190,7 @@ static int buffered_rate_limit(void *opaque)
     if (ret) {
         return ret;
     }
-    if (s->freeze_output)
-        return 1;
-
-    if (s->bytes_xfer > s->xfer_limit)
+    if (s->buffer_size >= s->xfer_limit)
         return 1;
 
     return 0;
-- 
1.7.1

Re: [Qemu-devel] [PATCH v2 3/4] buffered_file: rate-limit producers based on buffer size

[Blue Swirl]

On Tue, Nov 20, 2012 at 4:45 PM, Paolo Bonzini <pbonzini@redhat.com> 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.
>
> Also fix an off-by-one error, where > was used instead of >=.  With this
> fix, the condition in buffered_put_buffer is really the opposite of
> "rate limit reached", so write it explicitly like that.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  buffered_file.c |    7 ++-----
>  1 files changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/buffered_file.c b/buffered_file.c
> index edead5c..2dac99a 100644
> --- a/buffered_file.c
> +++ b/buffered_file.c
> @@ -125,7 +125,7 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
>
>      if (pos == 0 && size == 0) {
>          DPRINTF("file is ready\n");
> -        if (!s->freeze_output && s->bytes_xfer < s->xfer_limit) {
> +        if (!qemu_file_rate_limit(s->file)) {
>              DPRINTF("notifying client\n");
>              migrate_fd_put_ready(s->migration_state);
>          }
> @@ -190,10 +190,7 @@ static int buffered_rate_limit(void *opaque)
>      if (ret) {
>          return ret;
>      }
> -    if (s->freeze_output)
> -        return 1;
> -
> -    if (s->bytes_xfer > s->xfer_limit)
> +    if (s->buffer_size >= s->xfer_limit)

Please add braces.

>          return 1;
>
>      return 0;
> --
> 1.7.1
>
>
>

Re: [Qemu-devel] [PATCH v2 3/4] buffered_file: rate-limit producers based on buffer size

[Paolo Bonzini]

Il 24/11/2012 20:53, Blue Swirl ha scritto:
>> > -    if (s->bytes_xfer > s->xfer_limit)
>> > +    if (s->buffer_size >= s->xfer_limit)
> Please add braces.

Frankly I don't care much.  This code is going to disappear at the
beginning of 1.4.

Paolo

>> >          return 1;
>> >

[Qemu-devel] [PATCH v2 4/4] buffered_file: do not automatically unfreeze output on buffered_put_buffer

[Paolo Bonzini]

Only unfreeze if the call comes from migrate_fd_put_notify.
If the output if frozen we can still add to the buffer (which
will not grow in an unbounded manner anyway, both with and
without the previous patch), but we will not try to flush until the
socket becomes writable.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 buffered_file.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/buffered_file.c b/buffered_file.c
index 2dac99a..862aa8d 100644
--- a/buffered_file.c
+++ b/buffered_file.c
@@ -109,14 +109,18 @@ static int buffered_put_buffer(void *opaque, const uint8_t *buf, int64_t pos, in
         return error;
     }
 
-    DPRINTF("unfreezing output\n");
-    s->freeze_output = 0;
-
     if (size > 0) {
         DPRINTF("buffering %d bytes\n", size - offset);
         buffered_append(s, buf, size);
     }
 
+    if (pos == 0 && size == 0) {
+        DPRINTF("unfreezing output\n");
+        s->freeze_output = 0;
+    } else if (s->freeze_output) {
+        return size;
+    }
+
     error = buffered_flush(s);
     if (error < 0) {
         DPRINTF("buffered flush error. bailing: %s\n", strerror(-error));
-- 
1.7.1