Re: [PATCH 2/2] migration/multifd: Drop registered_yank
Peter Xu writes:
> On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote:
>> pet...@redhat.com writes:
>>
>> > From: Peter Xu
>> >
>> > 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
>> > Signed-off-by: Peter Xu
>> > ---
>> > 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,
Re: [PATCH 2/2] migration/multifd: Drop registered_yank
On Thu, Feb 08, 2024 at 09:48:33AM -0300, Fabiano Rosas wrote:
> pet...@redhat.com writes:
>
> > From: Peter Xu
> >
> > 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
> > Signed-off-by: Peter Xu
> > ---
> > 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
pet...@redhat.com writes:
> From: Peter Xu
>
> 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
> Signed-off-by: Peter Xu
> ---
> 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(>sem);
> qemu_sem_destroy(>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;
[PATCH 2/2] migration/multifd: Drop registered_yank
From: Peter Xu
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
Signed-off-by: Peter Xu
---
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(>sem);
qemu_sem_destroy(>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
