Stop reinit of XBZRLE.lock

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 patch adds a flag to stop re-initialisation - finding somewhere to cleanly destroy it where we can guarantee isn't happening at the same place we might want to take the lock is tricky.

It also adds comments to explain the lock use more.

(I considered rewriting this lockless but can't justify it given the simplicity of the fix).

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
arch_init.c | 30 ++++++++++++++++++++++++++----

1 file changed, 26 insertions(+), 4 deletions(-)

arch_init.c +0 -0

index 60c975d..16474b5 100644

arch_init.c +14 -3

 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;

@@ -187,6 +190,11 @@ 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;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
        return -1;
    }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
    if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
        if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
            return pow2floor(new_size);
        }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
        }
 
        XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
        if (XBZRLE.cache != NULL) {
            cache_to_free = XBZRLE.cache;
            XBZRLE.cache = new_cache;

arch_init.c +9 -0

+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that
+         * we can guarantee isn't being called when we might want
+         * to hold the lock.
+         */
+        if (!XBZRLE.lock_init) {
+            XBZRLE.lock_init = true;
+            qemu_mutex_init(&XBZRLE.lock);
+        }
        qemu_mutex_unlock_iothread();
 
        /* We prefer not to abort if there is no memory */

Signature · 1 line

1.8.5.3

nice catch

Quoted 25 lines

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 patch adds a flag to stop re-initialisation - finding somewhere to cleanly destroy it where we can guarantee isn't happening at the same place we might want to take the lock is tricky.

It also adds comments to explain the lock use more.

(I considered rewriting this lockless but can't justify it given the simplicity of the fix).

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
arch_init.c | 30 ++++++++++++++++++++++++++----

1 file changed, 26 insertions(+), 4 deletions(-)

arch_init.c +0 -0

index 60c975d..16474b5 100644

arch_init.c static struct { · line 169

Quoted 3 lines

arch_init.c +0 -0

@@ -167,10 +167,13 @@ static struct {
    /* Cache for XBZRLE, Protected by lock. */
    PageCache *cache;
    QemuMutex lock;

QemuMutex *lock;

arch_init.c static struct { · line 174

Quoted 5 lines

arch_init.c +1 -0

+    bool lock_init; /* True once we've init'd lock */
 } XBZRLE = {
    .encoded_buf = NULL,
    .current_buf = NULL,
    .cache = NULL,

.lock = NULL,

arch_init.c static struct { · line 177

Quoted 3 lines

arch_init.c +2 -0

+    /* .lock is initialised in ram_save_setup */
+    .lock_init = false
 };

maybe it is better that we malloc the lock in ram_save_setup dynamic, and free it in migration_end.

Quoted 42 lines

arch_init.c +14 -3

 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;

@@ -187,6 +190,11 @@ 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;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
        return -1;
    }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
    if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
        if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
            return pow2floor(new_size);
        }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
        }
 
        XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
        if (XBZRLE.cache != NULL) {
            cache_to_free = XBZRLE.cache;
            XBZRLE.cache = new_cache;

arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 747

Quoted 5 lines

arch_init.c +0 -1

@@ -744,7 +758,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
            DPRINTF("Error creating cache\n");
            return -1;
        }
-        qemu_mutex_init(&XBZRLE.lock);

we malloc the lock and init it. Then free it in migration_end. It is safe because migration_end holds qemu big lock.

Quoted 11 lines

arch_init.c +9 -0

+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that
+         * we can guarantee isn't being called when we might want
+         * to hold the lock.
+         */
+        if (!XBZRLE.lock_init) {
+            XBZRLE.lock_init = true;
+            qemu_mutex_init(&XBZRLE.lock);
+        }
        qemu_mutex_unlock_iothread();
 

Quoted 5 lines

arch_init.c +0 -0

        /* We prefer not to abort if there is no memory */

Signature · 1 line

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 patch adds a flag to stop re-initialisation - finding somewhere to cleanly destroy it where we can guarantee isn't happening at the same place we might want to take the lock is tricky.

It also adds comments to explain the lock use more.

(I considered rewriting this lockless but can't justify it given the simplicity of the fix).

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 arch_init.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

arch_init.c +0 -0

index 60c975d..16474b5 100644
@@ -167,10 +167,13 @@ static struct {
     /* Cache for XBZRLE, Protected by lock. */

arch_init.c +11 -4

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
     PageCache *new_cache, *cache_to_free;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         return -1;
     }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
     if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
             return pow2floor(new_size);
         }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         }
 
         XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
         if (XBZRLE.cache != NULL) {
             cache_to_free = XBZRLE.cache;
             XBZRLE.cache = new_cache;

@@ -744,7 +758,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        qemu_mutex_init(&XBZRLE.lock);
+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that

"Dr. David Alan Gilbert (git)" <[email protected]> writes:

Quoted 10 lines

arch_init.c +1 -0

index 60c975d..16474b5 100644
@@ -167,10 +167,13 @@ static struct {
     /* Cache for XBZRLE, Protected by lock. */
     PageCache *cache;
     QemuMutex lock;
+    bool lock_init; /* True once we've init'd lock */
 } XBZRLE = {

arch_init.c static struct { · line 177

Quoted 6 lines

arch_init.c +2 -0

     .encoded_buf = NULL,
     .current_buf = NULL,
     .cache = NULL,
+    /* .lock is initialised in ram_save_setup */
+    .lock_init = false
 };

Redundant initializers.

arch_init.c static void XBZRLE_cache_unlock(void) · line 195

Quoted 9 lines

arch_init.c +3 -0

 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;

@@ -187,6 +190,11 @@ 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

may be

arch_init.c static void XBZRLE_cache_unlock(void) · line 197

Quoted 2 lines

arch_init.c +2 -0

+ * call, hence changes to the cache are protected by XBZRLE.lock().
+ */

Style nit, since I'm nitpicking spelling already: our winged comments usually look like

/*
* Text
*/

Quoted 46 lines

arch_init.c +18 -4

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
     PageCache *new_cache, *cache_to_free;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         return -1;
     }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
     if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
             return pow2floor(new_size);
         }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         }
 
         XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
         if (XBZRLE.cache != NULL) {
             cache_to_free = XBZRLE.cache;
             XBZRLE.cache = new_cache;

@@ -744,7 +758,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        qemu_mutex_init(&XBZRLE.lock);
+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that
+         * we can guarantee isn't being called when we might want
+         * to hold the lock.
+         */
+        if (!XBZRLE.lock_init) {
+            XBZRLE.lock_init = true;
+            qemu_mutex_init(&XBZRLE.lock);
+        }
         qemu_mutex_unlock_iothread();
 

arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 772

Quoted 1 line

arch_init.c +0 -0

         /* We prefer not to abort if there is no memory */

I detest how the locking works in xbzrle_cache_resize().

The first XBZRLE.cache != NULL is not under XBZRLE.lock. As you explain in the comment, this is required, because we foolishly delay lock initialization until a migration starts, and it's safe, because cache creation is also under the BQL.

The second XBZRLE.cache != NULL *is* under XBZRLE.lock. Required, because cache destruction is *not* inder the BQL, only under XBZRLE.lock.

I'd very, very much prefer this to be made obviously safe: initialize XBZRLE.lock sufficiently early, then access XBZRLE.cache only under XBZRLE.lock.

Confusing and way too subtle for no good reason. But your patch doesn't add subtlety, it explains it, and fixes a bug. Therefore:

Reviewed-by Markus Armbruster <[email protected]>

Quoted 1 line · * Markus Armbruster ([email protected]) wrote:

"Dr. David Alan Gilbert (git)" <[email protected]> writes:

<snip>

Quoted 10 lines

Quoted 10 lines

arch_init.c +1 -0

index 60c975d..16474b5 100644
@@ -167,10 +167,13 @@ static struct {
     /* Cache for XBZRLE, Protected by lock. */
     PageCache *cache;
     QemuMutex lock;
+    bool lock_init; /* True once we've init'd lock */
 } XBZRLE = {

Quoted 8 lines

arch_init.c static struct { · line 177

Quoted 6 lines

arch_init.c +2 -0

     .encoded_buf = NULL,
     .current_buf = NULL,
     .cache = NULL,
+    /* .lock is initialised in ram_save_setup */
+    .lock_init = false
 };

Redundant initializers.

Given how subtle lock stuff is, I'll take making it obvious as more important.

Quoted 15 lines

arch_init.c static void XBZRLE_cache_unlock(void) · line 195

Quoted 9 lines

arch_init.c +3 -0

 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;

@@ -187,6 +190,11 @@ 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

may be

arch_init.c static void XBZRLE_cache_unlock(void) · line 197

Quoted 2 lines

arch_init.c +2 -0

+ * call, hence changes to the cache are protected by XBZRLE.lock().
+ */

Quoted 6 lines

Style nit, since I'm nitpicking spelling already: our winged comments usually look like

/*
* Text
*/

Oops, yes; if I need to respin I'll fix that (hmm I wonder if the check script could be tweaked to find those).

Quoted 46 lines

Quoted 46 lines

arch_init.c +18 -4

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
     PageCache *new_cache, *cache_to_free;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         return -1;
     }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
     if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
             return pow2floor(new_size);
         }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         }
 
         XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
         if (XBZRLE.cache != NULL) {
             cache_to_free = XBZRLE.cache;
             XBZRLE.cache = new_cache;

@@ -744,7 +758,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        qemu_mutex_init(&XBZRLE.lock);
+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that
+         * we can guarantee isn't being called when we might want
+         * to hold the lock.
+         */
+        if (!XBZRLE.lock_init) {
+            XBZRLE.lock_init = true;
+            qemu_mutex_init(&XBZRLE.lock);
+        }
         qemu_mutex_unlock_iothread();
 

Quoted 3 lines

arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 772

Quoted 1 line

arch_init.c +0 -0

         /* We prefer not to abort if there is no memory */

I detest how the locking works in xbzrle_cache_resize().

Yeh, it's tricky - I think the way to think about it is that the lock protects the cache and it's contents, not the pointer. Except that's not really true when it swaps the new one in - hmm.

Quoted 4 lines

The first XBZRLE.cache != NULL is not under XBZRLE.lock. As you explain in the comment, this is required, because we foolishly delay lock initialization until a migration starts, and it's safe, because cache creation is also under the BQL.

Some of this is down to the interfaces between migration generally, the devices being migrated and the specials for RAM/iterative migration.

1) A lot of the ram migration data, including XBZRLE, is global data in arch_init - this is bad.

