On Thu, Feb 08, 2024 at 04:10:58PM +0200, Avihai Horon wrote:
>
> On 08/02/2024 5:51, pet...@redhat.com wrote:
> > External email: Use caution opening links or attachments
> >
> >
> > From: Peter Xu <pet...@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 <pet...@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.
Yes we can. I'll add a patch and fix the spell, thanks.
--
Peter Xu
