[Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init

[Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init

[Peter Xu]

I saw some leaks during ram setup, so trying to fix them.  Since at
it, I cleaned up the ram init phase a bit along with ram cleanup.

Please review.  Thanks,

Peter Xu (4):
  migration: provide ram_state_init()
  migration: provide ram_state_cleanup
  migration: clean up xbzrle cache init/destroy
  migration: new ram_init_bitmaps()

 migration/ram.c | 194 +++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 128 insertions(+), 66 deletions(-)

-- 
2.13.5

[Qemu-devel] [PATCH 1/4] migration: provide ram_state_init()

[Peter Xu]

The old ram_state_init() is not really initializing the RAMState only,
but including lots of other stuff that is RAM-related.  Renaming it to
ram_init_all().  Instead, provide a real ram_state_init().

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index b83f8977c5..fd8e14c59b 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2009,12 +2009,34 @@ err:
 
 static int ram_state_init(RAMState **rsp)
 {
-    *rsp = g_new0(RAMState, 1);
+    *rsp = g_try_new0(RAMState, 1);
+
+    if (!*rsp) {
+        error_report("%s: Init ramstate fail", __func__);
+        return -1;
+    }
 
     qemu_mutex_init(&(*rsp)->bitmap_mutex);
     qemu_mutex_init(&(*rsp)->src_page_req_mutex);
     QSIMPLEQ_INIT(&(*rsp)->src_page_requests);
 
+    /*
+     * Count the total number of pages used by ram blocks not including any
+     * gaps due to alignment or unplugs.
+     */
+    (*rsp)->migration_dirty_pages = ram_bytes_total() >> TARGET_PAGE_BITS;
+
+    ram_state_reset(*rsp);
+
+    return 0;
+}
+
+static int ram_init_all(RAMState **rsp)
+{
+    if (ram_state_init(rsp)) {
+        return -1;
+    }
+
     if (migrate_use_xbzrle()) {
         XBZRLE_cache_lock();
         XBZRLE.zero_target_page = g_malloc0(TARGET_PAGE_SIZE);
@@ -2055,7 +2077,6 @@ static int ram_state_init(RAMState **rsp)
 
     qemu_mutex_lock_ramlist();
     rcu_read_lock();
-    ram_state_reset(*rsp);
 
     /* Skip setting bitmap if there is no RAM */
     if (ram_bytes_total()) {
@@ -2073,12 +2094,6 @@ static int ram_state_init(RAMState **rsp)
         }
     }
 
-    /*
-     * Count the total number of pages used by ram blocks not including any
-     * gaps due to alignment or unplugs.
-     */
-    (*rsp)->migration_dirty_pages = ram_bytes_total() >> TARGET_PAGE_BITS;
-
     memory_global_dirty_log_start();
     migration_bitmap_sync(*rsp);
     qemu_mutex_unlock_ramlist();
@@ -2110,7 +2125,7 @@ static int ram_save_setup(QEMUFile *f, void *opaque)
 
     /* migration has already setup the bitmap, reuse it. */
     if (!migration_in_colo_state()) {
-        if (ram_state_init(rsp) != 0) {
+        if (ram_init_all(rsp) != 0) {
             return -1;
         }
     }
-- 
2.13.5

Re: [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> The old ram_state_init() is not really initializing the RAMState only,
> but including lots of other stuff that is RAM-related.  Renaming it to
> ram_init_all().  Instead, provide a real ram_state_init().
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup

[Peter Xu]

There are two Mutexes that are created but not yet destroyed for
RAMState.  Fix that.

Since we are at it, provide helper function to clean up RAMState.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index fd8e14c59b..72c48e76e9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1566,6 +1566,15 @@ static void xbzrle_load_cleanup(void)
     XBZRLE.decoded_buf = NULL;
 }
 
+static void ram_state_cleanup(RAMState **rsp)
+{
+    migration_page_queue_free(*rsp);
+    qemu_mutex_destroy(&(*rsp)->bitmap_mutex);
+    qemu_mutex_destroy(&(*rsp)->src_page_req_mutex);
+    g_free(*rsp);
+    *rsp = NULL;
+}
+
 static void ram_save_cleanup(void *opaque)
 {
     RAMState **rsp = opaque;
@@ -1595,10 +1604,8 @@ static void ram_save_cleanup(void *opaque)
         XBZRLE.zero_target_page = NULL;
     }
     XBZRLE_cache_unlock();
-    migration_page_queue_free(*rsp);
     compress_threads_save_cleanup();
-    g_free(*rsp);
-    *rsp = NULL;
+    ram_state_cleanup(rsp);
 }
 
 static void ram_state_reset(RAMState *rs)
-- 
2.13.5

Re: [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> There are two Mutexes that are created but not yet destroyed for
> RAMState.  Fix that.
>
> Since we are at it, provide helper function to clean up RAMState.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy

[Peter Xu]

Let's further simplify ram_init_all() and ram_save_cleanup() by abstract
all the XBZRLE related codes into their own functions.

When allocating xbzrle cache, we are always very careful on -ENOMEM;
which makes sense.  Replacing the last g_malloc0() with g_try_malloc0(),
then refactor the logic a bit.

This patch should be fixing some memory leaks when some memory
allocation failed for XBZRLE in the past.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 121 +++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 76 insertions(+), 45 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 72c48e76e9..3df7715d63 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1575,6 +1575,22 @@ static void ram_state_cleanup(RAMState **rsp)
     *rsp = NULL;
 }
 
+static void xbzrle_cleanup(void)
+{
+    XBZRLE_cache_lock();
+    if (XBZRLE.cache) {
+        cache_fini(XBZRLE.cache);
+        g_free(XBZRLE.encoded_buf);
+        g_free(XBZRLE.current_buf);
+        g_free(XBZRLE.zero_target_page);
+        XBZRLE.cache = NULL;
+        XBZRLE.encoded_buf = NULL;
+        XBZRLE.current_buf = NULL;
+        XBZRLE.zero_target_page = NULL;
+    }
+    XBZRLE_cache_unlock();
+}
+
 static void ram_save_cleanup(void *opaque)
 {
     RAMState **rsp = opaque;
@@ -1592,18 +1608,7 @@ static void ram_save_cleanup(void *opaque)
         block->unsentmap = NULL;
     }
 
-    XBZRLE_cache_lock();
-    if (XBZRLE.cache) {
-        cache_fini(XBZRLE.cache);
-        g_free(XBZRLE.encoded_buf);
-        g_free(XBZRLE.current_buf);
-        g_free(XBZRLE.zero_target_page);
-        XBZRLE.cache = NULL;
-        XBZRLE.encoded_buf = NULL;
-        XBZRLE.current_buf = NULL;
-        XBZRLE.zero_target_page = NULL;
-    }
-    XBZRLE_cache_unlock();
+    xbzrle_cleanup();
     compress_threads_save_cleanup();
     ram_state_cleanup(rsp);
 }
@@ -2014,6 +2019,62 @@ err:
     return ret;
 }
 
+/*
+ * For every allocation, we will try not to crash the VM if the
+ * allocation failed.
+ */
+static int xbzrle_init(void)
+{
+    if (!migrate_use_xbzrle()) {
+        return 0;
+    }
+
+    XBZRLE_cache_lock();
+
+    XBZRLE.zero_target_page = g_try_malloc0(TARGET_PAGE_SIZE);
+    if (!XBZRLE.zero_target_page) {
+        error_report("%s: Error allocating zero page", __func__);
+        goto err_out;
+    }
+
+    XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
+                              TARGET_PAGE_SIZE,
+                              TARGET_PAGE_SIZE);
+    if (!XBZRLE.cache) {
+        error_report("%s: Error creating cache", __func__);
+        goto free_zero_page;
+    }
+
+    XBZRLE.encoded_buf = g_try_malloc0(TARGET_PAGE_SIZE);
+    if (!XBZRLE.encoded_buf) {
+        error_report("%s: Error allocating encoded_buf", __func__);
+        goto free_cache;
+    }
+
+    XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
+    if (!XBZRLE.current_buf) {
+        error_report("%s: Error allocating current_buf", __func__);
+        goto free_encoded_buf;
+    }
+
+    /* We are all good */
+    XBZRLE_cache_unlock();
+    return 0;
+
+free_encoded_buf:
+    g_free(XBZRLE.encoded_buf);
+    XBZRLE.encoded_buf = NULL;
+free_cache:
+    cache_fini(XBZRLE.cache);
+    XBZRLE.cache = NULL;
+free_zero_page:
+    g_free(XBZRLE.zero_target_page);
+    XBZRLE.zero_target_page = NULL;
+err_out:
+    XBZRLE_cache_unlock();
+    return -ENOMEM;
+}
+
 static int ram_state_init(RAMState **rsp)
 {
     *rsp = g_try_new0(RAMState, 1);
@@ -2044,39 +2105,9 @@ static int ram_init_all(RAMState **rsp)
         return -1;
     }
 