2) The management of these is generally glued onto the migration setup/iterate/complete set of methods used during migration, however they don't have any calls that correspond to the actual start/end of migration, or the like that could be sanely used to do any init that tied up with the actual start end of migration - this is bad.

3) Migration is in a separate thread and could finish at any time - this is expected but complicates life, especially when (2) means that the data structures for RAMs migration etc are cleared up when migration finishes.

I don't know the history but (1) seems to arrise from the semi-arbitrary split between arch_init.c, memory.c, savevm.c, and probably 3 or 4 other files I've failed to mention.

From 6ab76b05789de80d4e0919404429fadcbc0ea403 Mon Sep 17 00:00:00 2001

Quoted 24 lines · Dr. David Alan Gilbert

Quoted 1 line · * Markus Armbruster ([email protected]) wrote:

"Dr. David Alan Gilbert (git)" <[email protected]> writes:

<snip>

Quoted 10 lines

Quoted 10 lines

arch_init.c +1 -0

index 60c975d..16474b5 100644
@@ -167,10 +167,13 @@ static struct {
     /* Cache for XBZRLE, Protected by lock. */
     PageCache *cache;
     QemuMutex lock;
+    bool lock_init; /* True once we've init'd lock */
 } XBZRLE = {

Quoted 8 lines

arch_init.c static struct { · line 177

Quoted 6 lines

arch_init.c +2 -0

     .encoded_buf = NULL,
     .current_buf = NULL,
     .cache = NULL,
+    /* .lock is initialised in ram_save_setup */
+    .lock_init = false
 };

Redundant initializers.

Quoted 1 line

Given how subtle lock stuff is, I'll take making it obvious as more important.

That a static variable starts out zeroed is plenty obvious. More obvious in fact than a bunch of explicit initializers I actually have to read to see what they mean.

We rely on implicit zero-initialization of static variables all over the place.

Quoted 75 lines

Quoted 15 lines

arch_init.c static void XBZRLE_cache_unlock(void) · line 195

Quoted 9 lines

arch_init.c +3 -0

 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;

@@ -187,6 +190,11 @@ 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

may be

arch_init.c static void XBZRLE_cache_unlock(void) · line 197

Quoted 2 lines

arch_init.c +2 -0

+ * call, hence changes to the cache are protected by XBZRLE.lock().
+ */

Quoted 6 lines

Style nit, since I'm nitpicking spelling already: our winged comments usually look like

/*
* Text
*/

Oops, yes; if I need to respin I'll fix that (hmm I wonder if the check script could be tweaked to find those).

Quoted 49 lines

Quoted 37 lines

arch_init.c +11 -4

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
     PageCache *new_cache, *cache_to_free;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         return -1;
     }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
     if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
             return pow2floor(new_size);
         }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         }
 
         XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
         if (XBZRLE.cache != NULL) {
             cache_to_free = XBZRLE.cache;
             XBZRLE.cache = new_cache;

@@ -744,7 +758,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        qemu_mutex_init(&XBZRLE.lock);
+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that

arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 772

Quoted 10 lines

arch_init.c +7 -0

+         * we can guarantee isn't being called when we might want
+         * to hold the lock.
+         */
+        if (!XBZRLE.lock_init) {
+            XBZRLE.lock_init = true;
+            qemu_mutex_init(&XBZRLE.lock);
+        }
         qemu_mutex_unlock_iothread();
 
         /* We prefer not to abort if there is no memory */

I detest how the locking works in xbzrle_cache_resize().

Quoted 3 lines

Yeh, it's tricky - I think the way to think about it is that the lock protects the cache and it's contents, not the pointer. Except that's not really true when it swaps the new one in - hmm.

Yup, and that's what I call confusing and way too subtle.

Quoted 5 lines

Quoted 4 lines

The first XBZRLE.cache != NULL is not under XBZRLE.lock. As you explain in the comment, this is required, because we foolishly delay lock initialization until a migration starts, and it's safe, because cache creation is also under the BQL.

Quoted 5 lines

Some of this is down to the interfaces between migration generally, the devices being migrated and the specials for RAM/iterative migration.

1) A lot of the ram migration data, including XBZRLE, is global data in arch_init - this is bad.

