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: 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;