[Qemu-devel] [PATCH 0/5] live migration bug fix and refine

[Qemu-devel] [PATCH 0/5] live migration bug fix and refine

[Liang Li]

This patch set fixed a bug which will block live migration and another
potential issue when using multi-thread (de)compression.

The last three patches were submitted before, put them here together.

Liang Li (5):
  migration: Fix multi-thread compression bug
  migration: Fix a potential issue
  migration: remove useless code
  qemu-file: Fix qemu_put_compression_data flaw
  migration: refine ram_save_compressed_page

 include/migration/migration.h |  1 +
 migration/migration.c         |  2 +-
 migration/qemu-file.c         | 23 ++++++++++-
 migration/ram.c               | 93 ++++++++++++++++++++++++++++++-------------
 4 files changed, 88 insertions(+), 31 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/5] migration: Fix multi-thread compression bug

[Liang Li]

Recently, a bug related to multiple thread compression feature for
live migration is reported. The destination side will be blocked
during live migration if there are heavy workload in host and
memory intensive workload in guest, this is most likely to happen
when there is one decompression thread.

Some parts of the decompression code are incorrect:
1. The main thread receives data from source side will enter a busy
loop to wait for a free decompression thread.
2. A lock is needed to protect the decomp_param[idx]->start, because
it is checked in the main thread and is updated in the decompression
thread.