As long as there can only be one migration active at a time, it's not really bad, isn't it?

Quoted 5 lines

2) The management of these is generally glued onto the migration setup/iterate/complete set of methods used during migration, however they don't have any calls that correspond to the actual start/end of migration, or the like that could be sanely used to do any init that tied up with the actual start end of migration - this is bad.

Maybe. But why delay the initialization of XBZRLE.lock to start of migration? All the other members of XBZRLE are initialized on start of program!

Quoted 4 lines

3) Migration is in a separate thread and could finish at any time - this is expected but complicates life, especially when (2) means that the data structures for RAMs migration etc are cleared up when migration finishes.

Four activities: initialization, cache setup, cache use, cache teardown.

Initialization can be done during program startup, where we don't have to worry about threads and locks. The other three can then be simply put under the lock, and the comment "Protected by lock" becomes actually true. Sketch appended. It passes "make check", it must be purrfect!

Quoted 3 lines

I don't know the history but (1) seems to arrise from the semi-arbitrary split between arch_init.c, memory.c, savevm.c, and probably 3 or 4 other files I've failed to mention.

[...]

From 6ab76b05789de80d4e0919404429fadcbc0ea403 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <[email protected]>
Date: Wed, 19 Mar 2014 08:46:51 +0100
Subject: [PATCH] XBZRLE: Fix and simplify locking of cache WIP

