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

[Daniel P . Berrangé]

On Mon, Feb 05, 2024 at 04:49: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.

AFAICS, it is not ovewriting 'p->thread' because at the time when the
TLS thread is created, the main 'send thread' has not yet been
created. The TLS thread and send thread execution times are mutually
exclusive.

The 'p->running' flag is already set to true when the TLS thread is
created, so the existing cleanup should be working too, so I'm not
seeing a bug that needs fixing here.

> 
> Fixes: a1af605bd5 ("migration/multifd: fix hangup with TLS-Multifd due to 
> blocking handshake")
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 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 ef13e2e781..8195c1daf3 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -630,6 +630,10 @@ static void multifd_send_terminate_threads(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);
>          }
> @@ -921,7 +925,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 78a2317263..720c9d50db 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
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|