[PATCH 0/5] migration/multifd: Fix channel creation vs. cleanup races

[PATCH 0/5] migration/multifd: Fix channel creation vs. cleanup races

[Fabiano Rosas]

Hi,

This contains 2 patches from my previous series addressing the
p->running misuse and the TLS thread leak and 3 new patches to fix the
cleanup-while-creating-threads race.

For the p->running I'm keeping the idea from the other series to
remove p->running and use a more narrow p->thread_created flag. This
flag is used only inform whether the thread has been created so we can
join it.

For the cleanup race I have moved some code around and added a
semaphore to make multifd_save_setup() only return once all channel
creation tasks have started.

The idea is that after multifd_save_setup() returns, no new creations
are in flight and the p->thread_created flags will never change again,
so they're enough to cause the cleanup code to wait for the threads to
join.

CI run: https://gitlab.com/farosas/qemu/-/pipelines/1162798843

@Peter: I can rebase this on top of your series once we decide about
it.

Fabiano Rosas (5):
  migration/multifd: Join the TLS thread
  migration/multifd: Remove p->running
  migration/multifd: Move multifd_save_setup error handling in to the
    function
  migration/multifd: Move multifd_save_setup into migration thread
  migration/multifd: Add a synchronization point for channel creation

 migration/migration.c |  14 ++---
 migration/multifd.c   | 129 ++++++++++++++++++++++++------------------
 migration/multifd.h   |  11 ++--
 3 files changed, 83 insertions(+), 71 deletions(-)

-- 
2.35.3

[PATCH 1/5] migration/multifd: Join the TLS thread

[Fabiano Rosas]

We're currently leaking the resources of the TLS thread by not joining
it and also overwriting the p->thread pointer altogether.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 8 +++++++-
 migration/multifd.h | 2 ++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 25cbc6dc6b..b557d046a9 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -524,6 +524,10 @@ void multifd_save_cleanup(void)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
+        if (p->tls_thread_created) {
+            qemu_thread_join(&p->tls_thread);
+        }
+
         if (p->running) {
             qemu_thread_join(&p->thread);
         }
@@ -827,7 +831,9 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
     qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
     p->c = QIO_CHANNEL(tioc);
-    qemu_thread_create(&p->thread, "multifd-tls-handshake-worker",
+
+    p->tls_thread_created = true;
+    qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
                        multifd_tls_handshake_thread, p,
                        QEMU_THREAD_JOINABLE);
     return true;
diff --git a/migration/multifd.h b/migration/multifd.h
index 35d11f103c..5a69ef40e2 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -73,6 +73,8 @@ typedef struct {
     char *name;
     /* channel thread id */
     QemuThread thread;
+    QemuThread tls_thread;
+    bool tls_thread_created;
     /* communication channel */
     QIOChannel *c;
     /* is the yank function registered */
-- 
2.35.3

Re: [PATCH 1/5] migration/multifd: Join the TLS thread

[Peter Xu]

On Fri, Feb 02, 2024 at 04:11:24PM -0300, Fabiano Rosas wrote:
> We're currently leaking the resources of the TLS thread by not joining
> it and also overwriting the p->thread pointer altogether.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

Does this deserve below?

Fixes: a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to blocking handshake")
Cc: qemu-stable <qemu-stable@nongnu.org>

-- 
Peter Xu

[PATCH 2/5] migration/multifd: Remove p->running

[Fabiano Rosas]

We currently only need p->running to avoid calling qemu_thread_join()
on a non existent thread if the thread has never been created.

However, there are at least two bugs in this logic:

1) On the sending side, p->running is set too early and
qemu_thread_create() can be skipped due to an error during TLS
handshake, leaving the flag set and leading to a crash when
multifd_save_cleanup() calls qemu_thread_join().

2) During exit, the multifd thread clears the flag while holding the
channel lock. The counterpart at multifd_save_cleanup() reads the flag
outside of the lock and might free the mutex while the multifd thread
still has it locked.

Fix the first issue by setting the flag right before creating the
thread. Rename it from p->running to p->thread_created to clarify its
usage.

Fix the second issue by not clearing the flag at the multifd thread
exit. We don't have any use for that.

Note that these bugs are straight-forward logic issues and not race
conditions. There is still a gap for races to affect this code due to
multifd_save_cleanup() being allowed to run concurrently with the
thread creation loop. This issue is solved in the next patch.

Fixes: 29647140157a ("migration/tls: add support for multifd tls-handshake")
Reported-by: Avihai Horon <avihaih@nvidia.com>
Reported-by: <chenyuhui5@huawei.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 30 ++++++++++++------------------
 migration/multifd.h |  7 ++-----
 2 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index b557d046a9..402b7fd776 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -528,7 +528,7 @@ void multifd_save_cleanup(void)
             qemu_thread_join(&p->tls_thread);
         }
 
-        if (p->running) {
+        if (p->thread_created) {
             qemu_thread_join(&p->thread);
         }
     }
@@ -759,10 +759,6 @@ out:
         error_free(local_err);
     }
 
-    qemu_mutex_lock(&p->mutex);
-    p->running = false;
-    qemu_mutex_unlock(&p->mutex);
-
     rcu_unregister_thread();
     migration_threads_remove(thread);
     trace_multifd_send_thread_end(p->id, p->num_packets, p->total_normal_pages);
@@ -859,6 +855,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
     migration_ioc_register_yank(ioc);
     p->registered_yank = true;
     p->c = ioc;
+
+    p->thread_created = true;
     qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
                        QEMU_THREAD_JOINABLE);
     return true;
@@ -890,7 +888,6 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
     trace_multifd_new_send_channel_async(p->id);
     if (!qio_task_propagate_error(task, &local_err)) {
         qio_channel_set_delay(ioc, false);
-        p->running = true;
         if (multifd_channel_connect(p, ioc, &local_err)) {
             return;
         }
@@ -1030,15 +1027,15 @@ void multifd_load_cleanup(void)
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
 
-        if (p->running) {
-            /*
-             * multifd_recv_thread may hung at MULTIFD_FLAG_SYNC handle code,
-             * however try to wakeup it without harm in cleanup phase.
-             */
-            qemu_sem_post(&p->sem_sync);
-        }
+        /*
+         * multifd_recv_thread may hung at MULTIFD_FLAG_SYNC handle code,
+         * however try to wakeup it without harm in cleanup phase.
+         */
+        qemu_sem_post(&p->sem_sync);
 
-        qemu_thread_join(&p->thread);
+        if (p->thread_created) {
+            qemu_thread_join(&p->thread);
+        }
     }
     for (i = 0; i < migrate_multifd_channels(); i++) {
         MultiFDRecvParams *p = &multifd_recv_state->params[i];
@@ -1148,9 +1145,6 @@ static void *multifd_recv_thread(void *opaque)
         multifd_recv_terminate_threads(local_err);
         error_free(local_err);
     }
