[PATCH v2 0/5] migration: cleanup TLS channel referencing

[PATCH v2 0/5] migration: cleanup TLS channel referencing

[peterx]

From: Peter Xu <peterx@redhat.com>

v2:
- add patches
  - migration/multifd: Make multifd_channel_connect() return void
  - migration/multifd: Cleanup outgoing_args in state destroy
  - migration/multifd: Drop unnecessary helper to destroy IOC
- fix spelling

This is a small cleanup patchset to firstly cleanup tls iochannel deref on
error paths, then further remove one unused var on yank if the cleanup
applies.  In v2 three more small cleanups on top as suggested by reviewers.

Please feel free to have a look, thanks.

Peter Xu (5):
  migration/multifd: Cleanup TLS iochannel referencing
  migration/multifd: Drop registered_yank
  migration/multifd: Make multifd_channel_connect() return void
  migration/multifd: Cleanup outgoing_args in state destroy
  migration/multifd: Drop unnecessary helper to destroy IOC

 migration/multifd.h |  2 --
 migration/socket.h  |  3 ++-
 migration/multifd.c | 58 +++++++++++++++++++++++----------------------
 migration/socket.c  | 19 +++++++--------
 4 files changed, 40 insertions(+), 42 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] migration/multifd: Cleanup TLS iochannel referencing

[peterx]

From: Peter Xu <peterx@redhat.com>

Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
blocking handshake") introduced a thread for TLS channels, which will
resolve the issue on blocking the main thread.  However in the same commit
p->c is slightly abused just to be able to pass over the pointer "p" into
the thread.

That's the major reason we'll need to conditionally free the io channel in
the fault paths.

To clean it up, using a separate structure to pass over both "p" and "tioc"
in the tls handshake thread.  Then we can make it a rule that p->c will
never be set until the channel is completely setup.  With that, we can drop
the tricky conditional unref of the io channel in the error path.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index adfe8c9a0a..6b89a6c885 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -873,16 +873,22 @@ out:
 
 static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque);
 
+typedef struct {
+    MultiFDSendParams *p;
+    QIOChannelTLS *tioc;
+} MultiFDTLSThreadArgs;
+
 static void *multifd_tls_handshake_thread(void *opaque)
 {
-    MultiFDSendParams *p = opaque;
-    QIOChannelTLS *tioc = QIO_CHANNEL_TLS(p->c);
+    MultiFDTLSThreadArgs *args = opaque;
 
-    qio_channel_tls_handshake(tioc,
+    qio_channel_tls_handshake(args->tioc,
                               multifd_new_send_channel_async,
-                              p,
+                              args->p,
                               NULL,
                               NULL);
+    g_free(args);
+
     return NULL;
 }
 
@@ -892,6 +898,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
 {
     MigrationState *s = migrate_get_current();
     const char *hostname = s->hostname;
+    MultiFDTLSThreadArgs *args;
     QIOChannelTLS *tioc;
 
     tioc = migration_tls_client_create(ioc, hostname, errp);
@@ -906,11 +913,14 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     object_unref(OBJECT(ioc));
     trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
     qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
-    p->c = QIO_CHANNEL(tioc);
+
+    args = g_new0(MultiFDTLSThreadArgs, 1);
+    args->tioc = tioc;
+    args->p = p;
 
     p->tls_thread_created = true;
     qemu_thread_create(&p->tls_thread, "multifd-tls-handshake-worker",
-                       multifd_tls_handshake_thread, p,
+                       multifd_tls_handshake_thread, args,
                        QEMU_THREAD_JOINABLE);
     return true;
 }
@@ -923,6 +933,7 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
 
     migration_ioc_register_yank(ioc);
     p->registered_yank = true;
+    /* Setup p->c only if the channel is completely setup */
     p->c = ioc;
 
     p->thread_created = true;
@@ -976,14 +987,12 @@ out:
 
     trace_multifd_new_send_channel_async_error(p->id, local_err);
     multifd_send_set_error(local_err);
-    if (!p->c) {
-        /*
-         * If no channel has been created, drop the initial
-         * reference. Otherwise cleanup happens at
-         * multifd_send_channel_destroy()
-         */
-        object_unref(OBJECT(ioc));
-    }
+    /*
+     * For error cases (TLS or non-TLS), IO channel is always freed here
+     * rather than when cleanup multifd: since p->c is not set, multifd
+     * cleanup code doesn't even know its existence.
+     */
+    object_unref(OBJECT(ioc));
     error_free(local_err);
 }
 
-- 
2.43.0