---
 arch_init.c                | 49 ++++++++++++++++++++++------------------------
 include/sysemu/arch_init.h |  1 +
 vl.c                       |  1 +
 3 files changed, 25 insertions(+), 26 deletions(-)

arch_init.c +23 -26

index 60c975d..e41e9dd 100644
@@ -167,14 +167,16 @@ 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;
 
+void XBZRLE_init(void)
+{
+    qemu_mutex_init(&XBZRLE.lock);
+}
+
 static void XBZRLE_cache_lock(void)
 {
     if (migrate_use_xbzrle())

@@ -189,39 +191,35 @@ static void XBZRLE_cache_unlock(void)
 
 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;
+            goto out;
         }
 
-        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;
-        }
-        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,17 +733,16 @@ 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();
+            XBZRLE_cache_unlock();
             DPRINTF("Error creating cache\n");
             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);

include/sysemu/arch_init.h +1 -0

index be71bca..fdc3423 100644
@@ -26,6 +26,7 @@ enum {
 
 extern const uint32_t arch_type;
 
+void XBZRLE_init(void);
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const QemuOpts *opts);
 void do_smbios_option(QemuOpts *opts);

vl.c +1 -0

index f0fe48b..24433c8 100644
@@ -3008,6 +3008,7 @@ int main(int argc, char **argv, char **envp)
 
     qemu_init_auxval(envp);
     qemu_cache_utils_init();
