Patchwork [38/41] migration: move rate limiting to QEMUFile

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 15, 2013, 5:47 p.m.
Message ID <1360950433-17106-39-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/220800/
State New
Headers show

Comments

Paolo Bonzini - Feb. 15, 2013, 5:47 p.m.
Rate limiting is now simply a byte counter; client call
qemu_file_rate_limit() manually to determine if they have to exit.
So it is possible and simple to move the functionality to QEMUFile.

This makes the remaining functionality of s->file redundant;
in the next patch we can remove it and write directly to s->migration_file.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 docs/migration.txt            |   20 +--------------
 include/migration/qemu-file.h |   18 +-----------
 migration.c                   |   56 +---------------------------------------
 savevm.c                      |   31 ++++++++++++----------
 4 files changed, 22 insertions(+), 103 deletions(-)
Juan Quintela - Feb. 22, 2013, 11:42 a.m.
Paolo Bonzini <pbonzini@redhat.com> wrote:
> Rate limiting is now simply a byte counter; client call
> qemu_file_rate_limit() manually to determine if they have to exit.
> So it is possible and simple to move the functionality to QEMUFile.
>
> This makes the remaining functionality of s->file redundant;
> in the next patch we can remove it and write directly to s->migration_file.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>


Nice!!!

>      qemu_put_buffer(s->migration_file, buf, size);
> -    if (qemu_file_get_error(s->migration_file)) {
> -        return qemu_file_get_error(s->migration_file);
> -    }
> -
> -    s->bytes_xfer += size;
> -    return size;
> +    return qemu_file_get_error(s->migration_file);
>  }

You fix here the problem that I pointed in a previous patch, so feel
free not to fix it in the previous one.