-    if (migrate_use_xbzrle()) {
-        XBZRLE_cache_lock();
-        XBZRLE.zero_target_page = g_malloc0(TARGET_PAGE_SIZE);
-        XBZRLE.cache = cache_init(migrate_xbzrle_cache_size() /
-                                  TARGET_PAGE_SIZE,
-                                  TARGET_PAGE_SIZE);
-        if (!XBZRLE.cache) {
-            XBZRLE_cache_unlock();
-            error_report("Error creating cache");
-            g_free(*rsp);
-            *rsp = NULL;
-            return -1;
-        }
-        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) {
-            error_report("Error allocating encoded_buf");
-            g_free(*rsp);
-            *rsp = NULL;
-            return -1;
-        }
-
-        XBZRLE.current_buf = g_try_malloc(TARGET_PAGE_SIZE);
-        if (!XBZRLE.current_buf) {
-            error_report("Error allocating current_buf");
-            g_free(XBZRLE.encoded_buf);
-            XBZRLE.encoded_buf = NULL;
-            g_free(*rsp);
-            *rsp = NULL;
-            return -1;
-        }
+    if (xbzrle_init()) {
+        ram_state_cleanup(rsp);
+        return -1;
     }
 
     /* For memory_global_dirty_log_start below.  */
-- 
2.13.5