-    qemu_mutex_lock(&p->mutex);
-    p->running = false;
-    qemu_mutex_unlock(&p->mutex);
 
     rcu_unregister_thread();
     trace_multifd_recv_thread_end(p->id, p->num_packets, p->total_normal_pages);
@@ -1258,7 +1252,7 @@ void multifd_recv_new_channel(QIOChannel *ioc, Error **errp)
     /* initial packet */
     p->num_packets = 1;
 
-    p->running = true;
+    p->thread_created = true;
     qemu_thread_create(&p->thread, p->name, multifd_recv_thread, p,
                        QEMU_THREAD_JOINABLE);
     qatomic_inc(&multifd_recv_state->count);
diff --git a/migration/multifd.h b/migration/multifd.h
index 5a69ef40e2..917833c309 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -73,6 +73,7 @@ typedef struct {
     char *name;
     /* channel thread id */
     QemuThread thread;
+    bool thread_created;
     QemuThread tls_thread;
     bool tls_thread_created;
     /* communication channel */
@@ -95,8 +96,6 @@ typedef struct {
 
     /* this mutex protects the following parameters */
     QemuMutex mutex;
-    /* is this channel thread running */
-    bool running;
     /* should this thread finish */
     bool quit;
     /* multifd flags for each packet */
@@ -144,6 +143,7 @@ typedef struct {
     char *name;
     /* channel thread id */
     QemuThread thread;
+    bool thread_created;
     /* communication channel */
     QIOChannel *c;
     /* packet allocated len */
@@ -158,8 +158,6 @@ typedef struct {
 
     /* this mutex protects the following parameters */
     QemuMutex mutex;
-    /* is this channel thread running */
-    bool running;
     /* should this thread finish */
     bool quit;
     /* multifd flags for each packet */
@@ -209,4 +207,3 @@ typedef struct {
 void multifd_register_ops(int method, MultiFDMethods *ops);
 
 #endif
-
-- 
2.35.3

Re: [PATCH 2/5] migration/multifd: Remove p->running

[Peter Xu]

On Fri, Feb 02, 2024 at 04:11:25PM -0300, Fabiano Rosas wrote:
> We currently only need p->running to avoid calling qemu_thread_join()
> on a non existent thread if the thread has never been created.
> 
> However, there are at least two bugs in this logic:
> 
> 1) On the sending side, p->running is set too early and
> qemu_thread_create() can be skipped due to an error during TLS
> handshake, leaving the flag set and leading to a crash when
> multifd_save_cleanup() calls qemu_thread_join().
> 
> 2) During exit, the multifd thread clears the flag while holding the
> channel lock. The counterpart at multifd_save_cleanup() reads the flag
> outside of the lock and might free the mutex while the multifd thread
> still has it locked.
> 
> Fix the first issue by setting the flag right before creating the
> thread. Rename it from p->running to p->thread_created to clarify its
> usage.
> 
> Fix the second issue by not clearing the flag at the multifd thread
> exit. We don't have any use for that.
> 
> Note that these bugs are straight-forward logic issues and not race
> conditions. There is still a gap for races to affect this code due to
> multifd_save_cleanup() being allowed to run concurrently with the
> thread creation loop. This issue is solved in the next patch.
> 

Cc: qemu-stable <qemu-stable@nongnu.org>

> Fixes: 29647140157a ("migration/tls: add support for multifd tls-handshake")
> Reported-by: Avihai Horon <avihaih@nvidia.com>
> Reported-by: <chenyuhui5@huawei.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

[PATCH 3/5] migration/multifd: Move multifd_save_setup error handling in to the function

[Fabiano Rosas]

Hide the error handling inside multifd_save_setup to make it cleaner
for the next patch to move the function around.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c |  6 +-----
 migration/multifd.c   | 24 +++++++++++++++++-------
 migration/multifd.h   |  2 +-
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index d5f705ceef..55abb175cc 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3623,11 +3623,7 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         return;
     }
 
-    if (multifd_save_setup(&local_err) != 0) {
-        migrate_set_error(s, local_err);
-        error_report_err(local_err);
-        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
-                          MIGRATION_STATUS_FAILED);
+    if (!multifd_save_setup()) {
         migrate_fd_cleanup(s);
         return;
     }
diff --git a/migration/multifd.c b/migration/multifd.c
index 402b7fd776..1851206352 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -902,14 +902,16 @@ static void multifd_new_send_channel_create(gpointer opaque)
     socket_send_channel_create(multifd_new_send_channel_async, opaque);
 }
 
-int multifd_save_setup(Error **errp)
+bool multifd_save_setup(void)
 {
-    int thread_count;
+    MigrationState *s = migrate_get_current();
+    Error *local_err = NULL;
+    int thread_count, ret = 0;
     uint32_t page_count = MULTIFD_PACKET_SIZE / qemu_target_page_size();
     uint8_t i;
 
     if (!migrate_multifd()) {
-        return 0;
+        return true;
     }
 
     thread_count = migrate_multifd_channels();
@@ -953,14 +955,22 @@ int multifd_save_setup(Error **errp)
 
     for (i = 0; i < thread_count; i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
-        int ret;
 
-        ret = multifd_send_state->ops->send_setup(p, errp);
+        ret = multifd_send_state->ops->send_setup(p, &local_err);
         if (ret) {
-            return ret;
+            break;
         }
     }
-    return 0;
+
+    if (ret) {
+        migrate_set_error(s, local_err);
+        error_report_err(local_err);
+        migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
+                          MIGRATION_STATUS_FAILED);
+        return false;
+    }
+
+    return true;
 }
 
 struct {
diff --git a/migration/multifd.h b/migration/multifd.h
index 917833c309..4acbd65e91 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -13,7 +13,7 @@
 #ifndef QEMU_MIGRATION_MULTIFD_H
 #define QEMU_MIGRATION_MULTIFD_H
 
-int multifd_save_setup(Error **errp);
+bool multifd_save_setup(void);
 void multifd_save_cleanup(void);
 int multifd_load_setup(Error **errp);
 void multifd_load_cleanup(void);
-- 
2.35.3

Re: [PATCH 3/5] migration/multifd: Move multifd_save_setup error handling in to the function

[Peter Xu]

On Fri, Feb 02, 2024 at 04:11:26PM -0300, Fabiano Rosas wrote:
> Hide the error handling inside multifd_save_setup to make it cleaner
> for the next patch to move the function around.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

[PATCH 4/5] migration/multifd: Move multifd_save_setup into migration thread

[Fabiano Rosas]

We currently have an unfavorable situation around multifd channels
creation and the migration thread execution.

We create the multifd channels with qio_channel_socket_connect_async
-> qio_task_run_in_thread, but only connect them at the
multifd_new_send_channel_async callback, called from
qio_task_complete, which is registered as a glib event.

So at multifd_save_setup() we create the channels, but they will only
be actually usable after the whole multifd_save_setup() calling stack
returns back to the main loop. Which means that the migration thread
is already up and running without any possibility for the multifd
channels to be ready on time.

We currently rely on the channels-ready semaphore blocking
multifd_send_sync_main() until channels start to come up and release
it. However there have been bugs recently found when a channel's
creation fails and multifd_save_cleanup() is allowed to run while
other channels are still being created.

Let's start to organize this situation by moving the
multifd_save_setup() call into the migration thread. That way we
unblock the main-loop to dispatch the completion callbacks and
actually have a chance of getting the multifd channels ready for when
the migration thread needs them.

The next patches will deal with the synchronization aspects.

Note that this takes multifd_save_setup() out of the BQL.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 55abb175cc..c14d12497f 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -3315,6 +3315,10 @@ static void *migration_thread(void *opaque)
     object_ref(OBJECT(s));
     update_iteration_initial_status(s);
 
+    if (!multifd_save_setup()) {
+        goto out;
+    }
+
     bql_lock();
     qemu_savevm_state_header(s->to_dst_file);
     bql_unlock();
@@ -3386,6 +3390,7 @@ static void *migration_thread(void *opaque)
         urgent = migration_rate_limit();
     }
 
