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

[Avihai Horon <avihaih@nvidia.com>]

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
>