* [Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init
@ 2017-10-19 6:31 Peter Xu
2017-10-19 6:31 ` [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init() Peter Xu
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: Peter Xu @ 2017-10-19 6:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Juan Quintela, Dr . David Alan Gilbert, peterx
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init()
2017-10-19 6:31 [Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init Peter Xu
@ 2017-10-19 6:31 ` Peter Xu
2017-10-23 11:18 ` Juan Quintela
2017-10-19 6:31 ` [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup Peter Xu
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Peter Xu @ 2017-10-19 6:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Juan Quintela, Dr . David Alan Gilbert, peterx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup
2017-10-19 6:31 [Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init Peter Xu
2017-10-19 6:31 ` [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init() Peter Xu
@ 2017-10-19 6:31 ` Peter Xu
2017-10-23 11:19 ` Juan Quintela
2017-10-19 6:31 ` [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy Peter Xu
2017-10-19 6:32 ` [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps() Peter Xu
3 siblings, 1 reply; 9+ messages in thread
From: Peter Xu @ 2017-10-19 6:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Juan Quintela, Dr . David Alan Gilbert, peterx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy
2017-10-19 6:31 [Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init Peter Xu
2017-10-19 6:31 ` [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init() Peter Xu
2017-10-19 6:31 ` [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup Peter Xu
@ 2017-10-19 6:31 ` Peter Xu
2017-10-23 11:20 ` Juan Quintela
2017-10-19 6:32 ` [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps() Peter Xu
3 siblings, 1 reply; 9+ messages in thread
From: Peter Xu @ 2017-10-19 6:31 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Juan Quintela, Dr . David Alan Gilbert, peterx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps()
2017-10-19 6:31 [Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init Peter Xu
` (2 preceding siblings ...)
2017-10-19 6:31 ` [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy Peter Xu
@ 2017-10-19 6:32 ` Peter Xu
2017-10-23 11:38 ` Juan Quintela
3 siblings, 1 reply; 9+ messages in thread
From: Peter Xu @ 2017-10-19 6:32 UTC (permalink / raw)
To: qemu-devel; +Cc: Laurent Vivier, Juan Quintela, Dr . David Alan Gilbert, peterx
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init()
2017-10-19 6:31 ` [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init() Peter Xu
@ 2017-10-23 11:18 ` Juan Quintela
0 siblings, 0 replies; 9+ messages in thread
From: Juan Quintela @ 2017-10-23 11:18 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Laurent Vivier, Dr . David Alan Gilbert
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup
2017-10-19 6:31 ` [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup Peter Xu
@ 2017-10-23 11:19 ` Juan Quintela
0 siblings, 0 replies; 9+ messages in thread
From: Juan Quintela @ 2017-10-23 11:19 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Laurent Vivier, Dr . David Alan Gilbert
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy
2017-10-19 6:31 ` [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy Peter Xu
@ 2017-10-23 11:20 ` Juan Quintela
0 siblings, 0 replies; 9+ messages in thread
From: Juan Quintela @ 2017-10-23 11:20 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Laurent Vivier, Dr . David Alan Gilbert
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps()
2017-10-19 6:32 ` [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps() Peter Xu
@ 2017-10-23 11:38 ` Juan Quintela
0 siblings, 0 replies; 9+ messages in thread
From: Juan Quintela @ 2017-10-23 11:38 UTC (permalink / raw)
To: Peter Xu; +Cc: qemu-devel, Laurent Vivier, Dr . David Alan Gilbert
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-10-23 11:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-19 6:31 [Qemu-devel] [PATCH 0/4] migration: fixes and cleanups for ram state init Peter Xu
2017-10-19 6:31 ` [Qemu-devel] [PATCH 1/4] migration: provide ram_state_init() Peter Xu
2017-10-23 11:18 ` Juan Quintela
2017-10-19 6:31 ` [Qemu-devel] [PATCH 2/4] migration: provide ram_state_cleanup Peter Xu
2017-10-23 11:19 ` Juan Quintela
2017-10-19 6:31 ` [Qemu-devel] [PATCH 3/4] migration: clean up xbzrle cache init/destroy Peter Xu
2017-10-23 11:20 ` Juan Quintela
2017-10-19 6:32 ` [Qemu-devel] [PATCH 4/4] migration: new ram_init_bitmaps() Peter Xu
2017-10-23 11:38 ` Juan Quintela
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).