If anyone reading this, just thinking aloud, this function is called as

    if (f->is_write && f->buf_index > 0) {
        ret = f->ops->put_buffer(f->opaque, f->buf, f->buf_offset, f->buf_index);
        if (ret >= 0) {
            f->buf_offset += f->buf_index;
        }
        f->buf_index = 0;

So the change from "size" to "0" on the non-error case don't matter.
Orit Wasserman - Feb. 22, 2013, 1:55 p.m.
On 02/15/2013 07:47 PM, Paolo Bonzini wrote:
> Rate limiting is now simply a byte counter; client call
> qemu_file_rate_limit() manually to determine if they have to exit.
> So it is possible and simple to move the functionality to QEMUFile.
> 
> This makes the remaining functionality of s->file redundant;
> in the next patch we can remove it and write directly to s->migration_file.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  docs/migration.txt            |   20 +--------------
>  include/migration/qemu-file.h |   18 +-----------
>  migration.c                   |   56 +---------------------------------------
>  savevm.c                      |   31 ++++++++++++----------
>  4 files changed, 22 insertions(+), 103 deletions(-)
> 
> diff --git a/docs/migration.txt b/docs/migration.txt
> index f3ddd2f..0719a55 100644
> --- a/docs/migration.txt
> +++ b/docs/migration.txt
> @@ -55,10 +55,7 @@ QEMUFile with:
>  QEMUFile *qemu_fopen_ops(void *opaque,
>                           QEMUFilePutBufferFunc *put_buffer,
>                           QEMUFileGetBufferFunc *get_buffer,
> -                         QEMUFileCloseFunc *close,
> -                         QEMUFileRateLimit *rate_limit,
> -                         QEMUFileSetRateLimit *set_rate_limit,
> -                         QEMUFileGetRateLimit *get_rate_limit);
> +                         QEMUFileCloseFunc *close);
>  
>  The functions have the following functionality:
>  
> @@ -80,24 +77,9 @@ Close a file and return an error code.
>  
>  typedef int (QEMUFileCloseFunc)(void *opaque);
>  
> -Called to determine if the file has exceeded its bandwidth allocation.  The
> -bandwidth capping is a soft limit, not a hard limit.
> -
> -typedef int (QEMUFileRateLimit)(void *opaque);
> -
> -Called to change the current bandwidth allocation. This function must return
> -the new actual bandwidth. It should be new_rate if everything goes OK, and
> -the old rate otherwise.
> -
> -typedef size_t (QEMUFileSetRateLimit)(void *opaque, size_t new_rate);
> -typedef size_t (QEMUFileGetRateLimit)(void *opaque);
> -
>  You can use any internal state that you need using the opaque void *
>  pointer that is passed to all functions.
>  
> -The rate limiting functions are used to limit the bandwidth used by
> -QEMU migration.
> -
>  The important functions for us are put_buffer()/get_buffer() that
>  allow to write/read a buffer into the QEMUFile.
>  
> diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
> index 25e8461..df81261 100644
> --- a/include/migration/qemu-file.h
> +++ b/include/migration/qemu-file.h
> @@ -51,26 +51,11 @@ typedef int (QEMUFileCloseFunc)(void *opaque);
>   */
>  typedef int (QEMUFileGetFD)(void *opaque);
>  
> -/* Called to determine if the file has exceeded its bandwidth allocation.  The
> - * bandwidth capping is a soft limit, not a hard limit.
> - */
> -typedef int (QEMUFileRateLimit)(void *opaque);
> -
> -/* Called to change the current bandwidth allocation. This function must return
> - * the new actual bandwidth. It should be new_rate if everything goes ok, and
> - * the old rate otherwise
> - */
> -typedef int64_t (QEMUFileSetRateLimit)(void *opaque, int64_t new_rate);
> -typedef int64_t (QEMUFileGetRateLimit)(void *opaque);
> -
>  typedef struct QEMUFileOps {
>      QEMUFilePutBufferFunc *put_buffer;
>      QEMUFileGetBufferFunc *get_buffer;
>      QEMUFileCloseFunc *close;
>      QEMUFileGetFD *get_fd;
> -    QEMUFileRateLimit *rate_limit;
> -    QEMUFileSetRateLimit *set_rate_limit;
> -    QEMUFileGetRateLimit *get_rate_limit;
>  } QEMUFileOps;
>  
>  QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
> @@ -109,7 +94,8 @@ unsigned int qemu_get_be32(QEMUFile *f);
>  uint64_t qemu_get_be64(QEMUFile *f);
>  
>  int qemu_file_rate_limit(QEMUFile *f);
> -int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
> +void qemu_file_reset_rate_limit(QEMUFile *f);
> +void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
>  int64_t qemu_file_get_rate_limit(QEMUFile *f);
>  int qemu_file_get_error(QEMUFile *f);
>  
> diff --git a/migration.c b/migration.c
> index 308214f..7c1671f 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -501,12 +501,7 @@ static int migration_put_buffer(void *opaque, const uint8_t *buf,
>      }
>  
>      qemu_put_buffer(s->migration_file, buf, size);
> -    if (qemu_file_get_error(s->migration_file)) {
> -        return qemu_file_get_error(s->migration_file);
> -    }
> -
> -    s->bytes_xfer += size;
> -    return size;
> +    return qemu_file_get_error(s->migration_file);
>  }
>  
>  static int migration_close(void *opaque)
> @@ -530,49 +525,6 @@ static int migration_get_fd(void *opaque)
>      return qemu_get_fd(s->migration_file);
>  }
>  
> -/*
> - * The meaning of the return values is:
> - *   0: We can continue sending
> - *   1: Time to stop
> - *   negative: There has been an error
> - */
> -static int migration_rate_limit(void *opaque)
> -{
> -    MigrationState *s = opaque;
> -    int ret;
> -
> -    ret = qemu_file_get_error(s->file);
> -    if (ret) {
> -        return ret;
> -    }
> -
> -    if (s->bytes_xfer >= s->xfer_limit) {
> -        return 1;
> -    }
> -
> -    return 0;
> -}
> -
> -static int64_t migration_set_rate_limit(void *opaque, int64_t new_rate)
> -{
> -    MigrationState *s = opaque;
> -    if (qemu_file_get_error(s->file)) {
> -        goto out;
> -    }
> -
> -    s->xfer_limit = new_rate;
> -
> -out:
> -    return s->xfer_limit;
> -}
> -
> -static int64_t migration_get_rate_limit(void *opaque)
> -{
> -    MigrationState *s = opaque;
> -
> -    return s->xfer_limit;
> -}
> -
>  static void *migration_thread(void *opaque)
>  {
>      MigrationState *s = opaque;
> @@ -625,7 +577,7 @@ static void *migration_thread(void *opaque)
>                      " bandwidth %g max_size %" PRId64 "\n",
>                      transferred_bytes, time_spent, bandwidth, max_size);
>  
> -            s->bytes_xfer = 0;
> +            qemu_file_reset_rate_limit(s->file);
>              initial_time = current_time;
>              initial_bytes = qemu_ftell(s->file);
>          }
> @@ -656,15 +608,11 @@ static const QEMUFileOps migration_file_ops = {
>      .get_fd =         migration_get_fd,
>      .put_buffer =     migration_put_buffer,
>      .close =          migration_close,
> -    .rate_limit =     migration_rate_limit,
> -    .get_rate_limit = migration_get_rate_limit,
> -    .set_rate_limit = migration_set_rate_limit,
>  };
>  
>  void migrate_fd_connect(MigrationState *s)
>  {
>      s->state = MIG_STATE_ACTIVE;
> -    s->bytes_xfer = 0;
>      s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
>      s->file = qemu_fopen_ops(s, &migration_file_ops);
>  
> diff --git a/savevm.c b/savevm.c
> index f704f46..6683562 100644
> --- a/savevm.c
> +++ b/savevm.c
> @@ -119,6 +119,9 @@ struct QEMUFile {
>      void *opaque;
>      int is_write;
>  
> +    int64_t bytes_xfer;
> +    int64_t xfer_limit;
> +
>      int64_t pos; /* start of buffer when writing, end of buffer
>                      when reading */
>      int buf_index;
> @@ -479,7 +482,6 @@ QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
>      f->opaque = opaque;
>      f->ops = ops;
>      f->is_write = 0;
> -
>      return f;
>  }
>  
> @@ -605,6 +607,7 @@ void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
>          memcpy(f->buf + f->buf_index, buf, l);
>          f->is_write = 1;
>          f->buf_index += l;
> +        f->bytes_xfer += l;
>          buf += l;
>          size -= l;
>          if (f->buf_index >= IO_BUF_SIZE) {
> @@ -725,28 +728,28 @@ int64_t qemu_ftell(QEMUFile *f)
>  
>  int qemu_file_rate_limit(QEMUFile *f)
>  {
> -    if (f->ops->rate_limit)
> -        return f->ops->rate_limit(f->opaque);
> -
> +    if (qemu_file_get_error(f)) {
> +        return 1;
> +    }
> +    if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
> +        return 1;
> +    }
>      return 0;
>  }
>  
>  int64_t qemu_file_get_rate_limit(QEMUFile *f)
>  {
> -    if (f->ops->get_rate_limit)
> -        return f->ops->get_rate_limit(f->opaque);
> -
> -    return 0;
> +    return f->xfer_limit;
>  }
>  
> -int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate)
> +void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
>  {
> -    /* any failed or completed migration keeps its state to allow probing of
> -     * migration data, but has no associated file anymore */
> -    if (f && f->ops->set_rate_limit)
> -        return f->ops->set_rate_limit(f->opaque, new_rate);
> +    f->xfer_limit = limit;
> +}
>  
> -    return 0;
> +void qemu_file_reset_rate_limit(QEMUFile *f)
> +{
> +    f->bytes_xfer = 0;
>  }
>  
>  void qemu_put_be16(QEMUFile *f, unsigned int v)
> 
Reviewed-by: Orit Wasserman <owasserm@redhat.com>