Re: [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Let's further simplify ram_init_all() and ram_save_cleanup() by abstract
> all the XBZRLE related codes into their own functions.
>
> When allocating xbzrle cache, we are always very careful on -ENOMEM;
> which makes sense.  Replacing the last g_malloc0() with g_try_malloc0(),
> then refactor the logic a bit.
>
> This patch should be fixing some memory leaks when some memory
> allocation failed for XBZRLE in the past.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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

[Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps()

[Peter Xu]

Rearrange the bitmap initialization and the first sync.  Since at it,
make sure the locks are taken/released in correct order (I moved RCU
unlock upper - though it may not affect much).

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/ram.c | 51 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/migration/ram.c b/migration/ram.c
index 3df7715d63..0d24fc12f4 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -2099,30 +2099,15 @@ static int ram_state_init(RAMState **rsp)
     return 0;
 }
 
-static int ram_init_all(RAMState **rsp)
+static void ram_list_init_bitmaps(void)
 {
-    if (ram_state_init(rsp)) {
-        return -1;
-    }
-
-    if (xbzrle_init()) {
-        ram_state_cleanup(rsp);
-        return -1;
-    }
-
-    /* For memory_global_dirty_log_start below.  */
-    qemu_mutex_lock_iothread();
-
-    qemu_mutex_lock_ramlist();
-    rcu_read_lock();
+    RAMBlock *block;
+    unsigned long pages;
 
     /* Skip setting bitmap if there is no RAM */
     if (ram_bytes_total()) {
-        RAMBlock *block;
-
         QLIST_FOREACH_RCU(block, &ram_list.blocks, next) {
-            unsigned long pages = block->max_length >> TARGET_PAGE_BITS;
-
+            pages = block->max_length >> TARGET_PAGE_BITS;
             block->bmap = bitmap_new(pages);
             bitmap_set(block->bmap, 0, pages);
             if (migrate_postcopy_ram()) {
@@ -2131,12 +2116,36 @@ static int ram_init_all(RAMState **rsp)
             }
         }
     }
+}
+
+static void ram_init_bitmaps(RAMState *rs)
+{
+    /* For memory_global_dirty_log_start below.  */
+    qemu_mutex_lock_iothread();
+    qemu_mutex_lock_ramlist();
+    rcu_read_lock();
 
+    ram_list_init_bitmaps();
     memory_global_dirty_log_start();
-    migration_bitmap_sync(*rsp);
+    migration_bitmap_sync(rs);
+
+    rcu_read_unlock();
     qemu_mutex_unlock_ramlist();
     qemu_mutex_unlock_iothread();
-    rcu_read_unlock();
+}
+
+static int ram_init_all(RAMState **rsp)
+{
+    if (ram_state_init(rsp)) {
+        return -1;
+    }
+
+    if (xbzrle_init()) {
+        ram_state_cleanup(rsp);
+        return -1;
+    }
+
+    ram_init_bitmaps(*rsp);
 
     return 0;
 }
-- 
2.13.5

Re: [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps()

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> Rearrange the bitmap initialization and the first sync.  Since at it,
> make sure the locks are taken/released in correct order (I moved RCU
> unlock upper - though it may not affect much).
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

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