Re: [PATCH v2 1/5] migration/multifd: Cleanup TLS iochannel referencing

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> Commit a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to
> blocking handshake") introduced a thread for TLS channels, which will
> resolve the issue on blocking the main thread.  However in the same commit
> p->c is slightly abused just to be able to pass over the pointer "p" into
> the thread.
>
> That's the major reason we'll need to conditionally free the io channel in
> the fault paths.
>
> To clean it up, using a separate structure to pass over both "p" and "tioc"
> in the tls handshake thread.  Then we can make it a rule that p->c will
> never be set until the channel is completely setup.  With that, we can drop
> the tricky conditional unref of the io channel in the error path.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 2/5] migration/multifd: Drop registered_yank

[peterx]

From: Peter Xu <peterx@redhat.com>

With a clear definition of p->c protocol, where we only set it up if the
channel is fully established (TLS or non-TLS), registered_yank boolean will
have equal meaning of "p->c != NULL".

Drop registered_yank by checking p->c instead.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h | 2 --
 migration/multifd.c | 7 +++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 8a1cad0996..b3fe27ae93 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -78,8 +78,6 @@ typedef struct {
     bool tls_thread_created;
     /* communication channel */
     QIOChannel *c;
-    /* is the yank function registered */
-    bool registered_yank;
     /* packet allocated len */
     uint32_t packet_len;
     /* guest page size */
diff --git a/migration/multifd.c b/migration/multifd.c
index 6b89a6c885..da2e7c1db1 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
 
 static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 {
-    if (p->registered_yank) {
+    if (p->c) {
         migration_ioc_unregister_yank(p->c);
+        multifd_send_channel_destroy(p->c);
+        p->c = NULL;
     }
-    multifd_send_channel_destroy(p->c);
-    p->c = NULL;
     qemu_sem_destroy(&p->sem);
     qemu_sem_destroy(&p->sem_sync);
     g_free(p->name);
@@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
     qio_channel_set_delay(ioc, false);
 
     migration_ioc_register_yank(ioc);
-    p->registered_yank = true;
     /* Setup p->c only if the channel is completely setup */
     p->c = ioc;
 
-- 
2.43.0

[PATCH v2 3/5] migration/multifd: Make multifd_channel_connect() return void

[peterx]

From: Peter Xu <peterx@redhat.com>

It never fails, drop the retval and also the Error**.

Suggested-by: Avihai Horon <avihaih@nvidia.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index da2e7c1db1..f52f01ca85 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -925,9 +925,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
     return true;
 }
 
-static bool multifd_channel_connect(MultiFDSendParams *p,
-                                    QIOChannel *ioc,
-                                    Error **errp)
+static void multifd_channel_connect(MultiFDSendParams *p, QIOChannel *ioc)
 {
     qio_channel_set_delay(ioc, false);
 
@@ -938,7 +936,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
     p->thread_created = true;
     qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
                        QEMU_THREAD_JOINABLE);
-    return true;
 }
 
 /*
@@ -970,7 +967,8 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
             return;
         }
     } else {
-        ret = multifd_channel_connect(p, ioc, &local_err);
+        multifd_channel_connect(p, ioc);
+        ret = true;
     }
 
 out:
-- 
2.43.0

Re: [PATCH v2 3/5] migration/multifd: Make multifd_channel_connect() return void

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> It never fails, drop the retval and also the Error**.
>
> Suggested-by: Avihai Horon <avihaih@nvidia.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 4/5] migration/multifd: Cleanup outgoing_args in state destroy

[peterx]

From: Peter Xu <peterx@redhat.com>

outgoing_args is a global cache of socket address to be reused in multifd.
Freeing the cache in per-channel destructor is more or less a hack.  Move
it to multifd_send_cleanup_state() so it only get checked once.  Use a
small helper to do so because it's internal of socket.c.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/socket.h  |  2 ++
 migration/multifd.c |  1 +
 migration/socket.c  | 12 ++++++++----
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/migration/socket.h b/migration/socket.h
index 5e4c33b8ea..5f52eddd4c 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -29,4 +29,6 @@ void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
 
 void socket_start_outgoing_migration(MigrationState *s,
                                      SocketAddress *saddr, Error **errp);
+void socket_cleanup_outgoing_migration(void);
+
 #endif
diff --git a/migration/multifd.c b/migration/multifd.c
index f52f01ca85..e901b32c19 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -671,6 +671,7 @@ static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 
 static void multifd_send_cleanup_state(void)
 {
+    socket_cleanup_outgoing_migration();
     qemu_sem_destroy(&multifd_send_state->channels_created);
     qemu_sem_destroy(&multifd_send_state->channels_ready);
     g_free(multifd_send_state->params);
diff --git a/migration/socket.c b/migration/socket.c
index 98e3ea1514..3184c7c3c1 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -64,10 +64,6 @@ int socket_send_channel_destroy(QIOChannel *send)
 {
     /* Remove channel */
     object_unref(OBJECT(send));
-    if (outgoing_args.saddr) {
-        qapi_free_SocketAddress(outgoing_args.saddr);
-        outgoing_args.saddr = NULL;
-    }
     return 0;
 }
 