Fix these two issues by following the code pattern for compression.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Reported-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Daniel P. Berrange <berrange@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Tested-by: Daniel P. Berrange <berrange@redhat.com>
---
 migration/ram.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 3f05738..7ab6ab5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -263,6 +263,7 @@ typedef struct CompressParam CompressParam;
 
 struct DecompressParam {
     bool start;
+    bool done;
     QemuMutex mutex;
     QemuCond cond;
     void *des;
@@ -287,6 +288,8 @@ static bool quit_comp_thread;
 static bool quit_decomp_thread;
 static DecompressParam *decomp_param;
 static QemuThread *decompress_threads;
+static QemuMutex decomp_done_lock;
+static QemuCond decomp_done_cond;
 
 static int do_compress_ram_page(CompressParam *param);
 
@@ -834,6 +837,7 @@ static inline void start_compression(CompressParam *param)
 
 static inline void start_decompression(DecompressParam *param)
 {
+    param->done = false;
     qemu_mutex_lock(&param->mutex);
     param->start = true;
     qemu_cond_signal(&param->cond);
@@ -2193,19 +2197,24 @@ static void *do_data_decompress(void *opaque)
         qemu_mutex_lock(&param->mutex);
         while (!param->start && !quit_decomp_thread) {
             qemu_cond_wait(&param->cond, &param->mutex);
+        }
+        if (!quit_decomp_thread) {
             pagesize = TARGET_PAGE_SIZE;
-            if (!quit_decomp_thread) {
-                /* uncompress() will return failed in some case, especially
-                 * when the page is dirted when doing the compression, it's
-                 * not a problem because the dirty page will be retransferred
-                 * and uncompress() won't break the data in other pages.
-                 */
-                uncompress((Bytef *)param->des, &pagesize,
-                           (const Bytef *)param->compbuf, param->len);
-            }
-            param->start = false;
+            /* uncompress() will return failed in some case, especially
+             * when the page is dirted when doing the compression, it's
+             * not a problem because the dirty page will be retransferred
+             * and uncompress() won't break the data in other pages.
+             */
+            uncompress((Bytef *)param->des, &pagesize,
+                       (const Bytef *)param->compbuf, param->len);
         }
+        param->start = false;
         qemu_mutex_unlock(&param->mutex);
+
+        qemu_mutex_lock(&decomp_done_lock);
+        param->done = true;
+        qemu_cond_signal(&decomp_done_cond);
+        qemu_mutex_unlock(&decomp_done_lock);
     }
 
     return NULL;
@@ -2219,10 +2228,13 @@ void migrate_decompress_threads_create(void)
     decompress_threads = g_new0(QemuThread, thread_count);
     decomp_param = g_new0(DecompressParam, thread_count);
     quit_decomp_thread = false;
+    qemu_mutex_init(&decomp_done_lock);
+    qemu_cond_init(&decomp_done_cond);
     for (i = 0; i < thread_count; i++) {
         qemu_mutex_init(&decomp_param[i].mutex);
         qemu_cond_init(&decomp_param[i].cond);
         decomp_param[i].compbuf = g_malloc0(compressBound(TARGET_PAGE_SIZE));
+        decomp_param[i].done = true;
         qemu_thread_create(decompress_threads + i, "decompress",
                            do_data_decompress, decomp_param + i,
                            QEMU_THREAD_JOINABLE);
@@ -2258,9 +2270,10 @@ static void decompress_data_with_multi_threads(QEMUFile *f,
     int idx, thread_count;
 
     thread_count = migrate_decompress_threads();
+    qemu_mutex_lock(&decomp_done_lock);
     while (true) {
         for (idx = 0; idx < thread_count; idx++) {
-            if (!decomp_param[idx].start) {
+            if (decomp_param[idx].done) {
                 qemu_get_buffer(f, decomp_param[idx].compbuf, len);
                 decomp_param[idx].des = host;
                 decomp_param[idx].len = len;
@@ -2270,8 +2283,11 @@ static void decompress_data_with_multi_threads(QEMUFile *f,
         }
         if (idx < thread_count) {
             break;
+        } else {
+            qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
         }
     }
+    qemu_mutex_unlock(&decomp_done_lock);
 }
 
 /*
-- 
1.9.1

[Qemu-devel] [PATCH 2/5] migration: Fix a potential issue

[Liang Li]

At the end of live migration and before vm_start() on the destination
side, we should make sure all the decompression tasks are finished, if
this can not be guaranteed, the VM may get the incorrect memory data,
or the updated memory may be overwritten by the decompression thread.
Add the code to fix this potential issue.

Suggested-by: David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 include/migration/migration.h |  1 +
 migration/migration.c         |  2 +-
 migration/ram.c               | 20 ++++++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/include/migration/migration.h b/include/migration/migration.h
index ac2c12c..1c9051e 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -223,6 +223,7 @@ void migrate_compress_threads_create(void);
 void migrate_compress_threads_join(void);
 void migrate_decompress_threads_create(void);
 void migrate_decompress_threads_join(void);
+void wait_for_decompress_done(void);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_transferred(void);
 uint64_t ram_bytes_total(void);
diff --git a/migration/migration.c b/migration/migration.c
index 991313a..5228c28 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -347,7 +347,7 @@ static void process_incoming_migration_bh(void *opaque)
     /* If global state section was not received or we are in running
        state, we need to obey autostart. Any other state is set with
        runstate_set. */
-
+    wait_for_decompress_done();
     if (!global_state_received() ||
         global_state_get_runstate() == RUN_STATE_RUNNING) {
         if (autostart) {
diff --git a/migration/ram.c b/migration/ram.c
index 7ab6ab5..4459b38 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2220,6 +2220,26 @@ static void *do_data_decompress(void *opaque)
     return NULL;
 }
 
+void wait_for_decompress_done(void)
+{
+    int idx, thread_count;
+
+    if (!migrate_use_compression()) {
+        return;
+    }
+    thread_count = migrate_decompress_threads();
+    for (idx = 0; idx < thread_count; idx++) {
+        if (!decomp_param[idx].done) {
+            qemu_mutex_lock(&decomp_done_lock);
+            while (!decomp_param[idx].done) {
+                qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
+            }
+            qemu_mutex_unlock(&decomp_done_lock);
+        }
+    }
+
+}
+
 void migrate_decompress_threads_create(void)
 {
     int i, thread_count;
-- 
1.9.1

Re: [Qemu-devel] [PATCH 2/5] migration: Fix a potential issue

[Dr. David Alan Gilbert]

* Liang Li (liang.z.li@intel.com) wrote:
> At the end of live migration and before vm_start() on the destination
> side, we should make sure all the decompression tasks are finished, if
> this can not be guaranteed, the VM may get the incorrect memory data,
> or the updated memory may be overwritten by the decompression thread.
> Add the code to fix this potential issue.
> 
> Suggested-by: David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  include/migration/migration.h |  1 +
>  migration/migration.c         |  2 +-
>  migration/ram.c               | 20 ++++++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ac2c12c..1c9051e 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -223,6 +223,7 @@ void migrate_compress_threads_create(void);
>  void migrate_compress_threads_join(void);
>  void migrate_decompress_threads_create(void);
>  void migrate_decompress_threads_join(void);
> +void wait_for_decompress_done(void);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_transferred(void);
>  uint64_t ram_bytes_total(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 991313a..5228c28 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -347,7 +347,7 @@ static void process_incoming_migration_bh(void *opaque)
>      /* If global state section was not received or we are in running
>         state, we need to obey autostart. Any other state is set with
>         runstate_set. */
> -
> +    wait_for_decompress_done();

I wonder if that's early enough; the roder here is that we get:

   ram_load
   devices load
   wait_for_decompress_done()
   start VM

Loading the devices can access guest RAM though (especially virtio); so I
think you need:

   ram_load
   wait_for_decompress_done()
   devices load
   start VM

I think you could do that by placing the wait_for_decompress_done()
at the end of ram_load().

Dave
   
>      if (!global_state_received() ||
>          global_state_get_runstate() == RUN_STATE_RUNNING) {
>          if (autostart) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 7ab6ab5..4459b38 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2220,6 +2220,26 @@ static void *do_data_decompress(void *opaque)
>      return NULL;
>  }
>  
> +void wait_for_decompress_done(void)
> +{
> +    int idx, thread_count;
> +
> +    if (!migrate_use_compression()) {
> +        return;
> +    }
> +    thread_count = migrate_decompress_threads();
> +    for (idx = 0; idx < thread_count; idx++) {
> +        if (!decomp_param[idx].done) {
> +            qemu_mutex_lock(&decomp_done_lock);
> +            while (!decomp_param[idx].done) {
> +                qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> +            }
> +            qemu_mutex_unlock(&decomp_done_lock);
> +        }
> +    }
> +
> +}
> +
>  void migrate_decompress_threads_create(void)
>  {
>      int i, thread_count;
> -- 
> 1.9.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/5] migration: Fix a potential issue

[Li, Liang Z]

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> Sent: Wednesday, May 04, 2016 4:48 PM
> To: Li, Liang Z
> Cc: qemu-devel@nongnu.org; quintela@redhat.com;
> amit.shah@redhat.com; berrange@redhat.com
> Subject: Re: [PATCH 2/5] migration: Fix a potential issue
> 
> * Liang Li (liang.z.li@intel.com) wrote:
> > At the end of live migration and before vm_start() on the destination
> > side, we should make sure all the decompression tasks are finished, if
> > this can not be guaranteed, the VM may get the incorrect memory data,
> > or the updated memory may be overwritten by the decompression thread.
> > Add the code to fix this potential issue.
> >
> > Suggested-by: David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > ---
> >  include/migration/migration.h |  1 +
> >  migration/migration.c         |  2 +-
> >  migration/ram.c               | 20 ++++++++++++++++++++
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/migration/migration.h
> > b/include/migration/migration.h index ac2c12c..1c9051e 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -223,6 +223,7 @@ void migrate_compress_threads_create(void);
> >  void migrate_compress_threads_join(void);
> >  void migrate_decompress_threads_create(void);
> >  void migrate_decompress_threads_join(void);
> > +void wait_for_decompress_done(void);
> >  uint64_t ram_bytes_remaining(void);
> >  uint64_t ram_bytes_transferred(void);  uint64_t
> > ram_bytes_total(void); diff --git a/migration/migration.c
> > b/migration/migration.c index 991313a..5228c28 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -347,7 +347,7 @@ static void process_incoming_migration_bh(void
> *opaque)
> >      /* If global state section was not received or we are in running
> >         state, we need to obey autostart. Any other state is set with
> >         runstate_set. */
> > -
> > +    wait_for_decompress_done();
> 
> I wonder if that's early enough; the roder here is that we get:
> 
>    ram_load
>    devices load
>    wait_for_decompress_done()
>    start VM
> 
> Loading the devices can access guest RAM though (especially virtio); so I
> think you need:
> 
>    ram_load
>    wait_for_decompress_done()
>    devices load
>    start VM
> 
> I think you could do that by placing the wait_for_decompress_done() at the
> end of ram_load().
> 
> Dave

You are right, will change. Thanks!

Liang
> 
> >      if (!global_state_received() ||
> >          global_state_get_runstate() == RUN_STATE_RUNNING) {
> >          if (autostart) {
> > diff --git a/migration/ram.c b/migration/ram.c index 7ab6ab5..4459b38
> > 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2220,6 +2220,26 @@ static void *do_data_decompress(void *opaque)
> >      return NULL;
> >  }
> >
> > +void wait_for_decompress_done(void)
> > +{
> > +    int idx, thread_count;
> > +
> > +    if (!migrate_use_compression()) {
> > +        return;
> > +    }
> > +    thread_count = migrate_decompress_threads();
> > +    for (idx = 0; idx < thread_count; idx++) {
> > +        if (!decomp_param[idx].done) {
> > +            qemu_mutex_lock(&decomp_done_lock);
> > +            while (!decomp_param[idx].done) {
> > +                qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> > +            }
> > +            qemu_mutex_unlock(&decomp_done_lock);
> > +        }
> > +    }
> > +
> > +}
> > +
> >  void migrate_decompress_threads_create(void)
> >  {
> >      int i, thread_count;
> > --
> > 1.9.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/5] migration: Fix a potential issue

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> At the end of live migration and before vm_start() on the destination
> side, we should make sure all the decompression tasks are finished, if
> this can not be guaranteed, the VM may get the incorrect memory data,
> or the updated memory may be overwritten by the decompression thread.
> Add the code to fix this potential issue.
>
> Suggested-by: David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  include/migration/migration.h |  1 +
>  migration/migration.c         |  2 +-
>  migration/ram.c               | 20 ++++++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index ac2c12c..1c9051e 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -223,6 +223,7 @@ void migrate_compress_threads_create(void);
>  void migrate_compress_threads_join(void);
>  void migrate_decompress_threads_create(void);
>  void migrate_decompress_threads_join(void);
> +void wait_for_decompress_done(void);
>  uint64_t ram_bytes_remaining(void);
>  uint64_t ram_bytes_transferred(void);
>  uint64_t ram_bytes_total(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 991313a..5228c28 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -347,7 +347,7 @@ static void process_incoming_migration_bh(void *opaque)
>      /* If global state section was not received or we are in running
>         state, we need to obey autostart. Any other state is set with
>         runstate_set. */
> -
> +    wait_for_decompress_done();
>      if (!global_state_received() ||
>          global_state_get_runstate() == RUN_STATE_RUNNING) {
>          if (autostart) {
> diff --git a/migration/ram.c b/migration/ram.c
> index 7ab6ab5..4459b38 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -2220,6 +2220,26 @@ static void *do_data_decompress(void *opaque)
>      return NULL;
>  }
>  

why?

> +void wait_for_decompress_done(void)
> +{
> +    int idx, thread_count;
> +
> +    if (!migrate_use_compression()) {
> +        return;
> +    }
> +    thread_count = migrate_decompress_threads();
> +    for (idx = 0; idx < thread_count; idx++) {
> +        if (!decomp_param[idx].done) {
> +            qemu_mutex_lock(&decomp_done_lock);
> +            while (!decomp_param[idx].done) {
> +                qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> +            }
> +            qemu_mutex_unlock(&decomp_done_lock);
> +        }
> +    }
> +
> +}
> +

    thread_count = migrate_decompress_threads();
    qemu_mutex_lock(&decomp_done_lock);
    for (idx = 0; idx < thread_count; idx++) {
       while (!decomp_param[idx].done) {
           qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
       }
    }
    qemu_mutex_unlock(&decomp_done_lock);

Simpler and correct, no?

Later, Juan.

Re: [Qemu-devel] [PATCH 2/5] migration: Fix a potential issue

[Li, Liang Z]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+liang.z.li=intel.com@nongnu.org] On Behalf Of Juan Quintela
> Sent: Wednesday, May 04, 2016 5:18 PM
> To: Li, Liang Z
> Cc: amit.shah@redhat.com; qemu-devel@nongnu.org; dgilbert@redhat.com
> Subject: Re: [Qemu-devel] [PATCH 2/5] migration: Fix a potential issue
> 
> Liang Li <liang.z.li@intel.com> wrote:
> > At the end of live migration and before vm_start() on the destination
> > side, we should make sure all the decompression tasks are finished, if
> > this can not be guaranteed, the VM may get the incorrect memory data,
> > or the updated memory may be overwritten by the decompression thread.
> > Add the code to fix this potential issue.
> >
> > Suggested-by: David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > ---
> >  include/migration/migration.h |  1 +
> >  migration/migration.c         |  2 +-
> >  migration/ram.c               | 20 ++++++++++++++++++++
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/migration/migration.h
> > b/include/migration/migration.h index ac2c12c..1c9051e 100644
> > --- a/include/migration/migration.h
> > +++ b/include/migration/migration.h
> > @@ -223,6 +223,7 @@ void migrate_compress_threads_create(void);
> >  void migrate_compress_threads_join(void);
> >  void migrate_decompress_threads_create(void);
> >  void migrate_decompress_threads_join(void);
> > +void wait_for_decompress_done(void);
> >  uint64_t ram_bytes_remaining(void);
> >  uint64_t ram_bytes_transferred(void);  uint64_t
> > ram_bytes_total(void); diff --git a/migration/migration.c
> > b/migration/migration.c index 991313a..5228c28 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -347,7 +347,7 @@ static void process_incoming_migration_bh(void
> *opaque)
> >      /* If global state section was not received or we are in running
> >         state, we need to obey autostart. Any other state is set with
> >         runstate_set. */
> > -
> > +    wait_for_decompress_done();
> >      if (!global_state_received() ||
> >          global_state_get_runstate() == RUN_STATE_RUNNING) {
> >          if (autostart) {
> > diff --git a/migration/ram.c b/migration/ram.c index 7ab6ab5..4459b38
> > 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -2220,6 +2220,26 @@ static void *do_data_decompress(void *opaque)
> >      return NULL;
> >  }
> >
> 
> why?
> 
> > +void wait_for_decompress_done(void)
> > +{
> > +    int idx, thread_count;
> > +
> > +    if (!migrate_use_compression()) {
> > +        return;
> > +    }
> > +    thread_count = migrate_decompress_threads();
> > +    for (idx = 0; idx < thread_count; idx++) {
> > +        if (!decomp_param[idx].done) {
> > +            qemu_mutex_lock(&decomp_done_lock);
> > +            while (!decomp_param[idx].done) {
> > +                qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
> > +            }
> > +            qemu_mutex_unlock(&decomp_done_lock);
> > +        }
> > +    }
> > +
> > +}
> > +
> 
>     thread_count = migrate_decompress_threads();
>     qemu_mutex_lock(&decomp_done_lock);
>     for (idx = 0; idx < thread_count; idx++) {
>        while (!decomp_param[idx].done) {
>            qemu_cond_wait(&decomp_done_cond, &decomp_done_lock);
>        }
>     }
>     qemu_mutex_unlock(&decomp_done_lock);
> 
> Simpler and correct, no?
> 
> Later, Juan.


Yes, thanks! 
Again, I think I should clean up the multiple thread compression code.

Liang

[Qemu-devel] [PATCH 3/5] migration: remove useless code

[Liang Li]

page_buffer is set twice repeatedly, remove the previous set.

Signed-off-by: Liang Li <liang.z.li@intel.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 migration/ram.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/ram.c b/migration/ram.c
index 4459b38..bc34bc5 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2358,7 +2358,6 @@ static int ram_load_postcopy(QEMUFile *f)
                 ret = -EINVAL;
                 break;
             }
-            page_buffer = host;
             /*
              * Postcopy requires that we place whole host pages atomically.
              * To make it atomic, the data is read into a temporary page
-- 
1.9.1

Re: [Qemu-devel] [PATCH 3/5] migration: remove useless code

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> page_buffer is set twice repeatedly, remove the previous set.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

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

[Qemu-devel] [PATCH 4/5] qemu-file: Fix qemu_put_compression_data flaw

[Liang Li]

Current qemu_put_compression_data can only work with no writable
QEMUFile, and can't work with the writable QEMUFile. But it does
not provide any measure to prevent users from using it with a
writable QEMUFile.

We should fix this flaw to make it works with writable QEMUFile.

Suggested-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/qemu-file.c | 23 +++++++++++++++++++++--
 migration/ram.c       |  6 +++++-
 2 files changed, 26 insertions(+), 3 deletions(-)

diff --git a/migration/qemu-file.c b/migration/qemu-file.c
index 6f4a129..b0ef1f3 100644
--- a/migration/qemu-file.c
+++ b/migration/qemu-file.c
@@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
     return v;
 }
 
-/* compress size bytes of data start at p with specific compression
+/* Compress size bytes of data start at p with specific compression
  * level and store the compressed data to the buffer of f.
+ *
+ * When f is not writable, return -1 if f has no space to save the
+ * compressed data.
+ * When f is wirtable and it has no space to save the compressed data,
+ * do fflush first, if f still has no space to save the compressed
+ * data, return -1.
  */
 
 ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
@@ -617,7 +623,14 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
     ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
 
     if (blen < compressBound(size)) {
-        return 0;
+        if (!qemu_file_is_writable(f)) {
+            return -1;
+        }
+        qemu_fflush(f);
+        blen = IO_BUF_SIZE - sizeof(int32_t);
+        if (blen < compressBound(size)) {
+            return -1;
+        }
     }
     if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
                   (Bytef *)p, size, level) != Z_OK) {
@@ -625,7 +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
         return 0;
     }
     qemu_put_be32(f, blen);
+    if (f->ops->writev_buffer) {
+        add_to_iovec(f, f->buf + f->buf_index, blen);
+    }
     f->buf_index += blen;
+    if (f->buf_index == IO_BUF_SIZE) {
+        qemu_fflush(f);
+    }
     return blen + sizeof(int32_t);
 }
 
diff --git a/migration/ram.c b/migration/ram.c
index bc34bc5..7e62d8d 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -821,7 +821,11 @@ static int do_compress_ram_page(CompressParam *param)
                                   RAM_SAVE_FLAG_COMPRESS_PAGE);
     blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
                                      migrate_compress_level());
-    bytes_sent += blen;
+    if (blen < 0) {
+        error_report("Insufficient buffer for compressed data!");
+    } else {
+        bytes_sent += blen;
+    }
 
     return bytes_sent;
 }
-- 
1.9.1

Re: [Qemu-devel] [PATCH 4/5] qemu-file: Fix qemu_put_compression_data flaw

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> Current qemu_put_compression_data can only work with no writable
> QEMUFile, and can't work with the writable QEMUFile. But it does
> not provide any measure to prevent users from using it with a
> writable QEMUFile.
>
> We should fix this flaw to make it works with writable QEMUFile.
>
> Suggested-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>

Code is not enough.

> ---
>  migration/qemu-file.c | 23 +++++++++++++++++++++--
>  migration/ram.c       |  6 +++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index 6f4a129..b0ef1f3 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
>      return v;
>  }
>  
> -/* compress size bytes of data start at p with specific compression
> +/* Compress size bytes of data start at p with specific compression
>   * level and store the compressed data to the buffer of f.
> + *
> + * When f is not writable, return -1 if f has no space to save the
> + * compressed data.
> + * When f is wirtable and it has no space to save the compressed data,
> + * do fflush first, if f still has no space to save the compressed
> + * data, return -1.
>   */
>  
>  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
> @@ -617,7 +623,14 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
>  
>      if (blen < compressBound(size)) {
> -        return 0;
> +        if (!qemu_file_is_writable(f)) {
> +            return -1;
> +        }

I can't yet understand why we can be writting to a qemu_file that is not writable.
But well, no harm.


> +        qemu_fflush(f);
> +        blen = IO_BUF_SIZE - sizeof(int32_t);
> +        if (blen < compressBound(size)) {
> +            return -1;
> +        }
>      }
>      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
>                    (Bytef *)p, size, level) != Z_OK) {
> @@ -625,7 +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
>          return 0;
>      }
>      qemu_put_be32(f, blen);
> +    if (f->ops->writev_buffer) {
> +        add_to_iovec(f, f->buf + f->buf_index, blen);
> +    }



> +    }
>      return blen + sizeof(int32_t);
>  }
>  

