1. Patchew
  2. QEMU
  3. [PATCH 0/2] migration: cleanup TLS channel referencing
  4. View patch

[PATCH 2/2] migration/multifd: Drop registered_yank

peterx@redhat.com posted 2 patches 2 years ago
Maintainers: Peter Xu <peterx@redhat.com>, Fabiano Rosas <farosas@suse.de>
There is a newer version of this series
[PATCH 2/2] migration/multifd: Drop registered_yank
Posted by peterx@redhat.com 2 years ago 
From: Peter Xu <peterx@redhat.com>

With a clear definition of p->c protocol, where we only set it up if the
channel is fully established (TLS or non-TLS), registered_yank boolean will
have equal meaning of "p->c != NULL".

Drop registered_yank by checking p->c instead.

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.h | 2 --
 migration/multifd.c | 7 +++----
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/migration/multifd.h b/migration/multifd.h
index 8a1cad0996..b3fe27ae93 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -78,8 +78,6 @@ typedef struct {
     bool tls_thread_created;
     /* communication channel */
     QIOChannel *c;
-    /* is the yank function registered */
-    bool registered_yank;
     /* packet allocated len */
     uint32_t packet_len;
     /* guest page size */
diff --git a/migration/multifd.c b/migration/multifd.c
index 4a85a6b7b3..278453cf84 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
 
 static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
 {
-    if (p->registered_yank) {
+    if (p->c) {
         migration_ioc_unregister_yank(p->c);
+        multifd_send_channel_destroy(p->c);
+        p->c = NULL;
     }
-    multifd_send_channel_destroy(p->c);
-    p->c = NULL;
     qemu_sem_destroy(&p->sem);
     qemu_sem_destroy(&p->sem_sync);
     g_free(p->name);
@@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
     qio_channel_set_delay(ioc, false);
 
     migration_ioc_register_yank(ioc);
-    p->registered_yank = true;
     /* Setup p->c only if the channel is completely setup */
     p->c = ioc;
 
-- 
2.43.0
Re: [PATCH 2/2] migration/multifd: Drop registered_yank
Posted by Fabiano Rosas 2 years ago 
peterx@redhat.com writes:

> From: Peter Xu <peterx@redhat.com>
>
> With a clear definition of p->c protocol, where we only set it up if the
> channel is fully established (TLS or non-TLS), registered_yank boolean will
> have equal meaning of "p->c != NULL".
>
> Drop registered_yank by checking p->c instead.
>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/multifd.h | 2 --
>  migration/multifd.c | 7 +++----
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 8a1cad0996..b3fe27ae93 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -78,8 +78,6 @@ typedef struct {
>      bool tls_thread_created;
>      /* communication channel */
>      QIOChannel *c;
> -    /* is the yank function registered */
> -    bool registered_yank;
>      /* packet allocated len */
>      uint32_t packet_len;
>      /* guest page size */
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4a85a6b7b3..278453cf84 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
>  
>  static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>  {
> -    if (p->registered_yank) {
> +    if (p->c) {
>          migration_ioc_unregister_yank(p->c);
> +        multifd_send_channel_destroy(p->c);

At socket_send_channel_destroy the clean up of outgoing_args.saddr will
now be skipped. The failure at multifd_new_send_channel_async might have
been due to TLS, in which case all of plain socket setup will have
happened properly.

> +        p->c = NULL;
>      }
> -    multifd_send_channel_destroy(p->c);
> -    p->c = NULL;
>      qemu_sem_destroy(&p->sem);
>      qemu_sem_destroy(&p->sem_sync);
>      g_free(p->name);
> @@ -932,7 +932,6 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>      qio_channel_set_delay(ioc, false);
>  
>      migration_ioc_register_yank(ioc);
> -    p->registered_yank = true;
>      /* Setup p->c only if the channel is completely setup */
>      p->c = ioc;
Re: [PATCH 2/2] migration/multifd: Drop registered_yank
Posted by Peter Xu 1 year, 11 months ago 
On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote:
> peterx@redhat.com writes:
> 
> > From: Peter Xu <peterx@redhat.com>
> >
> > With a clear definition of p->c protocol, where we only set it up if the
> > channel is fully established (TLS or non-TLS), registered_yank boolean will
> > have equal meaning of "p->c != NULL".
> >
> > Drop registered_yank by checking p->c instead.
> >
> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/multifd.h | 2 --
> >  migration/multifd.c | 7 +++----
> >  2 files changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 8a1cad0996..b3fe27ae93 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -78,8 +78,6 @@ typedef struct {
> >      bool tls_thread_created;
> >      /* communication channel */
> >      QIOChannel *c;
> > -    /* is the yank function registered */
> > -    bool registered_yank;
> >      /* packet allocated len */
> >      uint32_t packet_len;
> >      /* guest page size */
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 4a85a6b7b3..278453cf84 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
> >  
> >  static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
> >  {
> > -    if (p->registered_yank) {
> > +    if (p->c) {
> >          migration_ioc_unregister_yank(p->c);
> > +        multifd_send_channel_destroy(p->c);
> 
> At socket_send_channel_destroy the clean up of outgoing_args.saddr will
> now be skipped. The failure at multifd_new_send_channel_async might have
> been due to TLS, in which case all of plain socket setup will have
> happened properly.

Right, IMHO it's a hack to free globals in a per-channel helper.  We should
have moved:

    if (outgoing_args.saddr) {
        qapi_free_SocketAddress(outgoing_args.saddr);
        outgoing_args.saddr = NULL;
    }

Outside irrelevant of that..

That could be done later I guess, because we have one more guard:

socket_start_outgoing_migration():
    /* in case previous migration leaked it */
    qapi_free_SocketAddress(outgoing_args.saddr);
    outgoing_args.saddr = addr;

If you think proper, I can add one more patch to do that cleanup, IOW, move
above free() into multifd_send_cleanup_state().

Thanks,

-- 
Peter Xu
Re: [PATCH 2/2] migration/multifd: Drop registered_yank
Posted by Fabiano Rosas 1 year, 11 months ago 
Peter Xu <peterx@redhat.com> writes:

> On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote:
>> peterx@redhat.com writes:
>> 
>> > From: Peter Xu <peterx@redhat.com>
>> >
>> > With a clear definition of p->c protocol, where we only set it up if the
>> > channel is fully established (TLS or non-TLS), registered_yank boolean will
>> > have equal meaning of "p->c != NULL".
>> >
>> > Drop registered_yank by checking p->c instead.
>> >
>> > Reviewed-by: Fabiano Rosas <farosas@suse.de>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  migration/multifd.h | 2 --
>> >  migration/multifd.c | 7 +++----
>> >  2 files changed, 3 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/migration/multifd.h b/migration/multifd.h
>> > index 8a1cad0996..b3fe27ae93 100644
>> > --- a/migration/multifd.h
>> > +++ b/migration/multifd.h
>> > @@ -78,8 +78,6 @@ typedef struct {
>> >      bool tls_thread_created;
>> >      /* communication channel */
>> >      QIOChannel *c;
>> > -    /* is the yank function registered */
>> > -    bool registered_yank;
>> >      /* packet allocated len */
>> >      uint32_t packet_len;
>> >      /* guest page size */
>> > diff --git a/migration/multifd.c b/migration/multifd.c
>> > index 4a85a6b7b3..278453cf84 100644
>> > --- a/migration/multifd.c
>> > +++ b/migration/multifd.c
>> > @@ -648,11 +648,11 @@ static int multifd_send_channel_destroy(QIOChannel *send)
>> >  
>> >  static bool multifd_send_cleanup_channel(MultiFDSendParams *p, Error **errp)
>> >  {
>> > -    if (p->registered_yank) {
>> > +    if (p->c) {
>> >          migration_ioc_unregister_yank(p->c);
>> > +        multifd_send_channel_destroy(p->c);
>> 
>> At socket_send_channel_destroy the clean up of outgoing_args.saddr will
>> now be skipped. The failure at multifd_new_send_channel_async might have
>> been due to TLS, in which case all of plain socket setup will have
>> happened properly.
>
> Right, IMHO it's a hack to free globals in a per-channel helper.  We should
> have moved:
>
>     if (outgoing_args.saddr) {
>         qapi_free_SocketAddress(outgoing_args.saddr);
>         outgoing_args.saddr = NULL;
>     }
>
> Outside irrelevant of that..
>
> That could be done later I guess, because we have one more guard:
>
> socket_start_outgoing_migration():
>     /* in case previous migration leaked it */
>     qapi_free_SocketAddress(outgoing_args.saddr);
>     outgoing_args.saddr = addr;
>
> If you think proper, I can add one more patch to do that cleanup, IOW, move
> above free() into multifd_send_cleanup_state().

Sounds good.

>
> Thanks,

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.