Patch

diff --git a/docs/migration.txt b/docs/migration.txt
index f3ddd2f..0719a55 100644
--- a/docs/migration.txt
+++ b/docs/migration.txt
@@ -55,10 +55,7 @@  QEMUFile with:
 QEMUFile *qemu_fopen_ops(void *opaque,
                          QEMUFilePutBufferFunc *put_buffer,
                          QEMUFileGetBufferFunc *get_buffer,
-                         QEMUFileCloseFunc *close,
-                         QEMUFileRateLimit *rate_limit,
-                         QEMUFileSetRateLimit *set_rate_limit,
-                         QEMUFileGetRateLimit *get_rate_limit);
+                         QEMUFileCloseFunc *close);
 
 The functions have the following functionality:
 
@@ -80,24 +77,9 @@  Close a file and return an error code.
 
 typedef int (QEMUFileCloseFunc)(void *opaque);
 
-Called to determine if the file has exceeded its bandwidth allocation.  The
-bandwidth capping is a soft limit, not a hard limit.
-
-typedef int (QEMUFileRateLimit)(void *opaque);
-
-Called to change the current bandwidth allocation. This function must return
-the new actual bandwidth. It should be new_rate if everything goes OK, and
-the old rate otherwise.
-
-typedef size_t (QEMUFileSetRateLimit)(void *opaque, size_t new_rate);
-typedef size_t (QEMUFileGetRateLimit)(void *opaque);
-
 You can use any internal state that you need using the opaque void *
 pointer that is passed to all functions.
 