Ok with the changes in this function.


> diff --git a/migration/ram.c b/migration/ram.c
> index bc34bc5..7e62d8d 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -821,7 +821,11 @@ static int do_compress_ram_page(CompressParam *param)
>                                    RAM_SAVE_FLAG_COMPRESS_PAGE);
>      blen = qemu_put_compression_data(param->file, p, TARGET_PAGE_SIZE,
>                                       migrate_compress_level());
> -    bytes_sent += blen;
> +    if (blen < 0) {
> +        error_report("Insufficient buffer for compressed data!");
> +    } else {
> +        bytes_sent += blen;
> +    }
>  
>      return bytes_sent;
>  }


Are you sure that this code is ok?

1st- there are two callers od do_compress_ram_page(), only one of it
     uses the return value
2nd- if we were not able to write the compressed page, we continue
     without a single error.  I think that if blen is < 0, we should
     just abort the migration at this point (we have just sent a header
     and we haven't been able to send the contents under that header.
     Without lot of changes, I can't see how to fix it).

Thanks, Juan.

Re: [Qemu-devel] [PATCH 4/5] qemu-file: Fix qemu_put_compression_data flaw

[Li, Liang Z]

> Liang Li <liang.z.li@intel.com> wrote:
> > Current qemu_put_compression_data can only work with no writable
> > QEMUFile, and can't work with the writable QEMUFile. But it does not
> > provide any measure to prevent users from using it with a writable
> > QEMUFile.
> >
> > We should fix this flaw to make it works with writable QEMUFile.
> >
> > Suggested-by: Juan Quintela <quintela@redhat.com>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> 
> Code is not enough.
> 
> > ---
> >  migration/qemu-file.c | 23 +++++++++++++++++++++--
> >  migration/ram.c       |  6 +++++-
> >  2 files changed, 26 insertions(+), 3 deletions(-)
> >
> > diff --git a/migration/qemu-file.c b/migration/qemu-file.c index
> > 6f4a129..b0ef1f3 100644
> > --- a/migration/qemu-file.c
> > +++ b/migration/qemu-file.c
> > @@ -607,8 +607,14 @@ uint64_t qemu_get_be64(QEMUFile *f)
> >      return v;
> >  }
> >
> > -/* compress size bytes of data start at p with specific compression
> > +/* Compress size bytes of data start at p with specific compression
> >   * level and store the compressed data to the buffer of f.
> > + *
> > + * When f is not writable, return -1 if f has no space to save the
> > + * compressed data.
> > + * When f is wirtable and it has no space to save the compressed
> > + data,
> > + * do fflush first, if f still has no space to save the compressed
> > + * data, return -1.
> >   */
> >
> >  ssize_t qemu_put_compression_data(QEMUFile *f, const uint8_t *p,
> > size_t size, @@ -617,7 +623,14 @@ ssize_t
> qemu_put_compression_data(QEMUFile *f, const uint8_t *p, size_t size,
> >      ssize_t blen = IO_BUF_SIZE - f->buf_index - sizeof(int32_t);
> >
> >      if (blen < compressBound(size)) {
> > -        return 0;
> > +        if (!qemu_file_is_writable(f)) {
> > +            return -1;
> > +        }
> 
> I can't yet understand why we can be writting to a qemu_file that is not
> writable.
> But well, no harm.
> 
> 
> > +        qemu_fflush(f);
> > +        blen = IO_BUF_SIZE - sizeof(int32_t);
> > +        if (blen < compressBound(size)) {
> > +            return -1;
> > +        }
> >      }
> >      if (compress2(f->buf + f->buf_index + sizeof(int32_t), (uLongf *)&blen,
> >                    (Bytef *)p, size, level) != Z_OK) { @@ -625,7
> > +638,13 @@ ssize_t qemu_put_compression_data(QEMUFile *f, const
> uint8_t *p, size_t size,
> >          return 0;
> >      }
> >      qemu_put_be32(f, blen);
> > +    if (f->ops->writev_buffer) {
> > +        add_to_iovec(f, f->buf + f->buf_index, blen);
> > +    }
> 
> 
> 
> > +    }
> >      return blen + sizeof(int32_t);
> >  }
> >
> 
> Ok with the changes in this function.
> 
> 
> > diff --git a/migration/ram.c b/migration/ram.c index bc34bc5..7e62d8d
> > 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -821,7 +821,11 @@ static int
> do_compress_ram_page(CompressParam *param)
> >                                    RAM_SAVE_FLAG_COMPRESS_PAGE);
> >      blen = qemu_put_compression_data(param->file, p,
> TARGET_PAGE_SIZE,
> >                                       migrate_compress_level());
> > -    bytes_sent += blen;
> > +    if (blen < 0) {
> > +        error_report("Insufficient buffer for compressed data!");
> > +    } else {
> > +        bytes_sent += blen;
> > +    }
> >
> >      return bytes_sent;
> >  }
> 
> 
> Are you sure that this code is ok?