+    XBZRLE_init();
 
     QLIST_INIT (&vm_change_state_head);
     os_setup_early_signal_handling();

Signature · 1 line

1.8.1.4

Quoted 111 lines · * Markus Armbruster ([email protected]) wrote:

Quoted 24 lines · Dr. David Alan Gilbert

Quoted 1 line · * Markus Armbruster ([email protected]) wrote:

"Dr. David Alan Gilbert (git)" <[email protected]> writes:

<snip>

Quoted 10 lines

Quoted 10 lines

arch_init.c +1 -0

index 60c975d..16474b5 100644
@@ -167,10 +167,13 @@ static struct {
     /* Cache for XBZRLE, Protected by lock. */
     PageCache *cache;
     QemuMutex lock;
+    bool lock_init; /* True once we've init'd lock */
 } XBZRLE = {

Quoted 8 lines

arch_init.c static struct { · line 177

Quoted 6 lines

arch_init.c +2 -0

     .encoded_buf = NULL,
     .current_buf = NULL,
     .cache = NULL,
+    /* .lock is initialised in ram_save_setup */
+    .lock_init = false
 };

Redundant initializers.

Quoted 2 lines

Given how subtle lock stuff is, I'll take making it obvious as more important.

That a static variable starts out zeroed is plenty obvious. More obvious in fact than a bunch of explicit initializers I actually have to read to see what they mean.

We rely on implicit zero-initialization of static variables all over the place.

Quoted 75 lines

Quoted 15 lines

arch_init.c static void XBZRLE_cache_unlock(void) · line 195

Quoted 9 lines

arch_init.c +3 -0

 /* buffer used for XBZRLE decoding */
 static uint8_t *xbzrle_decoded_buf;

@@ -187,6 +190,11 @@ 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

may be

arch_init.c static void XBZRLE_cache_unlock(void) · line 197

Quoted 2 lines

arch_init.c +2 -0

+ * call, hence changes to the cache are protected by XBZRLE.lock().
+ */

Quoted 6 lines

Style nit, since I'm nitpicking spelling already: our winged comments usually look like

/*
* Text
*/

Oops, yes; if I need to respin I'll fix that (hmm I wonder if the check script could be tweaked to find those).

Quoted 49 lines

Quoted 37 lines

arch_init.c +11 -4

 int64_t xbzrle_cache_resize(int64_t new_size)
 {
     PageCache *new_cache, *cache_to_free;

@@ -195,9 +203,12 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         return -1;
     }
 
-    /* no need to lock, the current thread holds qemu big lock */
+    /* The current thread holds qemu big lock, and we hold it while creating
+     * the cache in ram_save_setup, thus it's safe to test if the
+     * cache exists yet without it's own lock (which might not have been
+     * init'd yet)
+     */
     if (XBZRLE.cache != NULL) {
-        /* check XBZRLE.cache again later */
         if (pow2floor(new_size) == migrate_xbzrle_cache_size()) {
             return pow2floor(new_size);
         }

@@ -209,7 +220,10 @@ int64_t xbzrle_cache_resize(int64_t new_size)
         }
 
         XBZRLE_cache_lock();
-        /* the XBZRLE.cache may have be destroyed, check it again */
+        /* the migration might have finished between the check above and us
+         * taking the lock,  causing XBZRLE.cache to be destroyed
+         *   check it again
+         */
         if (XBZRLE.cache != NULL) {
             cache_to_free = XBZRLE.cache;
             XBZRLE.cache = new_cache;

@@ -744,7 +758,15 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
             DPRINTF("Error creating cache\n");
             return -1;
         }
-        qemu_mutex_init(&XBZRLE.lock);
+        /* mutex's can't be reinit'd without destroying them
+         * and we've not got a good place to destroy it that

arch_init.c static int ram_save_setup(QEMUFile *f, void *opaque) · line 772

Quoted 10 lines

arch_init.c +7 -0

+         * we can guarantee isn't being called when we might want
+         * to hold the lock.
+         */
+        if (!XBZRLE.lock_init) {
+            XBZRLE.lock_init = true;
+            qemu_mutex_init(&XBZRLE.lock);
+        }
         qemu_mutex_unlock_iothread();
 
         /* We prefer not to abort if there is no memory */

I detest how the locking works in xbzrle_cache_resize().

Quoted 5 lines

Quoted 3 lines

Yeh, it's tricky - I think the way to think about it is that the lock protects the cache and it's contents, not the pointer. Except that's not really true when it swaps the new one in - hmm.

Yup, and that's what I call confusing and way too subtle.

Yes, agreed - this shouldn't be a hard problem that needs subtlety.

Quoted 11 lines

Quoted 10 lines

Quoted 4 lines

The first XBZRLE.cache != NULL is not under XBZRLE.lock. As you explain in the comment, this is required, because we foolishly delay lock initialization until a migration starts, and it's safe, because cache creation is also under the BQL.

Some of this is down to the interfaces between migration generally, the devices being migrated and the specials for RAM/iterative migration.

1) A lot of the ram migration data, including XBZRLE, is global data in arch_init - this is bad.

