Stop reinit of XBZRLE.lock

Subject: [PATCH v2 1/2] Provide init function for ram migration

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

---
 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(-)

arch_init.c +6 -1

index 60c975d..0d2bf84 100644
@@ -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;

include/migration/migration.h +0 -2

index 3e1e6c7..31fbf17 100644
@@ -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);

include/sysemu/arch_init.h +1 -0

index be71bca..182d48d 100644
@@ -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);

vl.c +1 -3

index f0fe48b..3358c98 100644
@@ -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

Subject: [PATCH v2 2/2] Init the XBZRLE.lock in ram_mig_init

From: "Dr. David Alan Gilbert" <[email protected]>

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 <[email protected]>

---
 arch_init.c | 61 +++++++++++++++++++++++++++++++------------------------------
 1 file changed, 31 insertions(+), 30 deletions(-)

arch_init.c +30 -30

index 0d2bf84..f18f42e 100644
@@ -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;

arch_init.c +1 -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

From: "Dr. David Alan Gilbert" <[email protected]>

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

Quoted 13 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>

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

Quoted 3 lines

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 <[email protected]>