Re: [PULL 26/34] migration/multifd: Join the TLS thread

[Michael Tokarev]

08.02.2024 06:05, peterx@redhat.com :
> From: Fabiano Rosas <farosas@suse.de>
>
> We're currently leaking the resources of the TLS thread by not joining
> it and also overwriting the p->thread pointer altogether.
>
> 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>
> Link: 20240206215118.6171-2-farosas@suse.de">https://lore.kernel.org/r/20240206215118.6171-2-farosas@suse.de
> Signed-off-by: Peter Xu <peterx@redhat.com>

This change, which is suggested for -stable, while simple by its own, seems
to depend on the previous changes in this series, which are not for -stable.
In particular, whole "Finally recycle all the threads" loop in 
multifd_send_terminate_threads()
(to which the join is being added by this change) is moved from elsewhere by
12808db3b8 "migration/multifd: Cleanup multifd_save_cleanup()" (patch 24 in
this same series).

We can probably add the missing join right into the previous location of this
loop (before 12808db3b8).  I did this in the attached variant for 8.2, is
this correct?

Should we pick this up for 7.2 too?

Thanks,

/mjt

>   migration/multifd.h | 2 ++
>   migration/multifd.c | 8 +++++++-
>   2 files changed, 9 insertions(+), 1 deletion(-)
>
> 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 */
> diff --git a/migration/multifd.c b/migration/multifd.c
> index fbdb129088..5551711a2a 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;