Quoted 2 lines

As long as there can only be one migration active at a time, it's not really bad, isn't it?

I find it quite messy; there is state associated with migration all over, I couldn't honestly tell you all the things that need to be initialised, updated, etc when a migration is in progress. I'd rather hang stuff off MigrationState.

Quoted 6 lines

Quoted 5 lines

2) The management of these is generally glued onto the migration setup/iterate/complete set of methods used during migration, however they don't have any calls that correspond to the actual start/end of migration, or the like that could be sanely used to do any init that tied up with the actual start end of migration - this is bad.

Quoted 3 lines

Maybe. But why delay the initialization of XBZRLE.lock to start of migration? All the other members of XBZRLE are initialized on start of program!

That's just fall out from the shock that you couldn't staticly init the lock, the original patch version statically init'd the lock in the same way as the other members.

Quoted 11 lines

Quoted 4 lines

3) Migration is in a separate thread and could finish at any time - this is expected but complicates life, especially when (2) means that the data structures for RAMs migration etc are cleared up when migration finishes.

Four activities: initialization, cache setup, cache use, cache teardown.

Initialization can be done during program startup, where we don't have to worry about threads and locks. The other three can then be simply put under the lock, and the comment "Protected by lock" becomes actually true. Sketch appended. It passes "make check", it must be purrfect!

I'm mostly OK with this, with one small change at the bottom.

Quoted 144 lines

Quoted 3 lines

I don't know the history but (1) seems to arrise from the semi-arbitrary split between arch_init.c, memory.c, savevm.c, and probably 3 or 4 other files I've failed to mention.

[...]

From 6ab76b05789de80d4e0919404429fadcbc0ea403 Mon Sep 17 00:00:00 2001
From: Markus Armbruster <[email protected]>
Date: Wed, 19 Mar 2014 08:46:51 +0100
Subject: [PATCH] XBZRLE: Fix and simplify locking of cache WIP

---
 arch_init.c                | 49 ++++++++++++++++++++++------------------------
 include/sysemu/arch_init.h |  1 +
 vl.c                       |  1 +
 3 files changed, 25 insertions(+), 26 deletions(-)

arch_init.c +23 -26

index 60c975d..e41e9dd 100644
@@ -167,14 +167,16 @@ 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;
 
+void XBZRLE_init(void)
+{
+    qemu_mutex_init(&XBZRLE.lock);
+}
+
 static void XBZRLE_cache_lock(void)
 {
     if (migrate_use_xbzrle())

@@ -189,39 +191,35 @@ static void XBZRLE_cache_unlock(void)
 
 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;
+            goto out;
         }
 
-        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;
-        }
-        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,17 +733,16 @@ 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();
+            XBZRLE_cache_unlock();
             DPRINTF("Error creating cache\n");
             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);

include/sysemu/arch_init.h +1 -0

index be71bca..fdc3423 100644
@@ -26,6 +26,7 @@ enum {
 
 extern const uint32_t arch_type;
 
+void XBZRLE_init(void);
 void select_soundhw(const char *optarg);
 void do_acpitable_option(const QemuOpts *opts);
 void do_smbios_option(QemuOpts *opts);

vl.c +1 -0

index f0fe48b..24433c8 100644
@@ -3008,6 +3008,7 @@ int main(int argc, char **argv, char **envp)
 
     qemu_init_auxval(envp);
     qemu_cache_utils_init();
+    XBZRLE_init();
 
     QLIST_INIT (&vm_change_state_head);
     os_setup_early_signal_handling();

Signature · 1 line

1.8.1.4

I noticed that in main.c there is a call to a blk_mig_init() - so how about (not even build tested):

vl.c +0 -0

index 842e897..ab8cbf0 100644

vl.c +0 -2

@@ -4261,8 +4262,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;

arch_init.c +7 -1

index 16474b5..4f8376d 100644
@@ -190,6 +190,12 @@ static void XBZRLE_cache_unlock(void)
         qemu_mutex_unlock(&XBZRLE.lock);
 }
 
+void ram_mig_init(void)
+{
+    qemu_mutex_init(&XBZRLE.lock);
+    register_savevm_live(NULL, "ram", 0, 4, &savevm_ram_handlers, NULL);
+}
+
 /* 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

@@ -1117,7 +1123,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,

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

and that way we're simplifying vl.c rather than teaching it even more about the innards of ram migration.