No.
> 
> 1st- there are two callers od do_compress_ram_page(), only one of it
>      uses the return value
> 2nd- if we were not able to write the compressed page, we continue
>      without a single error.  I think that if blen is < 0, we should
>      just abort the migration at this point (we have just sent a header
>      and we haven't been able to send the contents under that header.
>      Without lot of changes, I can't see how to fix it).
> 

Abort the migration is the right choice,  use qemu_file_set_error() to set the error flag is enough? 
Thanks

Liang

> Thanks, Juan.

[Qemu-devel] [PATCH 5/5] migration: refine ram_save_compressed_page

[Liang Li]

Use qemu_put_compression_data to do the compression directly
instead of using do_compress_ram_page, avoid some data copy.
very small improvement, at the same time, add code to check
if the compression is successful.

Signed-off-by: Liang Li <liang.z.li@intel.com>
---
 migration/ram.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 7e62d8d..ec2c0bf 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -927,24 +927,20 @@ static int ram_save_compressed_page(QEMUFile *f, PageSearchStatus *pss,
                                     uint64_t *bytes_transferred)
 {
     int pages = -1;
-    uint64_t bytes_xmit;
+    uint64_t bytes_xmit = 0;
     uint8_t *p;
-    int ret;
+    int ret, blen;
     RAMBlock *block = pss->block;
     ram_addr_t offset = pss->offset;
 
     p = block->host + offset;
 
-    bytes_xmit = 0;
     ret = ram_control_save_page(f, block->offset,
                                 offset, TARGET_PAGE_SIZE, &bytes_xmit);
     if (bytes_xmit) {
         *bytes_transferred += bytes_xmit;
         pages = 1;
     }
-    if (block == last_sent_block) {
-        offset |= RAM_SAVE_FLAG_CONTINUE;
-    }
     if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
         if (ret != RAM_SAVE_CONTROL_DELAYED) {
             if (bytes_xmit > 0) {
@@ -964,17 +960,19 @@ static int ram_save_compressed_page(QEMUFile *f, PageSearchStatus *pss,
             flush_compressed_data(f);
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
-                set_compress_params(&comp_param[0], block, offset);
-                /* Use the qemu thread to compress the data to make sure the
-                 * first page is sent out before other pages
-                 */
-                bytes_xmit = do_compress_ram_page(&comp_param[0]);
-                acct_info.norm_pages++;
-                qemu_put_qemu_file(f, comp_param[0].file);
-                *bytes_transferred += bytes_xmit;
-                pages = 1;
+                /* Make sure the first page is sent out before other pages */
+                bytes_xmit = save_page_header(f, block, offset |
+                                              RAM_SAVE_FLAG_COMPRESS_PAGE);
+                blen = qemu_put_compression_data(f, p, TARGET_PAGE_SIZE,
+                                                 migrate_compress_level());
+                if (blen > 0) {
+                    *bytes_transferred += bytes_xmit + blen;
+                    acct_info.norm_pages++;
+                    pages = 1;
+                }
             }
         } else {
+            offset |= RAM_SAVE_FLAG_CONTINUE;
             pages = save_zero_page(f, block, offset, p, bytes_transferred);
             if (pages == -1) {
                 pages = compress_page_with_multi_thread(f, block, offset,
-- 
1.9.1

Re: [Qemu-devel] [PATCH 5/5] migration: refine ram_save_compressed_page

[Juan Quintela]

Liang Li <liang.z.li@intel.com> wrote:
> Use qemu_put_compression_data to do the compression directly
> instead of using do_compress_ram_page, avoid some data copy.
> very small improvement, at the same time, add code to check
> if the compression is successful.
>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
>  migration/ram.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 7e62d8d..ec2c0bf 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -927,24 +927,20 @@ static int ram_save_compressed_page(QEMUFile *f, PageSearchStatus *pss,
>                                      uint64_t *bytes_transferred)
>  {
>      int pages = -1;
> -    uint64_t bytes_xmit;
> +    uint64_t bytes_xmit = 0;
>      uint8_t *p;
> -    int ret;
> +    int ret, blen;
>      RAMBlock *block = pss->block;
>      ram_addr_t offset = pss->offset;
>  
>      p = block->host + offset;
>  
> -    bytes_xmit = 0;
>      ret = ram_control_save_page(f, block->offset,
>                                  offset, TARGET_PAGE_SIZE, &bytes_xmit);
>      if (bytes_xmit) {
>          *bytes_transferred += bytes_xmit;
>          pages = 1;
>      }
> -    if (block == last_sent_block) {
> -        offset |= RAM_SAVE_FLAG_CONTINUE;
> -    }
>      if (ret != RAM_SAVE_CONTROL_NOT_SUPP) {
>          if (ret != RAM_SAVE_CONTROL_DELAYED) {
>              if (bytes_xmit > 0) {
> @@ -964,17 +960,19 @@ static int ram_save_compressed_page(QEMUFile *f, PageSearchStatus *pss,
>              flush_compressed_data(f);
>              pages = save_zero_page(f, block, offset, p, bytes_transferred);
>              if (pages == -1) {
> -                set_compress_params(&comp_param[0], block, offset);
> -                /* Use the qemu thread to compress the data to make sure the
> -                 * first page is sent out before other pages
> -                 */
> -                bytes_xmit = do_compress_ram_page(&comp_param[0]);
> -                acct_info.norm_pages++;
> -                qemu_put_qemu_file(f, comp_param[0].file);
> -                *bytes_transferred += bytes_xmit;
> -                pages = 1;
> +                /* Make sure the first page is sent out before other pages */
> +                bytes_xmit = save_page_header(f, block, offset |
> +                                              RAM_SAVE_FLAG_COMPRESS_PAGE);
> +                blen = qemu_put_compression_data(f, p, TARGET_PAGE_SIZE,
> +                                                 migrate_compress_level());
> +                if (blen > 0) {
> +                    *bytes_transferred += bytes_xmit + blen;
> +                    acct_info.norm_pages++;
> +                    pages = 1;
> +                }
>              }
>          } else {
> +            offset |= RAM_SAVE_FLAG_CONTINUE;
>              pages = save_zero_page(f, block, offset, p, bytes_transferred);
>              if (pages == -1) {
>                  pages = compress_page_with_multi_thread(f, block, offset,

You need to decide what to do when blen <= 0.  As said in previous
patch, I can only think of aborting migration.  But ignoring the error
is not an option IMHO.

Later, Juan.