[Qemu-devel] [PATCH v2 0/2] Stop reinit of XBZRLE.lock

[Qemu-devel] [PATCH v2 0/2] Stop reinit of XBZRLE.lock

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Markus Armbruster spotted that the XBZRLE.lock might get initalised
multiple times in the case of a second attempted migration, and
that's undefined behaviour for pthread_mutex_init.

This patchset is based on Markus' proto-patch in the discussions
off v1.

--
v2
   Rework completely based on Markus's patch

Dr. David Alan Gilbert (2):
  Provide init function for ram migration
  Init the XBZRLE.lock in ram_mig_init

 arch_init.c                   | 68 +++++++++++++++++++++++--------------------
 include/migration/migration.h |  2 --
 include/sysemu/arch_init.h    |  1 +
 vl.c                          |  3 +-
 4 files changed, 39 insertions(+), 35 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH v2 1/2] Provide init function for ram migration

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Provide ram_mig_init (like blk_mig_init) for vl.c to initialise stuff
to do with ram migration (currently in arch_init.c).

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 arch_init.c                   | 7 ++++++-
 include/migration/migration.h | 2 --
 include/sysemu/arch_init.h    | 1 +
 vl.c                          | 3 +--
 4 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 60c975d..0d2bf84 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -1095,7 +1095,7 @@ done:
     return ret;
 }
 
-SaveVMHandlers savevm_ram_handlers = {
+static SaveVMHandlers savevm_ram_handlers = {
     .save_live_setup = ram_save_setup,
     .save_live_iterate = ram_save_iterate,
     .save_live_complete = ram_save_complete,
@@ -1104,6 +1104,11 @@ SaveVMHandlers savevm_ram_handlers = {
     .cancel = ram_migration_cancel,
 };
 
+void ram_mig_init(void)
+{
+    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
+}
+
 struct soundhw {
     const char *name;
     const char *descr;
diff --git a/include/migration/migration.h b/include/migration/migration.h
index 3e1e6c7..31fbf17 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -113,8 +113,6 @@ void free_xbzrle_decoded_buf(void);
 
 void acct_update_position(QEMUFile *f, size_t size, bool zero);
 
-extern SaveVMHandlers savevm_ram_handlers;
-
 uint64_t dup_mig_bytes_transferred(void);
 uint64_t dup_mig_pages_transferred(void);
 uint64_t skipped_mig_bytes_transferred(void);
diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
index be71bca..182d48d 100644
--- a/include/sysemu/arch_init.h
+++ b/include/sysemu/arch_init.h
@@ -29,6 +29,7 @@ extern const uint32_t arch_type;
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const QemuOpts *opts);
 void do_smbios_option(QemuOpts *opts);
+void ram_mig_init(void);
 void cpudef_init(void);
 void audio_init(void);
 int tcg_available(void);
diff --git a/vl.c b/vl.c
index f0fe48b..3358c98 100644
--- a/vl.c
+++ b/vl.c
@@ -4274,6 +4274,7 @@ int main(int argc, char **argv, char **envp)
     cpu_exec_init_all();
 
     blk_mig_init();
+    ram_mig_init();
 
     /* open the virtual block devices */
     if (snapshot)
@@ -4288,8 +4289,6 @@ int main(int argc, char **argv, char **envp)
     default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
     default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
 
-    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
-
     if (nb_numa_nodes > 0) {
         int i;
 
-- 
1.8.5.3

[Qemu-devel] [PATCH v2 2/2] Init the XBZRLE.lock in ram_mig_init

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Initialising the XBZRLE.lock earlier simplifies the lock use.

Based on Markus's patch in:
http://lists.gnu.org/archive/html/qemu-devel/2014-03/msg03879.html

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 arch_init.c | 61 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 0d2bf84..f18f42e 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -45,6 +45,7 @@
 #include "hw/audio/pcspk.h"
 #include "migration/page_cache.h"
 #include "qemu/config-file.h"
+#include "qemu/error-report.h"
 #include "qmp-commands.h"
 #include "trace.h"
 #include "exec/cpu-all.h"
@@ -167,11 +168,8 @@ static struct {
     /* Cache for XBZRLE, Protected by lock. */
     PageCache *cache;
     QemuMutex lock;
-} XBZRLE = {
-    .encoded_buf = NULL,
-    .current_buf = NULL,
-    .cache = NULL,
-};
+} XBZRLE;
+
 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;
 
@@ -187,41 +185,44 @@ static void XBZRLE_cache_unlock(void)
         qemu_mutex_unlock(&XBZRLE.lock);
 }
 
+/*
+ * called from qmp_migrate_set_cache_size in main thread, possibly while
+ * a migration is in progress.
+ * A running migration maybe using the cache and might finish during this
+ * call, hence changes to the cache are protected by XBZRLE.lock().
+ */
 int64_t xbzrle_cache_resize(int64_t new_size)
 {
-    PageCache *new_cache, *cache_to_free;
+    PageCache *new_cache;
+    int64_t ret;
 
     if (new_size < TARGET_PAGE_SIZE) {
         return -1;
     }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    XBZRLE_cache_lock();
+
     if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
-            return pow2floor(new_size);
+            goto out_new_size;
         }
         new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
                                         TARGET_PAGE_SIZE);
         if (!new_cache) {
-            DPRINTF("Error creating cache\n");
-            return -1;
-        }
-
-        XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
-        if (XBZRLE.cache != NULL) {
-            cache_to_free = XBZRLE.cache;
-            XBZRLE.cache = new_cache;
-        } else {
-            cache_to_free = new_cache;
+            error_report("Error creating cache");
+            ret = -1;
+            goto out;
         }
