[PATCH v1 1/4] migration/multifd: Change multifd_load_cleanup() signature and usage

[PATCH v1 1/4] migration/multifd: Change multifd_load_cleanup() signature and usage

[Leonardo Bras]

Since it's introduction in commit f986c3d256 ("migration: Create multifd
migration threads"), multifd_load_cleanup() never returned any value
different than 0, neither set up any error on errp.

Even though, on process_incoming_migration_bh() an if clause uses it's
return value to decide on setting autostart = false, which will never
happen.

In order to simplify the codebase, change multifd_load_cleanup() signature
to 'void multifd_load_cleanup(void)', and for every usage remove error
handling or decision made based on return value != 0.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 migration/multifd.h   |  2 +-
 migration/migration.c | 14 ++++----------
 migration/multifd.c   |  6 ++----
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index ff3aa2e2e9..9a7e1a8826 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -16,7 +16,7 @@
 int multifd_save_setup(Error **errp);
 void multifd_save_cleanup(void);
 int multifd_load_setup(Error **errp);
-int multifd_load_cleanup(Error **errp);
+void multifd_load_cleanup(void);
 bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
diff --git a/migration/migration.c b/migration/migration.c
index 7a14aa98d8..ce962ea577 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -543,13 +543,7 @@ static void process_incoming_migration_bh(void *opaque)
      */
     qemu_announce_self(&mis->announce_timer, migrate_announce_params());
 
-    if (multifd_load_cleanup(&local_err) != 0) {
-        error_report_err(local_err);
-        autostart = false;
-    }
-    /* If global state section was not received or we are in running
-       state, we need to obey autostart. Any other state is set with
-       runstate_set. */
+    multifd_load_cleanup();
 
     dirty_bitmap_mig_before_vm_start();
 
@@ -649,9 +643,9 @@ fail:
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_FAILED);
     qemu_fclose(mis->from_src_file);
-    if (multifd_load_cleanup(&local_err) != 0) {
-        error_report_err(local_err);
-    }
+
+    multifd_load_cleanup();
+
     exit(EXIT_FAILURE);
 }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index b7ad7002e0..174726982c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1022,12 +1022,12 @@ static void multifd_recv_terminate_threads(Error *err)
     }
 }
 
-int multifd_load_cleanup(Error **errp)
+void multifd_load_cleanup(void)
 {
     int i;
 
     if (!migrate_use_multifd() || !migrate_multi_channels_is_allowed()) {
-        return 0;
+        return;
     }
     multifd_recv_terminate_threads(NULL);
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -1067,8 +1067,6 @@ int multifd_load_cleanup(Error **errp)
     multifd_recv_state->params = NULL;
     g_free(multifd_recv_state);
     multifd_recv_state = NULL;
-
-    return 0;
 }
 
 void multifd_recv_sync_main(void)
-- 
2.39.1

[PATCH v1 2/4] migration/multifd: Remove unnecessary assignment on multifd_load_cleanup()

[Leonardo Bras]

Before assigning "p->quit = true" for every multifd channel,
multifd_load_cleanup() will call multifd_recv_terminate_threads() which
already does the same assignment, while protected by a mutex.