+out:
     trace_migration_thread_after_loop();
     migration_iteration_finish(s);
     object_unref(OBJECT(s));
@@ -3623,11 +3628,6 @@ void migrate_fd_connect(MigrationState *s, Error *error_in)
         return;
     }
 
-    if (!multifd_save_setup()) {
-        migrate_fd_cleanup(s);
-        return;
-    }
-
     if (migrate_background_snapshot()) {
         qemu_thread_create(&s->thread, "bg_snapshot",
                 bg_migration_thread, s, QEMU_THREAD_JOINABLE);
-- 
2.35.3

Re: [PATCH 4/5] migration/multifd: Move multifd_save_setup into migration thread

[Peter Xu]

On Fri, Feb 02, 2024 at 04:11:27PM -0300, Fabiano Rosas wrote:
> We currently have an unfavorable situation around multifd channels
> creation and the migration thread execution.
> 
> We create the multifd channels with qio_channel_socket_connect_async
> -> qio_task_run_in_thread, but only connect them at the
> multifd_new_send_channel_async callback, called from
> qio_task_complete, which is registered as a glib event.
> 
> So at multifd_save_setup() we create the channels, but they will only
> be actually usable after the whole multifd_save_setup() calling stack
> returns back to the main loop. Which means that the migration thread
> is already up and running without any possibility for the multifd
> channels to be ready on time.
> 
> We currently rely on the channels-ready semaphore blocking
> multifd_send_sync_main() until channels start to come up and release
> it. However there have been bugs recently found when a channel's
> creation fails and multifd_save_cleanup() is allowed to run while
> other channels are still being created.
> 
> Let's start to organize this situation by moving the
> multifd_save_setup() call into the migration thread. That way we
> unblock the main-loop to dispatch the completion callbacks and
> actually have a chance of getting the multifd channels ready for when
> the migration thread needs them.
> 
> The next patches will deal with the synchronization aspects.
> 
> Note that this takes multifd_save_setup() out of the BQL.
> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

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

-- 
Peter Xu

[PATCH 5/5] migration/multifd: Add a synchronization point for channel creation

[Fabiano Rosas]

It is possible that one of the multifd channels fails to be created at
multifd_new_send_channel_async() while the rest of the channel
creation tasks are still in flight.

This could lead to multifd_save_cleanup() executing the
qemu_thread_join() loop too early and not waiting for the threads
which haven't been created yet, leading to the freeing of resources
that the newly created threads will try to access and crash.

Add a synchronization point after which there will be no attempts at
thread creation and therefore calling multifd_save_cleanup() past that
point will ensure it properly waits for the threads.

A note about performance: Prior to this patch, if a channel took too
long to be established, other channels could finish connecting first
and already start taking load. Now we're bounded by the
slowest-connecting channel.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/multifd.c | 67 +++++++++++++++++++++++++--------------------
 1 file changed, 37 insertions(+), 30 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 1851206352..888ac8b05d 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -360,6 +360,11 @@ struct {
     MultiFDPages_t *pages;
     /* global number of generated multifd packets */
     uint64_t packet_num;
+    /*
+     * Synchronization point past which no more channels will be
+     * created.
+     */
+    QemuSemaphore channels_created;
     /* send channels ready */
     QemuSemaphore channels_ready;
     /*
@@ -561,6 +566,7 @@ void multifd_save_cleanup(void)
             error_free(local_err);
         }
     }
+    qemu_sem_destroy(&multifd_send_state->channels_created);
     qemu_sem_destroy(&multifd_send_state->channels_ready);
     g_free(multifd_send_state->params);
     multifd_send_state->params = NULL;
@@ -787,13 +793,6 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
     trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
 
     migrate_set_error(migrate_get_current(), err);
-    /*
-     * Error happen, mark multifd_send_thread status as 'quit' although it
-     * is not created, and then tell who pay attention to me.
-     */
-    p->quit = true;
-    qemu_sem_post(&multifd_send_state->channels_ready);
-    qemu_sem_post(&p->sem_sync);
     error_free(err);
 }
 
@@ -862,39 +861,37 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
     return true;
 }
 
-static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
-                                             QIOChannel *ioc, Error *err)
-{
-     migrate_set_error(migrate_get_current(), err);
-     /* Error happen, we need to tell who pay attention to me */
-     qemu_sem_post(&multifd_send_state->channels_ready);
-     qemu_sem_post(&p->sem_sync);
-     /*
-      * Although multifd_send_thread is not created, but main migration
-      * thread need to judge whether it is running, so we need to mark
-      * its status.
-      */
-     p->quit = true;
-     object_unref(OBJECT(ioc));
-     error_free(err);
-}
-
 static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
 {
     MultiFDSendParams *p = opaque;
     QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
     Error *local_err = NULL;
+    bool ret;
 
     trace_multifd_new_send_channel_async(p->id);
-    if (!qio_task_propagate_error(task, &local_err)) {
-        qio_channel_set_delay(ioc, false);
-        if (multifd_channel_connect(p, ioc, &local_err)) {
-            return;
-        }
+
+    if (qio_task_propagate_error(task, &local_err)) {
+        ret = false;
+        goto out;
+    }
+
+    qio_channel_set_delay(ioc, false);
+    ret = multifd_channel_connect(p, ioc, &local_err);
+
+out:
+    /*
+     * Here we're not interested whether creation succeeded, only that
+     * it happened at all.
+     */
+    qemu_sem_post(&multifd_send_state->channels_created);
+    if (ret) {
+        return;
     }
 
     trace_multifd_new_send_channel_async_error(p->id, local_err);
-    multifd_new_send_channel_cleanup(p, ioc, local_err);
+    migrate_set_error(migrate_get_current(), local_err);
+    object_unref(OBJECT(ioc));
+    error_free(local_err);
 }
 
 static void multifd_new_send_channel_create(gpointer opaque)
@@ -918,6 +915,7 @@ bool multifd_save_setup(void)
     multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
     multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
     multifd_send_state->pages = multifd_pages_init(page_count);
+    qemu_sem_init(&multifd_send_state->channels_created, 0);
     qemu_sem_init(&multifd_send_state->channels_ready, 0);
     qatomic_set(&multifd_send_state->exiting, 0);
     multifd_send_state->ops = multifd_ops[migrate_multifd_compression()];
@@ -953,6 +951,15 @@ bool multifd_save_setup(void)
         multifd_new_send_channel_create(p);
     }
 
+    /*
+     * Wait until channel creation has started for all channels. The
+     * creation can still fail, but no more channels will be created
+     * past this point.
+     */
+    for (i = 0; i < thread_count; i++) {
+        qemu_sem_wait(&multifd_send_state->channels_created);
+    }
+
     for (i = 0; i < thread_count; i++) {
         MultiFDSendParams *p = &multifd_send_state->params[i];
 
-- 
2.35.3

Re: [PATCH 5/5] migration/multifd: Add a synchronization point for channel creation

[Peter Xu]

On Fri, Feb 02, 2024 at 04:11:28PM -0300, Fabiano Rosas wrote:
> It is possible that one of the multifd channels fails to be created at
> multifd_new_send_channel_async() while the rest of the channel
> creation tasks are still in flight.
> 
> This could lead to multifd_save_cleanup() executing the
> qemu_thread_join() loop too early and not waiting for the threads
> which haven't been created yet, leading to the freeing of resources
> that the newly created threads will try to access and crash.
> 
> Add a synchronization point after which there will be no attempts at
> thread creation and therefore calling multifd_save_cleanup() past that
> point will ensure it properly waits for the threads.
> 
> A note about performance: Prior to this patch, if a channel took too
> long to be established, other channels could finish connecting first
> and already start taking load. Now we're bounded by the
> slowest-connecting channel.

Yes, I think this should (hopefully!) be fine.

> 
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
>  migration/multifd.c | 67 +++++++++++++++++++++++++--------------------
>  1 file changed, 37 insertions(+), 30 deletions(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 1851206352..888ac8b05d 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -360,6 +360,11 @@ struct {
>      MultiFDPages_t *pages;
>      /* global number of generated multifd packets */
>      uint64_t packet_num;
> +    /*
> +     * Synchronization point past which no more channels will be
> +     * created.
> +     */
> +    QemuSemaphore channels_created;
>      /* send channels ready */
>      QemuSemaphore channels_ready;
>      /*
> @@ -561,6 +566,7 @@ void multifd_save_cleanup(void)
>              error_free(local_err);
>          }
>      }
> +    qemu_sem_destroy(&multifd_send_state->channels_created);
>      qemu_sem_destroy(&multifd_send_state->channels_ready);
>      g_free(multifd_send_state->params);
>      multifd_send_state->params = NULL;
> @@ -787,13 +793,6 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
>      trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
>  
>      migrate_set_error(migrate_get_current(), err);
> -    /*
> -     * Error happen, mark multifd_send_thread status as 'quit' although it
> -     * is not created, and then tell who pay attention to me.
> -     */
> -    p->quit = true;
> -    qemu_sem_post(&multifd_send_state->channels_ready);
> -    qemu_sem_post(&p->sem_sync);
>      error_free(err);
>  }
>  
> @@ -862,39 +861,37 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>      return true;
>  }
>  
> -static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
> -                                             QIOChannel *ioc, Error *err)
> -{
> -     migrate_set_error(migrate_get_current(), err);
> -     /* Error happen, we need to tell who pay attention to me */
> -     qemu_sem_post(&multifd_send_state->channels_ready);
> -     qemu_sem_post(&p->sem_sync);
> -     /*
> -      * Although multifd_send_thread is not created, but main migration
> -      * thread need to judge whether it is running, so we need to mark
> -      * its status.
> -      */
> -     p->quit = true;
> -     object_unref(OBJECT(ioc));
> -     error_free(err);
> -}
> -
>  static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>  {
>      MultiFDSendParams *p = opaque;
>      QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
>      Error *local_err = NULL;
> +    bool ret;
>  
>      trace_multifd_new_send_channel_async(p->id);
> -    if (!qio_task_propagate_error(task, &local_err)) {
> -        qio_channel_set_delay(ioc, false);
> -        if (multifd_channel_connect(p, ioc, &local_err)) {
> -            return;
> -        }
> +
> +    if (qio_task_propagate_error(task, &local_err)) {
> +        ret = false;
> +        goto out;
> +    }
> +
> +    qio_channel_set_delay(ioc, false);
> +    ret = multifd_channel_connect(p, ioc, &local_err);
> +
> +out:
> +    /*
> +     * Here we're not interested whether creation succeeded, only that
> +     * it happened at all.
> +     */
> +    qemu_sem_post(&multifd_send_state->channels_created);
> +    if (ret) {
> +        return;
>      }
>  
>      trace_multifd_new_send_channel_async_error(p->id, local_err);
> -    multifd_new_send_channel_cleanup(p, ioc, local_err);
> +    migrate_set_error(migrate_get_current(), local_err);
> +    object_unref(OBJECT(ioc));
> +    error_free(local_err);
>  }
>  
>  static void multifd_new_send_channel_create(gpointer opaque)
> @@ -918,6 +915,7 @@ bool multifd_save_setup(void)
>      multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
>      multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
>      multifd_send_state->pages = multifd_pages_init(page_count);
> +    qemu_sem_init(&multifd_send_state->channels_created, 0);
>      qemu_sem_init(&multifd_send_state->channels_ready, 0);
>      qatomic_set(&multifd_send_state->exiting, 0);
>      multifd_send_state->ops = multifd_ops[migrate_multifd_compression()];
> @@ -953,6 +951,15 @@ bool multifd_save_setup(void)
>          multifd_new_send_channel_create(p);
>      }
>  
> +    /*
> +     * Wait until channel creation has started for all channels. The
> +     * creation can still fail, but no more channels will be created
> +     * past this point.
> +     */

Let me double check with you here on the TLS use case.

IIUC we still can have more channels to be created if TLS is enabled: we
notify the sem as long as the handshake thread is created, then the
handshake thread can further create the tls-armed iochannel?  However I
think I get your point, and that is fine, because if that is the case, even
though this loop can complete before tls further creates the final channel,
we'll still see tls_thread_created==true and join() that tls thread first,
then further we'll join() the next multifd thread even if a new one will
pop up, or if it failed then nothing to join besides the tls thread.

I'm not sure whether Avihai has any input, I think this can be a good idea
indeed.  there's a dependency chain on the ordering if my above
undertanding is correct; we may want to document this somewhere, perhaps
right here on the chaining of threads and how we handle that?

This may not allow a concurrent migrate_cancel to respond, but I assume
this is good enough; the migrate_cancel request is indeed at least so far
something I made up, but not a request from anyone.  We can leave that for
later and fix the race / crash first.  This seems to be a complete fix from
that regard.

> +    for (i = 0; i < thread_count; i++) {
> +        qemu_sem_wait(&multifd_send_state->channels_created);
> +    }
> +
>      for (i = 0; i < thread_count; i++) {
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>  
> -- 
> 2.35.3
> 

One other note is I think this will also deserve a cc: stable? But then
it'll mean all patch 3/4 will also need to copy stable to make Michael's
life easier.

Let's also copy Dan when repost; after all he more or less owns the TLS
part.

Thanks!

-- 
Peter Xu

Re: [PATCH 5/5] migration/multifd: Add a synchronization point for channel creation

[Avihai Horon]

On 05/02/2024 8:20, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> On Fri, Feb 02, 2024 at 04:11:28PM -0300, Fabiano Rosas wrote:
>> It is possible that one of the multifd channels fails to be created at
>> multifd_new_send_channel_async() while the rest of the channel
>> creation tasks are still in flight.
>>
>> This could lead to multifd_save_cleanup() executing the
>> qemu_thread_join() loop too early and not waiting for the threads
>> which haven't been created yet, leading to the freeing of resources
>> that the newly created threads will try to access and crash.
>>
>> Add a synchronization point after which there will be no attempts at
>> thread creation and therefore calling multifd_save_cleanup() past that
>> point will ensure it properly waits for the threads.
>>
>> A note about performance: Prior to this patch, if a channel took too
>> long to be established, other channels could finish connecting first
>> and already start taking load. Now we're bounded by the
>> slowest-connecting channel.
> Yes, I think this should (hopefully!) be fine.
>
>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> ---
>>   migration/multifd.c | 67 +++++++++++++++++++++++++--------------------
>>   1 file changed, 37 insertions(+), 30 deletions(-)
>>
>> diff --git a/migration/multifd.c b/migration/multifd.c
>> index 1851206352..888ac8b05d 100644
>> --- a/migration/multifd.c
>> +++ b/migration/multifd.c
>> @@ -360,6 +360,11 @@ struct {
>>       MultiFDPages_t *pages;
>>       /* global number of generated multifd packets */
>>       uint64_t packet_num;
>> +    /*
>> +     * Synchronization point past which no more channels will be
>> +     * created.
>> +     */
>> +    QemuSemaphore channels_created;
>>       /* send channels ready */
>>       QemuSemaphore channels_ready;
>>       /*
>> @@ -561,6 +566,7 @@ void multifd_save_cleanup(void)
>>               error_free(local_err);
>>           }
>>       }
>> +    qemu_sem_destroy(&multifd_send_state->channels_created);
>>       qemu_sem_destroy(&multifd_send_state->channels_ready);
>>       g_free(multifd_send_state->params);
>>       multifd_send_state->params = NULL;
>> @@ -787,13 +793,6 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
>>       trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
>>
>>       migrate_set_error(migrate_get_current(), err);
>> -    /*
>> -     * Error happen, mark multifd_send_thread status as 'quit' although it
>> -     * is not created, and then tell who pay attention to me.
>> -     */
>> -    p->quit = true;
>> -    qemu_sem_post(&multifd_send_state->channels_ready);
>> -    qemu_sem_post(&p->sem_sync);
>>       error_free(err);
>>   }
>>
>> @@ -862,39 +861,37 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>>       return true;
>>   }
>>
>> -static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
>> -                                             QIOChannel *ioc, Error *err)
>> -{
>> -     migrate_set_error(migrate_get_current(), err);
>> -     /* Error happen, we need to tell who pay attention to me */
>> -     qemu_sem_post(&multifd_send_state->channels_ready);
>> -     qemu_sem_post(&p->sem_sync);
>> -     /*
>> -      * Although multifd_send_thread is not created, but main migration
>> -      * thread need to judge whether it is running, so we need to mark
>> -      * its status.
>> -      */
>> -     p->quit = true;
>> -     object_unref(OBJECT(ioc));
>> -     error_free(err);
>> -}
>> -
>>   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>>   {
>>       MultiFDSendParams *p = opaque;
>>       QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
>>       Error *local_err = NULL;
>> +    bool ret;
>>
>>       trace_multifd_new_send_channel_async(p->id);
>> -    if (!qio_task_propagate_error(task, &local_err)) {
>> -        qio_channel_set_delay(ioc, false);
>> -        if (multifd_channel_connect(p, ioc, &local_err)) {
>> -            return;
>> -        }
>> +
>> +    if (qio_task_propagate_error(task, &local_err)) {
>> +        ret = false;
>> +        goto out;
>> +    }
>> +
>> +    qio_channel_set_delay(ioc, false);
>> +    ret = multifd_channel_connect(p, ioc, &local_err);
>> +
>> +out:
>> +    /*
>> +     * Here we're not interested whether creation succeeded, only that
>> +     * it happened at all.
>> +     */
>> +    qemu_sem_post(&multifd_send_state->channels_created);
>> +    if (ret) {
>> +        return;
>>       }
>>
>>       trace_multifd_new_send_channel_async_error(p->id, local_err);
>> -    multifd_new_send_channel_cleanup(p, ioc, local_err);
>> +    migrate_set_error(migrate_get_current(), local_err);
>> +    object_unref(OBJECT(ioc));
>> +    error_free(local_err);
>>   }
>>
>>   static void multifd_new_send_channel_create(gpointer opaque)
>> @@ -918,6 +915,7 @@ bool multifd_save_setup(void)
>>       multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
>>       multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
>>       multifd_send_state->pages = multifd_pages_init(page_count);
>> +    qemu_sem_init(&multifd_send_state->channels_created, 0);
>>       qemu_sem_init(&multifd_send_state->channels_ready, 0);
>>       qatomic_set(&multifd_send_state->exiting, 0);
>>       multifd_send_state->ops = multifd_ops[migrate_multifd_compression()];
>> @@ -953,6 +951,15 @@ bool multifd_save_setup(void)
>>           multifd_new_send_channel_create(p);
>>       }
>>
>> +    /*
>> +     * Wait until channel creation has started for all channels. The
>> +     * creation can still fail, but no more channels will be created
>> +     * past this point.
>> +     */
> Let me double check with you here on the TLS use case.
>
> IIUC we still can have more channels to be created if TLS is enabled: we
> notify the sem as long as the handshake thread is created, then the
> handshake thread can further create the tls-armed iochannel?  However I
> think I get your point, and that is fine, because if that is the case, even
> though this loop can complete before tls further creates the final channel,
> we'll still see tls_thread_created==true and join() that tls thread first,
> then further we'll join() the next multifd thread even if a new one will
> pop up, or if it failed then nothing to join besides the tls thread.
>
> I'm not sure whether Avihai has any input, I think this can be a good idea
> indeed.

Nothing special, my understanding of this is the same as yours.
This fix looks solid.

>    there's a dependency chain on the ordering if my above
> undertanding is correct; we may want to document this somewhere, perhaps
> right here on the chaining of threads and how we handle that?

I agree, this is subtle and may deserve a small note or hint.

>
> This may not allow a concurrent migrate_cancel to respond, but I assume
> this is good enough; the migrate_cancel request is indeed at least so far
> something I made up, but not a request from anyone.  We can leave that for
> later and fix the race / crash first.  This seems to be a complete fix from
> that regard.
>
>> +    for (i = 0; i < thread_count; i++) {
>> +        qemu_sem_wait(&multifd_send_state->channels_created);
>> +    }
>> +
>>       for (i = 0; i < thread_count; i++) {
>>           MultiFDSendParams *p = &multifd_send_state->params[i];
>>
>> --
>> 2.35.3
>>
> One other note is I think this will also deserve a cc: stable? But then
> it'll mean all patch 3/4 will also need to copy stable to make Michael's
> life easier.
>
> Let's also copy Dan when repost; after all he more or less owns the TLS
> part.
>
> Thanks!
>
> --
> Peter Xu
>

Re: [PATCH 5/5] migration/multifd: Add a synchronization point for channel creation

[Peter Xu]

On Mon, Feb 05, 2024 at 01:10:14PM +0200, Avihai Horon wrote:
> 
> On 05/02/2024 8:20, Peter Xu wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > On Fri, Feb 02, 2024 at 04:11:28PM -0300, Fabiano Rosas wrote:
> > > It is possible that one of the multifd channels fails to be created at
> > > multifd_new_send_channel_async() while the rest of the channel
> > > creation tasks are still in flight.
> > > 
> > > This could lead to multifd_save_cleanup() executing the
> > > qemu_thread_join() loop too early and not waiting for the threads
> > > which haven't been created yet, leading to the freeing of resources
> > > that the newly created threads will try to access and crash.
> > > 
> > > Add a synchronization point after which there will be no attempts at
> > > thread creation and therefore calling multifd_save_cleanup() past that
> > > point will ensure it properly waits for the threads.
> > > 
> > > A note about performance: Prior to this patch, if a channel took too
> > > long to be established, other channels could finish connecting first
> > > and already start taking load. Now we're bounded by the
> > > slowest-connecting channel.
> > Yes, I think this should (hopefully!) be fine.
> > 
> > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > > ---
> > >   migration/multifd.c | 67 +++++++++++++++++++++++++--------------------
> > >   1 file changed, 37 insertions(+), 30 deletions(-)
> > > 
> > > diff --git a/migration/multifd.c b/migration/multifd.c
> > > index 1851206352..888ac8b05d 100644
> > > --- a/migration/multifd.c
> > > +++ b/migration/multifd.c
> > > @@ -360,6 +360,11 @@ struct {
> > >       MultiFDPages_t *pages;
> > >       /* global number of generated multifd packets */
> > >       uint64_t packet_num;
> > > +    /*
> > > +     * Synchronization point past which no more channels will be
> > > +     * created.
> > > +     */
> > > +    QemuSemaphore channels_created;
> > >       /* send channels ready */
> > >       QemuSemaphore channels_ready;
> > >       /*
> > > @@ -561,6 +566,7 @@ void multifd_save_cleanup(void)
> > >               error_free(local_err);
> > >           }
> > >       }
> > > +    qemu_sem_destroy(&multifd_send_state->channels_created);
> > >       qemu_sem_destroy(&multifd_send_state->channels_ready);
> > >       g_free(multifd_send_state->params);
> > >       multifd_send_state->params = NULL;
> > > @@ -787,13 +793,6 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
> > >       trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
> > > 
> > >       migrate_set_error(migrate_get_current(), err);
> > > -    /*
> > > -     * Error happen, mark multifd_send_thread status as 'quit' although it
> > > -     * is not created, and then tell who pay attention to me.
> > > -     */
> > > -    p->quit = true;
> > > -    qemu_sem_post(&multifd_send_state->channels_ready);
> > > -    qemu_sem_post(&p->sem_sync);
> > >       error_free(err);
> > >   }
> > > 
> > > @@ -862,39 +861,37 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
> > >       return true;
> > >   }
> > > 
> > > -static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
> > > -                                             QIOChannel *ioc, Error *err)
> > > -{
> > > -     migrate_set_error(migrate_get_current(), err);
> > > -     /* Error happen, we need to tell who pay attention to me */
> > > -     qemu_sem_post(&multifd_send_state->channels_ready);
> > > -     qemu_sem_post(&p->sem_sync);
> > > -     /*
> > > -      * Although multifd_send_thread is not created, but main migration
> > > -      * thread need to judge whether it is running, so we need to mark
> > > -      * its status.
> > > -      */
> > > -     p->quit = true;
> > > -     object_unref(OBJECT(ioc));
> > > -     error_free(err);
> > > -}
> > > -
> > >   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
> > >   {
> > >       MultiFDSendParams *p = opaque;
> > >       QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
> > >       Error *local_err = NULL;
> > > +    bool ret;
> > > 
> > >       trace_multifd_new_send_channel_async(p->id);
> > > -    if (!qio_task_propagate_error(task, &local_err)) {
> > > -        qio_channel_set_delay(ioc, false);
> > > -        if (multifd_channel_connect(p, ioc, &local_err)) {
> > > -            return;
> > > -        }
> > > +
> > > +    if (qio_task_propagate_error(task, &local_err)) {
> > > +        ret = false;
> > > +        goto out;
> > > +    }
> > > +
> > > +    qio_channel_set_delay(ioc, false);
> > > +    ret = multifd_channel_connect(p, ioc, &local_err);
> > > +
> > > +out:
> > > +    /*
> > > +     * Here we're not interested whether creation succeeded, only that
> > > +     * it happened at all.
> > > +     */
> > > +    qemu_sem_post(&multifd_send_state->channels_created);
> > > +    if (ret) {
> > > +        return;
> > >       }
> > > 
> > >       trace_multifd_new_send_channel_async_error(p->id, local_err);
> > > -    multifd_new_send_channel_cleanup(p, ioc, local_err);
> > > +    migrate_set_error(migrate_get_current(), local_err);
> > > +    object_unref(OBJECT(ioc));
> > > +    error_free(local_err);
> > >   }
> > > 
> > >   static void multifd_new_send_channel_create(gpointer opaque)
> > > @@ -918,6 +915,7 @@ bool multifd_save_setup(void)
> > >       multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
> > >       multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
> > >       multifd_send_state->pages = multifd_pages_init(page_count);
> > > +    qemu_sem_init(&multifd_send_state->channels_created, 0);
> > >       qemu_sem_init(&multifd_send_state->channels_ready, 0);
> > >       qatomic_set(&multifd_send_state->exiting, 0);
> > >       multifd_send_state->ops = multifd_ops[migrate_multifd_compression()];
> > > @@ -953,6 +951,15 @@ bool multifd_save_setup(void)
> > >           multifd_new_send_channel_create(p);
> > >       }
> > > 
> > > +    /*
> > > +     * Wait until channel creation has started for all channels. The
> > > +     * creation can still fail, but no more channels will be created
> > > +     * past this point.
> > > +     */
> > Let me double check with you here on the TLS use case.
> > 
> > IIUC we still can have more channels to be created if TLS is enabled: we
> > notify the sem as long as the handshake thread is created, then the
> > handshake thread can further create the tls-armed iochannel?  However I
> > think I get your point, and that is fine, because if that is the case, even
> > though this loop can complete before tls further creates the final channel,
> > we'll still see tls_thread_created==true and join() that tls thread first,
> > then further we'll join() the next multifd thread even if a new one will
> > pop up, or if it failed then nothing to join besides the tls thread.
> > 
> > I'm not sure whether Avihai has any input, I think this can be a good idea
> > indeed.
> 
> Nothing special, my understanding of this is the same as yours.
> This fix looks solid.
> 
> >    there's a dependency chain on the ordering if my above
> > undertanding is correct; we may want to document this somewhere, perhaps
> > right here on the chaining of threads and how we handle that?
> 
> I agree, this is subtle and may deserve a small note or hint.