-        XBZRLE_cache_unlock();
 
-        cache_fini(cache_to_free);
+        cache_fini(XBZRLE.cache);
+        XBZRLE.cache = new_cache;
     }
 
-    return pow2floor(new_size);
+out_new_size:
+    ret = pow2floor(new_size);
+out:
+    XBZRLE_cache_unlock();
+    return ret;
 }
 
 /* accounting for migration statistics */
@@ -735,28 +736,27 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
     dirty_rate_high_cnt = 0;
 
     if (migrate_use_xbzrle()) {
-        qemu_mutex_lock_iothread();
+        XBZRLE_cache_lock();
         XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
                                   TARGET_PAGE_SIZE,
                                   TARGET_PAGE_SIZE);
         if (!XBZRLE.cache) {
-            qemu_mutex_unlock_iothread();
-            DPRINTF("Error creating cache\n");
+            XBZRLE_cache_unlock();
+            error_report("Error creating cache");
             return -1;
         }
-        qemu_mutex_init(&XBZRLE.lock);
-        qemu_mutex_unlock_iothread();
+        XBZRLE_cache_unlock();
 
         /* We prefer not to abort if there is no memory */
         XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
         if (!XBZRLE.encoded_buf) {
-            DPRINTF("Error allocating encoded_buf\n");
+            error_report("Error allocating encoded_buf");
             return -1;
         }
 
         XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
         if (!XBZRLE.current_buf) {
-            DPRINTF("Error allocating current_buf\n");
+            error_report("Error allocating current_buf");
             g_free(XBZRLE.encoded_buf);
             XBZRLE.encoded_buf = NULL;
             return -1;
@@ -1106,6 +1106,7 @@ static SaveVMHandlers savevm_ram_handlers = {
 
 void ram_mig_init(void)
 {
+    qemu_mutex_init(&XBZRLE.lock);
     register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
 }
 
-- 
1.8.5.3

Re: [Qemu-devel] [PATCH v2 0/2] Stop reinit of XBZRLE.lock

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Markus Armbruster spotted that the XBZRLE.lock might get initalised
> multiple times in the case of a second attempted migration, and
> that's undefined behaviour for pthread_mutex_init.
>
> This patchset is based on Markus' proto-patch in the discussions
> off v1.
>
> --
> v2
>    Rework completely based on Markus's patch
>
> Dr. David Alan Gilbert (2):
>   Provide init function for ram migration
>   Init the XBZRLE.lock in ram_mig_init

Second patch does a fair but more than its subject suggests.  Perhaps
"XBZRLE: Rework locking to fix double initialization" would be better.
Still doesn't cover the upgrade from debugging prints to proper error
reporting.  Maybe that should be a separate patch.  Up to you.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/2] Provide init function for ram migration

[Gonglei (Arei)]

> Subject: [PATCH v2 1/2] Provide init function for ram migration
> 
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Provide ram_mig_init (like blk_mig_init) for vl.c to initialise stuff
> to do with ram migration (currently in arch_init.c).
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  arch_init.c                   | 7 ++++++-
>  include/migration/migration.h | 2 --
>  include/sysemu/arch_init.h    | 1 +
>  vl.c                          | 3 +--
>  4 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 60c975d..0d2bf84 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -1095,7 +1095,7 @@ done:
>      return ret;
>  }
> 
> -SaveVMHandlers savevm_ram_handlers = {
> +static SaveVMHandlers savevm_ram_handlers = {
>      .save_live_setup = ram_save_setup,
>      .save_live_iterate = ram_save_iterate,
>      .save_live_complete = ram_save_complete,
> @@ -1104,6 +1104,11 @@ SaveVMHandlers savevm_ram_handlers = {
>      .cancel = ram_migration_cancel,
>  };
> 
> +void ram_mig_init(void)
> +{
> +    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers,
> NULL);
> +}
> +
>  struct soundhw {
>      const char *name;
>      const char *descr;
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 3e1e6c7..31fbf17 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -113,8 +113,6 @@ void free_xbzrle_decoded_buf(void);
> 
>  void acct_update_position(QEMUFile *f, size_t size, bool zero);
> 
> -extern SaveVMHandlers savevm_ram_handlers;
> -
>  uint64_t dup_mig_bytes_transferred(void);
>  uint64_t dup_mig_pages_transferred(void);
>  uint64_t skipped_mig_bytes_transferred(void);
> diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h
> index be71bca..182d48d 100644
> --- a/include/sysemu/arch_init.h
> +++ b/include/sysemu/arch_init.h
> @@ -29,6 +29,7 @@ extern const uint32_t arch_type;
>  void select_soundhw(const char *optarg);
>  void do_acpitable_option(const QemuOpts *opts);
>  void do_smbios_option(QemuOpts *opts);
> +void ram_mig_init(void);
>  void cpudef_init(void);
>  void audio_init(void);
>  int tcg_available(void);
> diff --git a/vl.c b/vl.c
> index f0fe48b..3358c98 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4274,6 +4274,7 @@ int main(int argc, char **argv, char **envp)
>      cpu_exec_init_all();
> 
>      blk_mig_init();
> +    ram_mig_init();
> 
>      /* open the virtual block devices */
>      if (snapshot)
> @@ -4288,8 +4289,6 @@ int main(int argc, char **argv, char **envp)
>      default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>      default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
> 
> -    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
> -
>      if (nb_numa_nodes > 0) {
>          int i;
> 
> --
> 1.8.5.3

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 2/2] Init the XBZRLE.lock in ram_mig_init

[Gonglei (Arei)]

> Subject: [PATCH v2 2/2] Init the XBZRLE.lock in ram_mig_init
> 
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Initialising the XBZRLE.lock earlier simplifies the lock use.
> 
> Based on Markus's patch in:
> http://lists.gnu.org/archive/html/qemu-devel/2014-03/msg03879.html
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  arch_init.c | 61 +++++++++++++++++++++++++++++++------------------------------
>  1 file changed, 31 insertions(+), 30 deletions(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 0d2bf84..f18f42e 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -45,6 +45,7 @@
>  #include "hw/audio/pcspk.h"
>  #include "migration/page_cache.h"
>  #include "qemu/config-file.h"
> +#include "qemu/error-report.h"
>  #include "qmp-commands.h"
>  #include "trace.h"
>  #include "exec/cpu-all.h"
> @@ -167,11 +168,8 @@ static struct {
>      /* Cache for XBZRLE, Protected by lock. */
>      PageCache *cache;
>      QemuMutex lock;
> -} XBZRLE = {
> -    .encoded_buf = NULL,
> -    .current_buf = NULL,
> -    .cache = NULL,
> -};
> +} XBZRLE;
> +
>  /* buffer used for XBZRLE decoding */
>  static uint8_t *xbzrle_decoded_buf;
> 
> @@ -187,41 +185,44 @@ static void XBZRLE_cache_unlock(void)
>          qemu_mutex_unlock(&XBZRLE.lock);
>  }
> 
> +/*
> + * called from qmp_migrate_set_cache_size in main thread, possibly while
> + * a migration is in progress.
> + * A running migration maybe using the cache and might finish during this
> + * call, hence changes to the cache are protected by XBZRLE.lock().
> + */
>  int64_t xbzrle_cache_resize(int64_t new_size)
>  {
> -    PageCache *new_cache, *cache_to_free;
> +    PageCache *new_cache;
> +    int64_t ret;
> 
>      if (new_size < TARGET_PAGE_SIZE) {
>          return -1;
>      }
> 
> -    /* no need to lock, the current thread holds qemu big lock */
> +    XBZRLE_cache_lock();
> +
>      if (XBZRLE.cache != NULL) {
> -        /* check XBZRLE.cache again later */
>          if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
> -            return pow2floor(new_size);
> +            goto out_new_size;
>          }
>          new_cache = cache_init(new_size / TARGET_PAGE_SIZE,
>                                          TARGET_PAGE_SIZE);
>          if (!new_cache) {
> -            DPRINTF("Error creating cache\n");
> -            return -1;
> -        }
> -
> -        XBZRLE_cache_lock();
> -        /* the XBZRLE.cache may have be destroyed, check it again */
> -        if (XBZRLE.cache != NULL) {
> -            cache_to_free = XBZRLE.cache;
> -            XBZRLE.cache = new_cache;
> -        } else {
> -            cache_to_free = new_cache;
> +            error_report("Error creating cache");
> +            ret = -1;
> +            goto out;
>          }
> -        XBZRLE_cache_unlock();
> 
> -        cache_fini(cache_to_free);
> +        cache_fini(XBZRLE.cache);
> +        XBZRLE.cache = new_cache;
>      }
> 
> -    return pow2floor(new_size);
> +out_new_size:
> +    ret = pow2floor(new_size);
> +out:
> +    XBZRLE_cache_unlock();
> +    return ret;
>  }
> 
>  /* accounting for migration statistics */
> @@ -735,28 +736,27 @@ static int ram_save_setup(QEMUFile *f, void
> *opaque)
>      dirty_rate_high_cnt = 0;
> 
>      if (migrate_use_xbzrle()) {
> -        qemu_mutex_lock_iothread();
> +        XBZRLE_cache_lock();
>          XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
>                                    TARGET_PAGE_SIZE,
>                                    TARGET_PAGE_SIZE);
>          if (!XBZRLE.cache) {
> -            qemu_mutex_unlock_iothread();
> -            DPRINTF("Error creating cache\n");
> +            XBZRLE_cache_unlock();
> +            error_report("Error creating cache");
>              return -1;
>          }
> -        qemu_mutex_init(&XBZRLE.lock);
> -        qemu_mutex_unlock_iothread();
> +        XBZRLE_cache_unlock();
> 
>          /* We prefer not to abort if there is no memory */
>          XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
>          if (!XBZRLE.encoded_buf) {
> -            DPRINTF("Error allocating encoded_buf\n");
> +            error_report("Error allocating encoded_buf");
>              return -1;
>          }
> 
>          XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
>          if (!XBZRLE.current_buf) {
> -            DPRINTF("Error allocating current_buf\n");
> +            error_report("Error allocating current_buf");
>              g_free(XBZRLE.encoded_buf);
>              XBZRLE.encoded_buf = NULL;
>              return -1;
> @@ -1106,6 +1106,7 @@ static SaveVMHandlers savevm_ram_handlers = {
> 
>  void ram_mig_init(void)
>  {
> +    qemu_mutex_init(&XBZRLE.lock);
>      register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers,
> NULL);
>  }
> 
> --
> 1.8.5.3

Reviewed-by: Gonglei <arei.gonglei@huawei.com>

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 0/2] Stop reinit of XBZRLE.lock

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Markus Armbruster spotted that the XBZRLE.lock might get initalised
> > multiple times in the case of a second attempted migration, and
> > that's undefined behaviour for pthread_mutex_init.
> >
> > This patchset is based on Markus' proto-patch in the discussions
> > off v1.
> >
> > --
> > v2
> >    Rework completely based on Markus's patch
> >
> > Dr. David Alan Gilbert (2):
> >   Provide init function for ram migration
> >   Init the XBZRLE.lock in ram_mig_init
> 
> Second patch does a fair but more than its subject suggests.  Perhaps
> "XBZRLE: Rework locking to fix double initialization" would be better.
> Still doesn't cover the upgrade from debugging prints to proper error
> reporting.  Maybe that should be a separate patch.  Up to you.

I added the error reporting ones, because I ended up respinning my last
patch after Paolo asked for them.

Dave

> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK