Re: [Qemu-devel] [PATCH 1/5] ram: Call migration_page_queue_free() at ram_migration_cleanup()
On Fri, Jun 02, 2017 at 12:08:09AM +0200, Juan Quintela wrote:
> We shouldn't be using memory later than that.
>
> Signed-off-by: Juan Quintela
Reviewed-by: Peter Xu
> ---
> migration/migration.c | 2 --
> migration/ram.c | 5 +++--
> migration/ram.h | 1 -
> 3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index af4c2cc..ea3d41c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -814,8 +814,6 @@ static void migrate_fd_cleanup(void *opaque)
> qemu_bh_delete(s->cleanup_bh);
> s->cleanup_bh = NULL;
>
> -migration_page_queue_free();
> -
> if (s->to_dst_file) {
> trace_migrate_fd_cleanup();
> qemu_mutex_unlock_iothread();
> diff --git a/migration/ram.c b/migration/ram.c
> index db7f4b0..e503277 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1181,10 +1181,9 @@ static bool get_queued_page(RAMState *rs,
> PageSearchStatus *pss)
> * be some left. in case that there is any page left, we drop it.
> *
> */
> -void migration_page_queue_free(void)
> +static void migration_page_queue_free(RAMState *rs)
> {
> struct RAMSrcPageRequest *mspr, *next_mspr;
> -RAMState *rs = &ram_state;
> /* This queue generally should be empty - but in the case of a failed
> * migration might have some droppings in.
> */
> @@ -1434,6 +1433,7 @@ void free_xbzrle_decoded_buf(void)
>
> static void ram_migration_cleanup(void *opaque)
> {
> +RAMState *rs = opaque;
> RAMBlock *block;
>
> /* caller have hold iothread lock or is in a bh, so there is
> @@ -1459,6 +1459,7 @@ static void ram_migration_cleanup(void *opaque)
> XBZRLE.current_buf = NULL;
> }
> XBZRLE_cache_unlock();
> +migration_page_queue_free(rs);
> }
>
> static void ram_state_reset(RAMState *rs)
> diff --git a/migration/ram.h b/migration/ram.h
> index c9563d1..d4da419 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -53,7 +53,6 @@ void migrate_decompress_threads_create(void);
> void migrate_decompress_threads_join(void);
>
> uint64_t ram_pagesize_summary(void);
> -void migration_page_queue_free(void);
> int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t
> len);
> void acct_update_position(QEMUFile *f, size_t size, bool zero);
> void free_xbzrle_decoded_buf(void);
> --
> 2.9.4
>
--
Peter Xu
Re: [Qemu-devel] [PATCH 1/5] ram: Call migration_page_queue_free() at ram_migration_cleanup()
* Juan Quintela (quint...@redhat.com) wrote:
> We shouldn't be using memory later than that.
>
> Signed-off-by: Juan Quintela
Yes, I think I agree:
migration_completion calls await_return_path_close_on_source that
makes sure there's no more incoming requests.
migration_completion is called prior to the call to qemu_savevm_state_cleanup
qemu_savevm_state_cleanup calls the ram_migration_cleanup.
So you're moving it later which should be safe:
Reviewed-by: Dr. David Alan Gilbert
> ---
> migration/migration.c | 2 --
> migration/ram.c | 5 +++--
> migration/ram.h | 1 -
> 3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index af4c2cc..ea3d41c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -814,8 +814,6 @@ static void migrate_fd_cleanup(void *opaque)
> qemu_bh_delete(s->cleanup_bh);
> s->cleanup_bh = NULL;
>
> -migration_page_queue_free();
> -
> if (s->to_dst_file) {
> trace_migrate_fd_cleanup();
> qemu_mutex_unlock_iothread();
> diff --git a/migration/ram.c b/migration/ram.c
> index db7f4b0..e503277 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -1181,10 +1181,9 @@ static bool get_queued_page(RAMState *rs,
> PageSearchStatus *pss)
> * be some left. in case that there is any page left, we drop it.
> *
> */
> -void migration_page_queue_free(void)
> +static void migration_page_queue_free(RAMState *rs)
> {
> struct RAMSrcPageRequest *mspr, *next_mspr;
> -RAMState *rs = &ram_state;
> /* This queue generally should be empty - but in the case of a failed
> * migration might have some droppings in.
> */
> @@ -1434,6 +1433,7 @@ void free_xbzrle_decoded_buf(void)
>
> static void ram_migration_cleanup(void *opaque)
> {
> +RAMState *rs = opaque;
> RAMBlock *block;
>
> /* caller have hold iothread lock or is in a bh, so there is
> @@ -1459,6 +1459,7 @@ static void ram_migration_cleanup(void *opaque)
> XBZRLE.current_buf = NULL;
> }
> XBZRLE_cache_unlock();
> +migration_page_queue_free(rs);
> }
>
> static void ram_state_reset(RAMState *rs)
> diff --git a/migration/ram.h b/migration/ram.h
> index c9563d1..d4da419 100644
> --- a/migration/ram.h
> +++ b/migration/ram.h
> @@ -53,7 +53,6 @@ void migrate_decompress_threads_create(void);
> void migrate_decompress_threads_join(void);
>
> uint64_t ram_pagesize_summary(void);
> -void migration_page_queue_free(void);
> int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t
> len);
> void acct_update_position(QEMUFile *f, size_t size, bool zero);
> void free_xbzrle_decoded_buf(void);
> --
> 2.9.4
>
--
Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
[Qemu-devel] [PATCH 1/5] ram: Call migration_page_queue_free() at ram_migration_cleanup()
We shouldn't be using memory later than that.
Signed-off-by: Juan Quintela
---
migration/migration.c | 2 --
migration/ram.c | 5 +++--
migration/ram.h | 1 -
3 files changed, 3 insertions(+), 5 deletions(-)
diff --git a/migration/migration.c b/migration/migration.c
index af4c2cc..ea3d41c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -814,8 +814,6 @@ static void migrate_fd_cleanup(void *opaque)
qemu_bh_delete(s->cleanup_bh);
s->cleanup_bh = NULL;
-migration_page_queue_free();
-
if (s->to_dst_file) {
trace_migrate_fd_cleanup();
qemu_mutex_unlock_iothread();
diff --git a/migration/ram.c b/migration/ram.c
index db7f4b0..e503277 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -1181,10 +1181,9 @@ static bool get_queued_page(RAMState *rs,
PageSearchStatus *pss)
* be some left. in case that there is any page left, we drop it.
*
*/
-void migration_page_queue_free(void)
+static void migration_page_queue_free(RAMState *rs)
{
struct RAMSrcPageRequest *mspr, *next_mspr;
-RAMState *rs = &ram_state;
/* This queue generally should be empty - but in the case of a failed
* migration might have some droppings in.
*/
@@ -1434,6 +1433,7 @@ void free_xbzrle_decoded_buf(void)
static void ram_migration_cleanup(void *opaque)
{
+RAMState *rs = opaque;
RAMBlock *block;
/* caller have hold iothread lock or is in a bh, so there is
@@ -1459,6 +1459,7 @@ static void ram_migration_cleanup(void *opaque)
XBZRLE.current_buf = NULL;
}
XBZRLE_cache_unlock();
+migration_page_queue_free(rs);
}
static void ram_state_reset(RAMState *rs)
diff --git a/migration/ram.h b/migration/ram.h
index c9563d1..d4da419 100644
--- a/migration/ram.h
+++ b/migration/ram.h
@@ -53,7 +53,6 @@ void migrate_decompress_threads_create(void);
void migrate_decompress_threads_join(void);
uint64_t ram_pagesize_summary(void);
-void migration_page_queue_free(void);
int ram_save_queue_pages(const char *rbname, ram_addr_t start, ram_addr_t len);
void acct_update_position(QEMUFile *f, size_t size, bool zero);
void free_xbzrle_decoded_buf(void);
--
2.9.4