IMHO it'll be always better to be verbose on these than "not enough info".

One thing I'd also like a comment is now the order is a must to firstly
join tls threads then multifd threads, not vice versa, not anymore. We may
want a comment above the two join()s there to state this hard requirement.

> 
> > 
> > This may not allow a concurrent migrate_cancel to respond, but I assume
> > this is good enough; the migrate_cancel request is indeed at least so far
> > something I made up, but not a request from anyone.  We can leave that for
> > later and fix the race / crash first.  This seems to be a complete fix from
> > that regard.
> > 
> > > +    for (i = 0; i < thread_count; i++) {
> > > +        qemu_sem_wait(&multifd_send_state->channels_created);
> > > +    }
> > > +
> > >       for (i = 0; i < thread_count; i++) {
> > >           MultiFDSendParams *p = &multifd_send_state->params[i];
> > > 
> > > --
> > > 2.35.3
> > > 
> > One other note is I think this will also deserve a cc: stable? But then
> > it'll mean all patch 3/4 will also need to copy stable to make Michael's
> > life easier.
> > 
> > Let's also copy Dan when repost; after all he more or less owns the TLS
> > part.
> > 
> > Thanks!
> > 
> > --
> > Peter Xu
> > 
> 

-- 
Peter Xu

Re: [PATCH 5/5] migration/multifd: Add a synchronization point for channel creation

[Fabiano Rosas]

Peter Xu <peterx@redhat.com> writes:

> On Mon, Feb 05, 2024 at 01:10:14PM +0200, Avihai Horon wrote:
>> 
>> On 05/02/2024 8:20, Peter Xu wrote:
>> > External email: Use caution opening links or attachments
>> > 
>> > 
>> > On Fri, Feb 02, 2024 at 04:11:28PM -0300, Fabiano Rosas wrote:
>> > > It is possible that one of the multifd channels fails to be created at
>> > > multifd_new_send_channel_async() while the rest of the channel
>> > > creation tasks are still in flight.
>> > > 
>> > > This could lead to multifd_save_cleanup() executing the
>> > > qemu_thread_join() loop too early and not waiting for the threads
>> > > which haven't been created yet, leading to the freeing of resources
>> > > that the newly created threads will try to access and crash.
>> > > 
>> > > Add a synchronization point after which there will be no attempts at
>> > > thread creation and therefore calling multifd_save_cleanup() past that
>> > > point will ensure it properly waits for the threads.
>> > > 
>> > > A note about performance: Prior to this patch, if a channel took too
>> > > long to be established, other channels could finish connecting first
>> > > and already start taking load. Now we're bounded by the
>> > > slowest-connecting channel.
>> > Yes, I think this should (hopefully!) be fine.
>> > 
>> > > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > > ---
>> > >   migration/multifd.c | 67 +++++++++++++++++++++++++--------------------
>> > >   1 file changed, 37 insertions(+), 30 deletions(-)
>> > > 
>> > > diff --git a/migration/multifd.c b/migration/multifd.c
>> > > index 1851206352..888ac8b05d 100644
>> > > --- a/migration/multifd.c
>> > > +++ b/migration/multifd.c
>> > > @@ -360,6 +360,11 @@ struct {
>> > >       MultiFDPages_t *pages;
>> > >       /* global number of generated multifd packets */
>> > >       uint64_t packet_num;
>> > > +    /*
>> > > +     * Synchronization point past which no more channels will be
>> > > +     * created.
>> > > +     */
>> > > +    QemuSemaphore channels_created;
>> > >       /* send channels ready */
>> > >       QemuSemaphore channels_ready;
>> > >       /*
>> > > @@ -561,6 +566,7 @@ void multifd_save_cleanup(void)
>> > >               error_free(local_err);
>> > >           }
>> > >       }
>> > > +    qemu_sem_destroy(&multifd_send_state->channels_created);
>> > >       qemu_sem_destroy(&multifd_send_state->channels_ready);
>> > >       g_free(multifd_send_state->params);
>> > >       multifd_send_state->params = NULL;
>> > > @@ -787,13 +793,6 @@ static void multifd_tls_outgoing_handshake(QIOTask *task,
>> > >       trace_multifd_tls_outgoing_handshake_error(ioc, error_get_pretty(err));
>> > > 
>> > >       migrate_set_error(migrate_get_current(), err);
>> > > -    /*
>> > > -     * Error happen, mark multifd_send_thread status as 'quit' although it
>> > > -     * is not created, and then tell who pay attention to me.
>> > > -     */
>> > > -    p->quit = true;
>> > > -    qemu_sem_post(&multifd_send_state->channels_ready);
>> > > -    qemu_sem_post(&p->sem_sync);
>> > >       error_free(err);
>> > >   }
>> > > 
>> > > @@ -862,39 +861,37 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>> > >       return true;
>> > >   }
>> > > 
>> > > -static void multifd_new_send_channel_cleanup(MultiFDSendParams *p,
>> > > -                                             QIOChannel *ioc, Error *err)
>> > > -{
>> > > -     migrate_set_error(migrate_get_current(), err);
>> > > -     /* Error happen, we need to tell who pay attention to me */
>> > > -     qemu_sem_post(&multifd_send_state->channels_ready);
>> > > -     qemu_sem_post(&p->sem_sync);
>> > > -     /*
>> > > -      * Although multifd_send_thread is not created, but main migration
>> > > -      * thread need to judge whether it is running, so we need to mark
>> > > -      * its status.
>> > > -      */
>> > > -     p->quit = true;
>> > > -     object_unref(OBJECT(ioc));
>> > > -     error_free(err);
>> > > -}
>> > > -
>> > >   static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
>> > >   {
>> > >       MultiFDSendParams *p = opaque;
>> > >       QIOChannel *ioc = QIO_CHANNEL(qio_task_get_source(task));
>> > >       Error *local_err = NULL;
>> > > +    bool ret;
>> > > 
>> > >       trace_multifd_new_send_channel_async(p->id);
>> > > -    if (!qio_task_propagate_error(task, &local_err)) {
>> > > -        qio_channel_set_delay(ioc, false);
>> > > -        if (multifd_channel_connect(p, ioc, &local_err)) {
>> > > -            return;
>> > > -        }
>> > > +
>> > > +    if (qio_task_propagate_error(task, &local_err)) {
>> > > +        ret = false;
>> > > +        goto out;
>> > > +    }
>> > > +
>> > > +    qio_channel_set_delay(ioc, false);
>> > > +    ret = multifd_channel_connect(p, ioc, &local_err);
>> > > +
>> > > +out:
>> > > +    /*
>> > > +     * Here we're not interested whether creation succeeded, only that
>> > > +     * it happened at all.
>> > > +     */
>> > > +    qemu_sem_post(&multifd_send_state->channels_created);
>> > > +    if (ret) {
>> > > +        return;
>> > >       }
>> > > 
>> > >       trace_multifd_new_send_channel_async_error(p->id, local_err);
>> > > -    multifd_new_send_channel_cleanup(p, ioc, local_err);
>> > > +    migrate_set_error(migrate_get_current(), local_err);
>> > > +    object_unref(OBJECT(ioc));
>> > > +    error_free(local_err);
>> > >   }
>> > > 
>> > >   static void multifd_new_send_channel_create(gpointer opaque)
>> > > @@ -918,6 +915,7 @@ bool multifd_save_setup(void)
>> > >       multifd_send_state = g_malloc0(sizeof(*multifd_send_state));
>> > >       multifd_send_state->params = g_new0(MultiFDSendParams, thread_count);
>> > >       multifd_send_state->pages = multifd_pages_init(page_count);
>> > > +    qemu_sem_init(&multifd_send_state->channels_created, 0);
>> > >       qemu_sem_init(&multifd_send_state->channels_ready, 0);
>> > >       qatomic_set(&multifd_send_state->exiting, 0);
>> > >       multifd_send_state->ops = multifd_ops[migrate_multifd_compression()];
>> > > @@ -953,6 +951,15 @@ bool multifd_save_setup(void)
>> > >           multifd_new_send_channel_create(p);
>> > >       }
>> > > 
>> > > +    /*
>> > > +     * Wait until channel creation has started for all channels. The
>> > > +     * creation can still fail, but no more channels will be created
>> > > +     * past this point.
>> > > +     */
>> > Let me double check with you here on the TLS use case.
>> > 
>> > IIUC we still can have more channels to be created if TLS is enabled: we
>> > notify the sem as long as the handshake thread is created, then the
>> > handshake thread can further create the tls-armed iochannel?