@@ -137,6 +133,14 @@ void socket_start_outgoing_migration(MigrationState *s,
                                      NULL);
 }
 
+void socket_cleanup_outgoing_migration(void)
+{
+    if (outgoing_args.saddr) {
+        qapi_free_SocketAddress(outgoing_args.saddr);
+        outgoing_args.saddr = NULL;
+    }
+}
+
 static void socket_accept_incoming_migration(QIONetListener *listener,
                                              QIOChannelSocket *cioc,
                                              gpointer opaque)
-- 
2.43.0

Re: [PATCH v2 4/5] migration/multifd: Cleanup outgoing_args in state destroy

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> outgoing_args is a global cache of socket address to be reused in multifd.
> Freeing the cache in per-channel destructor is more or less a hack.  Move
> it to multifd_send_cleanup_state() so it only get checked once.  Use a
> small helper to do so because it's internal of socket.c.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH v2 5/5] migration/multifd: Drop unnecessary helper to destroy IOC

[peterx]

From: Peter Xu <peterx@redhat.com>

Both socket_send_channel_destroy() and multifd_send_channel_destroy() are
unnecessary wrappers to destroy an IOC, as the only thing to do is to
release the final IOC reference.  We have plenty of code that destroys an
IOC using direct unref() already; keep that style.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/socket.h  | 1 -
 migration/multifd.c | 7 +------
 migration/socket.c  | 7 -------
 3 files changed, 1 insertion(+), 14 deletions(-)

diff --git a/migration/socket.h b/migration/socket.h
index 5f52eddd4c..46c233ecd2 100644
--- a/migration/socket.h
+++ b/migration/socket.h
@@ -23,7 +23,6 @@
 
 void socket_send_channel_create(QIOTaskFunc f, void *data);
 QIOChannel *socket_send_channel_create_sync(Error **errp);
-int socket_send_channel_destroy(QIOChannel *send);
 
 void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
 
diff --git a/migration/multifd.c b/migration/multifd.c
index e901b32c19..c2eac0c3e6 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -641,16 +641,11 @@ static void multifd_send_terminate_threads(void)
     }
 }
 
-static int multifd_send_channel_destroy(QIOChannel *send)
-{
-    return socket_send_channel_destroy(send);
-}
-
 static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 {
     if (p->c) {
         migration_ioc_unregister_yank(p->c);
-        multifd_send_channel_destroy(p->c);
+        object_unref(OBJECT(p->c));
         p->c = NULL;
     }
     qemu_sem_destroy(&p->sem);
diff --git a/migration/socket.c b/migration/socket.c
index 3184c7c3c1..9ab89b1e08 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -60,13 +60,6 @@ QIOChannel *socket_send_channel_create_sync(Error **errp)
     return QIO_CHANNEL(sioc);
 }
 
-int socket_send_channel_destroy(QIOChannel *send)
-{
-    /* Remove channel */
-    object_unref(OBJECT(send));
-    return 0;
-}
-
 struct SocketConnectData {
     MigrationState *s;
     char *hostname;
-- 
2.43.0

Re: [PATCH v2 5/5] migration/multifd: Drop unnecessary helper to destroy IOC

[Fabiano Rosas]

peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> Both socket_send_channel_destroy() and multifd_send_channel_destroy() are
> unnecessary wrappers to destroy an IOC, as the only thing to do is to
> release the final IOC reference.  We have plenty of code that destroys an
> IOC using direct unref() already; keep that style.
>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

Re: [PATCH v2 0/5] migration: cleanup TLS channel referencing

[Peter Xu]

On Thu, Feb 22, 2024 at 05:52:56PM +0800, peterx@redhat.com wrote:
> From: Peter Xu <peterx@redhat.com>
> 
> v2:
> - add patches
>   - migration/multifd: Make multifd_channel_connect() return void
>   - migration/multifd: Cleanup outgoing_args in state destroy
>   - migration/multifd: Drop unnecessary helper to destroy IOC
> - fix spelling
> 
> This is a small cleanup patchset to firstly cleanup tls iochannel deref on
> error paths, then further remove one unused var on yank if the cleanup
> applies.  In v2 three more small cleanups on top as suggested by reviewers.
> 
> Please feel free to have a look, thanks.
> 
> Peter Xu (5):
>   migration/multifd: Cleanup TLS iochannel referencing
>   migration/multifd: Drop registered_yank
>   migration/multifd: Make multifd_channel_connect() return void
>   migration/multifd: Cleanup outgoing_args in state destroy
>   migration/multifd: Drop unnecessary helper to destroy IOC

queued.

-- 
Peter Xu