-The rate limiting functions are used to limit the bandwidth used by
-QEMU migration.
-
 The important functions for us are put_buffer()/get_buffer() that
 allow to write/read a buffer into the QEMUFile.
 
diff --git a/include/migration/qemu-file.h b/include/migration/qemu-file.h
index 25e8461..df81261 100644
--- a/include/migration/qemu-file.h
+++ b/include/migration/qemu-file.h
@@ -51,26 +51,11 @@  typedef int (QEMUFileCloseFunc)(void *opaque);
  */
 typedef int (QEMUFileGetFD)(void *opaque);
 
-/* Called to determine if the file has exceeded its bandwidth allocation.  The
- * bandwidth capping is a soft limit, not a hard limit.
- */
-typedef int (QEMUFileRateLimit)(void *opaque);
-
-/* Called to change the current bandwidth allocation. This function must return
- * the new actual bandwidth. It should be new_rate if everything goes ok, and
- * the old rate otherwise
- */
-typedef int64_t (QEMUFileSetRateLimit)(void *opaque, int64_t new_rate);
-typedef int64_t (QEMUFileGetRateLimit)(void *opaque);
-
 typedef struct QEMUFileOps {
     QEMUFilePutBufferFunc *put_buffer;
     QEMUFileGetBufferFunc *get_buffer;
     QEMUFileCloseFunc *close;
     QEMUFileGetFD *get_fd;
-    QEMUFileRateLimit *rate_limit;
-    QEMUFileSetRateLimit *set_rate_limit;
-    QEMUFileGetRateLimit *get_rate_limit;
 } QEMUFileOps;
 
 QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops);
@@ -109,7 +94,8 @@  unsigned int qemu_get_be32(QEMUFile *f);
 uint64_t qemu_get_be64(QEMUFile *f);
 
 int qemu_file_rate_limit(QEMUFile *f);
-int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
+void qemu_file_reset_rate_limit(QEMUFile *f);
+void qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate);
 int64_t qemu_file_get_rate_limit(QEMUFile *f);
 int qemu_file_get_error(QEMUFile *f);
 
diff --git a/migration.c b/migration.c
index 308214f..7c1671f 100644
--- a/migration.c
+++ b/migration.c
@@ -501,12 +501,7 @@  static int migration_put_buffer(void *opaque, const uint8_t *buf,
     }
 
     qemu_put_buffer(s->migration_file, buf, size);
-    if (qemu_file_get_error(s->migration_file)) {
-        return qemu_file_get_error(s->migration_file);
-    }
-
-    s->bytes_xfer += size;
-    return size;
+    return qemu_file_get_error(s->migration_file);
 }
 
 static int migration_close(void *opaque)
@@ -530,49 +525,6 @@  static int migration_get_fd(void *opaque)
     return qemu_get_fd(s->migration_file);
 }
 