So there is no point doing the same assignment again.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 migration/multifd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 174726982c..1a445b36f1 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1034,7 +1034,6 @@ void multifd_load_cleanup(void)
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
         if (p->running) {
-            p->quit = true;
             /*
              * multifd_recv_thread may hung at MULTIFD_FLAG_SYNC handle code,
              * however try to wakeup it without harm in cleanup phase.
-- 
2.39.1

Re: [PATCH v1 2/4] migration/multifd: Remove unnecessary assignment on multifd_load_cleanup()

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Before assigning "p->quit = true" for every multifd channel,
> multifd_load_cleanup() will call multifd_recv_terminate_threads() which
> already does the same assignment, while protected by a mutex.
>
> So there is no point doing the same assignment again.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

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

good catch

Re: [PATCH v1 2/4] migration/multifd: Remove unnecessary assignment on multifd_load_cleanup()

[Peter Xu]

On Fri, Feb 10, 2023 at 03:36:29AM -0300, Leonardo Bras wrote:
> Before assigning "p->quit = true" for every multifd channel,
> multifd_load_cleanup() will call multifd_recv_terminate_threads() which
> already does the same assignment, while protected by a mutex.
> 
> So there is no point doing the same assignment again.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 3/4] migration/multifd: Join all multifd threads in order to avoid leaks

[Leonardo Bras]

Current approach will only join threads that are still running.

For the threads not joined, resources or private memory are always kept in
the process space and never reclaimed before process end, and this risks
serious memory leaks.

This should usually not represent a big problem, since multifd migration
is usually just ran at most a few times, and after it succeeds there is
not much to be done before exiting the process.

Yet still, it should not hurt performance to join all of them.

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 migration/multifd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 1a445b36f1..7e37a459ed 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1039,8 +1039,9 @@ void multifd_load_cleanup(void)
              * however try to wakeup it without harm in cleanup phase.
              */
             qemu_sem_post(&p->sem_sync);
-            qemu_thread_join(&p->thread);
         }
+
+        qemu_thread_join(&p->thread);
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
-- 
2.39.1

Re: [PATCH v1 3/4] migration/multifd: Join all multifd threads in order to avoid leaks

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Current approach will only join threads that are still running.
>
> For the threads not joined, resources or private memory are always kept in
> the process space and never reclaimed before process end, and this risks
> serious memory leaks.
>
> This should usually not represent a big problem, since multifd migration
> is usually just ran at most a few times, and after it succeeds there is
> not much to be done before exiting the process.
>
> Yet still, it should not hurt performance to join all of them.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

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

Re: [PATCH v1 3/4] migration/multifd: Join all multifd threads in order to avoid leaks

[Peter Xu]

On Fri, Feb 10, 2023 at 03:36:30AM -0300, Leonardo Bras wrote:
> Current approach will only join threads that are still running.
> 
> For the threads not joined, resources or private memory are always kept in
> the process space and never reclaimed before process end, and this risks
> serious memory leaks.
> 
> This should usually not represent a big problem, since multifd migration
> is usually just ran at most a few times, and after it succeeds there is
> not much to be done before exiting the process.
> 
> Yet still, it should not hurt performance to join all of them.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 4/4] migration/multifd: Move load_cleanup inside incoming_state_destroy

[Leonardo Bras]

Currently running migration_incoming_state_destroy() without first running
multifd_load_cleanup() will cause a yank error:

qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance:
Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
(core dumped)

The above error happens in the target host, when multifd is being used
for precopy, and then postcopy is triggered and the migration finishes.
This will crash the VM in the target host.

To avoid that, move multifd_load_cleanup() inside
migration_incoming_state_destroy(), so that the load cleanup becomes part
of the incoming state destroying process.

Running multifd_load_cleanup() twice can become an issue, though, but the
only scenario it could be ran twice is on process_incoming_migration_bh().
So removing this extra call is necessary.

On the other hand, this multifd_load_cleanup() call happens way before the
migration_incoming_state_destroy() and having this happening before
dirty_bitmap_mig_before_vm_start() and vm_start() may be a need.

So introduce a new function multifd_load_shutdown() that will mainly stop
all multifd threads and close their QIOChannels. Then use this function
instead of multifd_load_cleanup() to make sure nothing else is received
before dirty_bitmap_mig_before_vm_start().

Signed-off-by: Leonardo Bras <leobras@redhat.com>
---
 migration/multifd.h   | 1 +
 migration/migration.c | 4 +++-
 migration/multifd.c   | 7 +++++++
 3 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 9a7e1a8826..7cfc265148 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -17,6 +17,7 @@ int multifd_save_setup(Error **errp);
 void multifd_save_cleanup(void);
 int multifd_load_setup(Error **errp);
 void multifd_load_cleanup(void);
