| Message ID | 20231110200241.20679-3-farosas@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Fix multifd qemu_mutex_destroy race | expand |
On Fri, Nov 10, 2023 at 05:02:39PM -0300, Fabiano Rosas wrote: > @@ -826,7 +832,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 = g_new0(QemuThread, 1); > + qemu_thread_create(p->tls_thread, "multifd-tls-handshake-worker", > multifd_tls_handshake_thread, p, > QEMU_THREAD_JOINABLE); I understand your way of doing this, but personally I prefer bool and make QemuThread not a pointer but still statically allocated. Same comment to the next patch. IMHO we can even add support for QemuThread in general to remember the bool itself: struct QemuThread { pthread_t thread; bool thread_created; }; Changing qemu_thread_create() to set the bool, and join() to skip the real join if it's not even created; clearing the bool otherwise after join()ed. I _think_ it'll work transparently to existing callers, but start to allow join() to be bypassed if thread not even created. Thanks,
diff --git a/migration/multifd.c b/migration/multifd.c index 409460684f..d632dbc095 100644 --- a/migration/multifd.c +++ b/migration/multifd.c @@ -525,6 +525,10 @@ void multifd_save_cleanup(void) for (i = 0; i < migrate_multifd_channels(); i++) { MultiFDSendParams *p = &multifd_send_state->params[i]; + if (p->tls_thread) { + qemu_thread_join(p->tls_thread); + } + if (p->running) { qemu_thread_join(&p->thread); } @@ -552,6 +556,8 @@ void multifd_save_cleanup(void) p->iov = NULL; g_free(p->normal); p->normal = NULL; + g_free(p->tls_thread); + p->tls_thread = NULL; multifd_send_state->ops->send_cleanup(p, &local_err); if (local_err) { migrate_set_error(migrate_get_current(), local_err); @@ -826,7 +832,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 = g_new0(QemuThread, 1); + 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 a835643b48..4ff78e9863 100644 --- a/migration/multifd.h +++ b/migration/multifd.h @@ -75,6 +75,7 @@ typedef struct { char *name; /* channel thread id */ QemuThread thread; + QemuThread *tls_thread; /* communication channel */ QIOChannel *c; /* is the yank function registered */
We're currently leaking the resources of the TLS thread by not joining it and also overwriting the p->thread pointer altogether. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- migration/multifd.c | 10 +++++++++- migration/multifd.h | 1 + 2 files changed, 10 insertions(+), 1 deletion(-)