-/*
- * The meaning of the return values is:
- *   0: We can continue sending
- *   1: Time to stop
- *   negative: There has been an error
- */
-static int migration_rate_limit(void *opaque)
-{
-    MigrationState *s = opaque;
-    int ret;
-
-    ret = qemu_file_get_error(s->file);
-    if (ret) {
-        return ret;
-    }
-
-    if (s->bytes_xfer >= s->xfer_limit) {
-        return 1;
-    }
-
-    return 0;
-}
-
-static int64_t migration_set_rate_limit(void *opaque, int64_t new_rate)
-{
-    MigrationState *s = opaque;
-    if (qemu_file_get_error(s->file)) {
-        goto out;
-    }
-
-    s->xfer_limit = new_rate;
-
-out:
-    return s->xfer_limit;
-}
-
-static int64_t migration_get_rate_limit(void *opaque)
-{
-    MigrationState *s = opaque;
-
-    return s->xfer_limit;
-}
-
 static void *migration_thread(void *opaque)
 {
     MigrationState *s = opaque;
@@ -625,7 +577,7 @@  static void *migration_thread(void *opaque)
                     " bandwidth %g max_size %" PRId64 "\n",
                     transferred_bytes, time_spent, bandwidth, max_size);
 
-            s->bytes_xfer = 0;
+            qemu_file_reset_rate_limit(s->file);
             initial_time = current_time;
             initial_bytes = qemu_ftell(s->file);
         }
@@ -656,15 +608,11 @@  static const QEMUFileOps migration_file_ops = {
     .get_fd =         migration_get_fd,
     .put_buffer =     migration_put_buffer,
     .close =          migration_close,
-    .rate_limit =     migration_rate_limit,
-    .get_rate_limit = migration_get_rate_limit,
-    .set_rate_limit = migration_set_rate_limit,
 };
 
 void migrate_fd_connect(MigrationState *s)
 {
     s->state = MIG_STATE_ACTIVE;
-    s->bytes_xfer = 0;
     s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s);
     s->file = qemu_fopen_ops(s, &migration_file_ops);
 
diff --git a/savevm.c b/savevm.c
index f704f46..6683562 100644
--- a/savevm.c
+++ b/savevm.c
@@ -119,6 +119,9 @@  struct QEMUFile {
     void *opaque;
     int is_write;
 
+    int64_t bytes_xfer;
+    int64_t xfer_limit;
+
     int64_t pos; /* start of buffer when writing, end of buffer
                     when reading */
     int buf_index;
@@ -479,7 +482,6 @@  QEMUFile *qemu_fopen_ops(void *opaque, const QEMUFileOps *ops)
     f->opaque = opaque;
     f->ops = ops;
     f->is_write = 0;
-
     return f;
 }
 
@@ -605,6 +607,7 @@  void qemu_put_buffer(QEMUFile *f, const uint8_t *buf, int size)
         memcpy(f->buf + f->buf_index, buf, l);
         f->is_write = 1;
         f->buf_index += l;
+        f->bytes_xfer += l;
         buf += l;
         size -= l;
         if (f->buf_index >= IO_BUF_SIZE) {
@@ -725,28 +728,28 @@  int64_t qemu_ftell(QEMUFile *f)
 
 int qemu_file_rate_limit(QEMUFile *f)
 {
-    if (f->ops->rate_limit)
-        return f->ops->rate_limit(f->opaque);
-
+    if (qemu_file_get_error(f)) {
+        return 1;
+    }
+    if (f->xfer_limit > 0 && f->bytes_xfer > f->xfer_limit) {
+        return 1;
+    }
     return 0;
 }
 
 int64_t qemu_file_get_rate_limit(QEMUFile *f)
 {
-    if (f->ops->get_rate_limit)
-        return f->ops->get_rate_limit(f->opaque);
-
-    return 0;
+    return f->xfer_limit;
 }
 
-int64_t qemu_file_set_rate_limit(QEMUFile *f, int64_t new_rate)
+void qemu_file_set_rate_limit(QEMUFile *f, int64_t limit)
 {
-    /* any failed or completed migration keeps its state to allow probing of
-     * migration data, but has no associated file anymore */
-    if (f && f->ops->set_rate_limit)
-        return f->ops->set_rate_limit(f->opaque, new_rate);
+    f->xfer_limit = limit;
+}
 
-    return 0;
+void qemu_file_reset_rate_limit(QEMUFile *f)
+{
+    f->bytes_xfer = 0;
 }
 
 void qemu_put_be16(QEMUFile *f, unsigned int v)