+void multifd_load_shutdown(void);
 bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
diff --git a/migration/migration.c b/migration/migration.c
index ce962ea577..9f69447320 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -302,6 +302,8 @@ void migration_incoming_state_destroy(void)
 {
     struct MigrationIncomingState *mis = migration_incoming_get_current();
 
+    multifd_load_cleanup();
+
     if (mis->to_src_file) {
         /* Tell source that we are done */
         migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
@@ -543,7 +545,7 @@ static void process_incoming_migration_bh(void *opaque)
      */
     qemu_announce_self(&mis->announce_timer, migrate_announce_params());
 
-    multifd_load_cleanup();
+    multifd_load_shutdown();
 
     dirty_bitmap_mig_before_vm_start();
 
diff --git a/migration/multifd.c b/migration/multifd.c
index 7e37a459ed..9302c9f6cf 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1022,6 +1022,13 @@ static void multifd_recv_terminate_threads(Error *err)
     }
 }
 
+void multifd_load_shutdown(void)
+{
+    if (migrate_use_multifd() && migrate_multi_channels_is_allowed()) {
+        multifd_recv_terminate_threads(NULL);
+    }
+}
+
 void multifd_load_cleanup(void)
 {
     int i;
-- 
2.39.1

Re: [PATCH v1 4/4] migration/multifd: Move load_cleanup inside incoming_state_destroy

[Leonardo Brás]

On Fri, 2023-02-10 at 03:36 -0300, Leonardo Bras wrote:
> Currently running migration_incoming_state_destroy() without first running
> multifd_load_cleanup() will cause a yank error:
> 
> qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance:
> Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> (core dumped)
> 
> The above error happens in the target host, when multifd is being used
> for precopy, and then postcopy is triggered and the migration finishes.
> This will crash the VM in the target host.
> 
> To avoid that, move multifd_load_cleanup() inside
> migration_incoming_state_destroy(), so that the load cleanup becomes part
> of the incoming state destroying process.
> 
> Running multifd_load_cleanup() twice can become an issue, though, but the
> only scenario it could be ran twice is on process_incoming_migration_bh().
> So removing this extra call is necessary.
> 
> On the other hand, this multifd_load_cleanup() call happens way before the
> migration_incoming_state_destroy() and having this happening before
> dirty_bitmap_mig_before_vm_start() and vm_start() may be a need.
> 
> So introduce a new function multifd_load_shutdown() that will mainly stop
> all multifd threads and close their QIOChannels. Then use this function
> instead of multifd_load_cleanup() to make sure nothing else is received
> before dirty_bitmap_mig_before_vm_start().

Please add:

Fixes: b5eea99ec2 ("migration: Add yank feature")
Reported-by: Li Xiaohui <xiaohli@redhat.com>

> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>
> ---
>  migration/multifd.h   | 1 +
>  migration/migration.c | 4 +++-
>  migration/multifd.c   | 7 +++++++
>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 9a7e1a8826..7cfc265148 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -17,6 +17,7 @@ int multifd_save_setup(Error **errp);
>  void multifd_save_cleanup(void);
>  int multifd_load_setup(Error **errp);
>  void multifd_load_cleanup(void);
> +void multifd_load_shutdown(void);
>  bool multifd_recv_all_channels_created(void);
>  void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
>  void multifd_recv_sync_main(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index ce962ea577..9f69447320 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -302,6 +302,8 @@ void migration_incoming_state_destroy(void)
>  {
>      struct MigrationIncomingState *mis = migration_incoming_get_current();
>  
> +    multifd_load_cleanup();
> +
>      if (mis->to_src_file) {
>          /* Tell source that we are done */
>          migrate_send_rp_shut(mis, qemu_file_get_error(mis->from_src_file) != 0);
> @@ -543,7 +545,7 @@ static void process_incoming_migration_bh(void *opaque)
>       */
>      qemu_announce_self(&mis->announce_timer, migrate_announce_params());
>  
> -    multifd_load_cleanup();
> +    multifd_load_shutdown();
>  
>      dirty_bitmap_mig_before_vm_start();
>  
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 7e37a459ed..9302c9f6cf 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -1022,6 +1022,13 @@ static void multifd_recv_terminate_threads(Error *err)
>      }
>  }
>  
> +void multifd_load_shutdown(void)
> +{
> +    if (migrate_use_multifd() && migrate_multi_channels_is_allowed()) {
> +        multifd_recv_terminate_threads(NULL);
> +    }
> +}
> +
>  void multifd_load_cleanup(void)
>  {
>      int i;

Re: [PATCH v1 4/4] migration/multifd: Move load_cleanup inside incoming_state_destroy

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Currently running migration_incoming_state_destroy() without first running
> multifd_load_cleanup() will cause a yank error:
>
> qemu-system-x86_64: ../util/yank.c:107: yank_unregister_instance:
> Assertion `QLIST_EMPTY(&entry->yankfns)' failed.
> (core dumped)
>
> The above error happens in the target host, when multifd is being used
> for precopy, and then postcopy is triggered and the migration finishes.
> This will crash the VM in the target host.
>
> To avoid that, move multifd_load_cleanup() inside
> migration_incoming_state_destroy(), so that the load cleanup becomes part
> of the incoming state destroying process.
>
> Running multifd_load_cleanup() twice can become an issue, though, but the
> only scenario it could be ran twice is on process_incoming_migration_bh().
> So removing this extra call is necessary.
>
> On the other hand, this multifd_load_cleanup() call happens way before the
> migration_incoming_state_destroy() and having this happening before
> dirty_bitmap_mig_before_vm_start() and vm_start() may be a need.
>
> So introduce a new function multifd_load_shutdown() that will mainly stop
> all multifd threads and close their QIOChannels. Then use this function
> instead of multifd_load_cleanup() to make sure nothing else is received
> before dirty_bitmap_mig_before_vm_start().
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

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

Re: [PATCH v1 1/4] migration/multifd: Change multifd_load_cleanup() signature and usage

[Juan Quintela]

Leonardo Bras <leobras@redhat.com> wrote:
> Since it's introduction in commit f986c3d256 ("migration: Create multifd
> migration threads"), multifd_load_cleanup() never returned any value
> different than 0, neither set up any error on errp.
>
> Even though, on process_incoming_migration_bh() an if clause uses it's
> return value to decide on setting autostart = false, which will never
> happen.
>
> In order to simplify the codebase, change multifd_load_cleanup() signature
> to 'void multifd_load_cleanup(void)', and for every usage remove error
> handling or decision made based on return value != 0.
>
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

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

Re: [PATCH v1 1/4] migration/multifd: Change multifd_load_cleanup() signature and usage

[Peter Xu]

Leo,

Please still provide a cover letter as long as >1 patches will be posted as
a set.

Not only because it still always help to provide an overview for reviewers
before reading each of them (e.g. I have a habit of prioritizing reviews
based on cover letters first), but also when you're confident enough the
reviewer(s) can ACK the patches in one reply. :-)

On Fri, Feb 10, 2023 at 03:36:28AM -0300, Leonardo Bras wrote:
> Since it's introduction in commit f986c3d256 ("migration: Create multifd
> migration threads"), multifd_load_cleanup() never returned any value
> different than 0, neither set up any error on errp.
> 
> Even though, on process_incoming_migration_bh() an if clause uses it's
> return value to decide on setting autostart = false, which will never
> happen.
> 
> In order to simplify the codebase, change multifd_load_cleanup() signature
> to 'void multifd_load_cleanup(void)', and for every usage remove error
> handling or decision made based on return value != 0.
> 
> Signed-off-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

[PATCH v1 1/4] migration/multifd: Change multifd_load_cleanup() signature and usage

[Leonardo Bras]

Since it's introduction in commit f986c3d256 ("migration: Create multifd
migration threads"), multifd_load_cleanup() never returned any value
different than 0, neither set up any error on errp.

Even though, on process_incoming_migration_bh() an if clause uses it's
return value to decide on setting autostart = false, which will never
happen.

In order to simplify the codebase, change multifd_load_cleanup() signature
to 'void multifd_load_cleanup(void)', and for every usage remove error
handling or decision made based on return value != 0.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 migration/multifd.h   |  2 +-
 migration/migration.c | 14 ++++----------
 migration/multifd.c   |  6 ++----
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index ff3aa2e2e9..9a7e1a8826 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -16,7 +16,7 @@
 int multifd_save_setup(Error **errp);
 void multifd_save_cleanup(void);
 int multifd_load_setup(Error **errp);
-int multifd_load_cleanup(Error **errp);
+void multifd_load_cleanup(void);
 bool multifd_recv_all_channels_created(void);
 void multifd_recv_new_channel(QIOChannel *ioc, Error **errp);
 void multifd_recv_sync_main(void);
diff --git a/migration/migration.c b/migration/migration.c
index 7a14aa98d8..ce962ea577 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -543,13 +543,7 @@ static void process_incoming_migration_bh(void *opaque)
      */
     qemu_announce_self(&mis->announce_timer, migrate_announce_params());
 
-    if (multifd_load_cleanup(&local_err) != 0) {
-        error_report_err(local_err);
-        autostart = false;
-    }
-    /* If global state section was not received or we are in running
-       state, we need to obey autostart. Any other state is set with
-       runstate_set. */
+    multifd_load_cleanup();
 
     dirty_bitmap_mig_before_vm_start();
 
@@ -649,9 +643,9 @@ fail:
     migrate_set_state(&mis->state, MIGRATION_STATUS_ACTIVE,
                       MIGRATION_STATUS_FAILED);
     qemu_fclose(mis->from_src_file);
-    if (multifd_load_cleanup(&local_err) != 0) {
-        error_report_err(local_err);
-    }
+
+    multifd_load_cleanup();
+
     exit(EXIT_FAILURE);
 }
 
diff --git a/migration/multifd.c b/migration/multifd.c
index b7ad7002e0..174726982c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1022,12 +1022,12 @@ static void multifd_recv_terminate_threads(Error *err)
     }
 }
 
-int multifd_load_cleanup(Error **errp)
+void multifd_load_cleanup(void)
 {
     int i;
 
     if (!migrate_use_multifd() || !migrate_multi_channels_is_allowed()) {
-        return 0;
+        return;
     }
     multifd_recv_terminate_threads(NULL);
     for (i = 0; i < migrate_multifd_channels(); i++) {
@@ -1067,8 +1067,6 @@ int multifd_load_cleanup(Error **errp)
     multifd_recv_state->params = NULL;
     g_free(multifd_recv_state);
     multifd_recv_state = NULL;
-
-    return 0;
 }
 
 void multifd_recv_sync_main(void)
-- 
2.39.1

[PATCH v1 3/4] migration/multifd: Join all multifd threads in order to avoid leaks

[Leonardo Bras]

Current approach will only join threads that are still running.

For the threads not joined, resources or private memory are always kept in
the process space and never reclaimed before process end, and this risks
serious memory leaks.

This should usually not represent a big problem, since multifd migration
is usually just ran at most a few times, and after it succeeds there is
not much to be done before exiting the process.

Yet still, it should not hurt performance to join all of them.

Signed-off-by: Leonardo Bras <leobras.c@gmail.com>
---
 migration/multifd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 1a445b36f1..7e37a459ed 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -1039,8 +1039,9 @@ void multifd_load_cleanup(void)
              * however try to wakeup it without harm in cleanup phase.
              */
             qemu_sem_post(&p->sem_sync);
-            qemu_thread_join(&p->thread);
         }
+
+        qemu_thread_join(&p->thread);
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
-- 
2.39.1