Oh, that's just a mistake on my part. We cannot allow "no more channels
will be created" above to be a lie.

I'll fix it for v2. I just found a way to stop calling
multifd_channel_connect() twice so now I can have the TLS thread posting
the semaphore at the appropriate place.

>> > However I
>> > think I get your point, and that is fine, because if that is the case, even
>> > though this loop can complete before tls further creates the final channel,
>> > we'll still see tls_thread_created==true and join() that tls thread first,
>> > then further we'll join() the next multifd thread even if a new one will
>> > pop up, or if it failed then nothing to join besides the tls thread.
>> > 
>> > I'm not sure whether Avihai has any input, I think this can be a good idea
>> > indeed.
>> 
>> Nothing special, my understanding of this is the same as yours.
>> This fix looks solid.
>> 
>> >    there's a dependency chain on the ordering if my above
>> > undertanding is correct; we may want to document this somewhere, perhaps
>> > right here on the chaining of threads and how we handle that?
>> 
>> I agree, this is subtle and may deserve a small note or hint.
>
> IMHO it'll be always better to be verbose on these than "not enough info".
>
> One thing I'd also like a comment is now the order is a must to firstly
> join tls threads then multifd threads, not vice versa, not anymore. We may
> want a comment above the two join()s there to state this hard requirement.
>
>> 
>> > 
>> > This may not allow a concurrent migrate_cancel to respond, but I assume
>> > this is good enough; the migrate_cancel request is indeed at least so far
>> > something I made up, but not a request from anyone.  We can leave that for
>> > later and fix the race / crash first.  This seems to be a complete fix from
>> > that regard.
>> > 
>> > > +    for (i = 0; i < thread_count; i++) {
>> > > +        qemu_sem_wait(&multifd_send_state->channels_created);
>> > > +    }
>> > > +
>> > >       for (i = 0; i < thread_count; i++) {
>> > >           MultiFDSendParams *p = &multifd_send_state->params[i];
>> > > 
>> > > --
>> > > 2.35.3
>> > > 
>> > One other note is I think this will also deserve a cc: stable? But then
>> > it'll mean all patch 3/4 will also need to copy stable to make Michael's
>> > life easier.
>> > 
>> > Let's also copy Dan when repost; after all he more or less owns the TLS
>> > part.
>> > 
>> > Thanks!
>> > 
>> > --
>> > Peter Xu
>> > 
>> 

Re: [PATCH 0/5] migration/multifd: Fix channel creation vs. cleanup races

[Peter Xu]

On Fri, Feb 02, 2024 at 04:11:23PM -0300, Fabiano Rosas wrote:
> Hi,
> 
> This contains 2 patches from my previous series addressing the
> p->running misuse and the TLS thread leak and 3 new patches to fix the
> cleanup-while-creating-threads race.
> 
> For the p->running I'm keeping the idea from the other series to
> remove p->running and use a more narrow p->thread_created flag. This
> flag is used only inform whether the thread has been created so we can
> join it.
> 
> For the cleanup race I have moved some code around and added a
> semaphore to make multifd_save_setup() only return once all channel
> creation tasks have started.
> 
> The idea is that after multifd_save_setup() returns, no new creations
> are in flight and the p->thread_created flags will never change again,
> so they're enough to cause the cleanup code to wait for the threads to
> join.
> 
> CI run: https://gitlab.com/farosas/qemu/-/pipelines/1162798843
> 
> @Peter: I can rebase this on top of your series once we decide about
> it.

I have one thing to double check with you in patch 5, besides that the
whole set looks all good to me.  Copy Dan here in case he has any input.

If you confirm both sides (my replies to last patch of both this set and
the other lockless change of mine), feel free to repost directly based on
that series for v2.

Thanks,

-- 
Peter Xu