diff mbox series

[1/2] migration/multifd: Cleanup TLS iochannel referencing

Message ID 20240208035126.370620-2-peterx@redhat.com
State New
Headers show
Series migration: cleanup TLS channel referencing | expand

Commit Message

Peter Xu Feb. 8, 2024, 3:51 a.m. UTC 
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(-)

Comments

Fabiano Rosas Feb. 8, 2024, 12:44 p.m. UTC | #1 
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>

Ok, I'm convinced after reading your reply on the other thread.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Avihai Horon Feb. 8, 2024, 2:10 p.m. UTC | #2 
On 08/02/2024 5:51, peterx@redhat.com wrote:
> External email: Use caution opening links or attachments
>
>
> 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..4a85a6b7b3 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 existance.

Small nit:
s/existance/existence

BTW, I just noticed that multifd_channel_connect() can't fail, probably 
would be good to turn it into a void function.

Thanks.

> +     */
> +    object_unref(OBJECT(ioc));
>       error_free(local_err);
>   }
>
> --
> 2.43.0
>
Peter Xu Feb. 21, 2024, 3:21 a.m. UTC | #3 
On Thu, Feb 08, 2024 at 04:10:58PM +0200, Avihai Horon wrote:
> 
> On 08/02/2024 5:51, peterx@redhat.com wrote:
> > External email: Use caution opening links or attachments
> > 
> > 
> > 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..4a85a6b7b3 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 existance.
> 
> Small nit:
> s/existance/existence
> 
> BTW, I just noticed that multifd_channel_connect() can't fail, probably
> would be good to turn it into a void function.

Yes we can.  I'll add a patch and fix the spell, thanks.
diff mbox series

Patch

diff --git a/migration/multifd.c b/migration/multifd.c
index adfe8c9a0a..4a85a6b7b3 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 existance.
+     */
+    object_unref(OBJECT(ioc));
     error_free(local_err);
 }