All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] multifd: Implement yank for multifd send side
@ 2021-09-01 15:58 Lukas Straub
  2021-09-01 17:57 ` Peter Xu
  2021-09-09  7:18 ` Juan Quintela
  0 siblings, 2 replies; 6+ messages in thread
From: Lukas Straub @ 2021-09-01 15:58 UTC (permalink / raw)
  To: qemu-devel
  Cc: Leonardo Bras Soares Passos, Dr. David Alan Gilbert, Peter Xu,
	Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 1809 bytes --]

When introducing yank functionality in the migration code I forgot
to cover the multifd send side.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Tested-by: Leonardo Bras <leobras@redhat.com>
Reviewed-by: Leonardo Bras <leobras@redhat.com>
---

-v2:
 -add Tested-by and Reviewed-by tags

 migration/multifd.c | 6 +++++-
 migration/multifd.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 377da78f5b..5a4f158f3c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
         MultiFDSendParams *p = &multifd_send_state->params[i];
         Error *local_err = NULL;
 
+        if (p->registered_yank) {
+            migration_ioc_unregister_yank(p->c);
+        }
         socket_send_channel_destroy(p->c);
         p->c = NULL;
         qemu_mutex_destroy(&p->mutex);
@@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
                 return false;
             }
         } else {
-            /* update for tls qio channel */
+            migration_ioc_register_yank(ioc);
+            p->registered_yank = true;
             p->c = ioc;
             qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
                                    QEMU_THREAD_JOINABLE);
diff --git a/migration/multifd.h b/migration/multifd.h
index 8d6751f5ed..16c4d112d1 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -85,6 +85,8 @@ typedef struct {
     bool running;
     /* should this thread finish */
     bool quit;
+    /* is the yank function registered */
+    bool registered_yank;
     /* thread has work to do */
     int pending_job;
     /* array of pages to sent */
-- 
2.32.0

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] multifd: Implement yank for multifd send side
  2021-09-01 15:58 [PATCH] multifd: Implement yank for multifd send side Lukas Straub
@ 2021-09-01 17:57 ` Peter Xu
  2021-09-09  7:18 ` Juan Quintela
  1 sibling, 0 replies; 6+ messages in thread
From: Peter Xu @ 2021-09-01 17:57 UTC (permalink / raw)
  To: Lukas Straub
  Cc: Juan Quintela, qemu-devel, Leonardo Bras Soares Passos,
	Dr. David Alan Gilbert

On Wed, Sep 01, 2021 at 05:58:57PM +0200, Lukas Straub wrote:
> When introducing yank functionality in the migration code I forgot
> to cover the multifd send side.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Tested-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Leonardo Bras <leobras@redhat.com>
> ---
> 
> -v2:
>  -add Tested-by and Reviewed-by tags
> 
>  migration/multifd.c | 6 +++++-
>  migration/multifd.h | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..5a4f158f3c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>          Error *local_err = NULL;
>  
> +        if (p->registered_yank) {
> +            migration_ioc_unregister_yank(p->c);
> +        }
>          socket_send_channel_destroy(p->c);
>          p->c = NULL;
>          qemu_mutex_destroy(&p->mutex);
> @@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>                  return false;
>              }
>          } else {
> -            /* update for tls qio channel */
> +            migration_ioc_register_yank(ioc);
> +            p->registered_yank = true;
>              p->c = ioc;
>              qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>                                     QEMU_THREAD_JOINABLE);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 8d6751f5ed..16c4d112d1 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -85,6 +85,8 @@ typedef struct {
>      bool running;
>      /* should this thread finish */
>      bool quit;
> +    /* is the yank function registered */
> +    bool registered_yank;
>      /* thread has work to do */
>      int pending_job;
>      /* array of pages to sent */
> -- 
> 2.32.0

This is probably yet another case that I'm wondering whether we made unregister
of yank function/instance too strict so we should loose them.

Logically a remove/unregister function doesn't need to assert and crash qemu if
the entry doesn't exist at all.  Then it's just something like "makes sure XXX
is removed", and noop if lookup failed.  The extra fields do not really help us
from anything else..

Thanks,

-- 
Peter Xu



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] multifd: Implement yank for multifd send side
  2021-09-01 15:58 [PATCH] multifd: Implement yank for multifd send side Lukas Straub
  2021-09-01 17:57 ` Peter Xu
@ 2021-09-09  7:18 ` Juan Quintela
  1 sibling, 0 replies; 6+ messages in thread
From: Juan Quintela @ 2021-09-09  7:18 UTC (permalink / raw)
  To: Lukas Straub
  Cc: Leonardo Bras Soares Passos, qemu-devel, Peter Xu,
	Dr. David Alan Gilbert

Lukas Straub <lukasstraub2@web.de> wrote:
> When introducing yank functionality in the migration code I forgot
> to cover the multifd send side.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> Tested-by: Leonardo Bras <leobras@redhat.com>
> Reviewed-by: Leonardo Bras <leobras@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] multifd: Implement yank for multifd send side
  2021-08-16  5:44 ` Leonardo Bras Soares Passos
@ 2021-08-17 17:55   ` Leonardo Bras Soares Passos
  0 siblings, 0 replies; 6+ messages in thread
From: Leonardo Bras Soares Passos @ 2021-08-17 17:55 UTC (permalink / raw)
  To: Lukas Straub
  Cc: Li Xiaohui, Juan Quintela, qemu-devel, Dr. David Alan Gilbert

On Mon, Aug 16, 2021 at 2:44 AM Leonardo Bras Soares Passos
<leobras@redhat.com> wrote:
>
> Hello Lukas,
>
> On Wed, Aug 4, 2021 at 4:27 PM Lukas Straub <lukasstraub2@web.de> wrote:
> >
> > When introducing yank functionality in the migration code I forgot
> > to cover the multifd send side.
> >
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >
> > @Leonardo Could you check if this fixes your issue?
>
> In the same scenario I was testing my previous patch,
> (http://patchwork.ozlabs.org/project/qemu-devel/patch/20210730074043.54260-1-leobras@redhat.com/)
> this patch also seems to fix the issue .
> (https://bugzilla.redhat.com/show_bug.cgi?id=1970337).

Regarding this single test:
Tested-by: Leonardo Bras <leobras@redhat.com>

I am by no means a yank or migration expert, but the change seems to
make sense based on what I could learn in the above BZ.

So, FWIW:
Reviewed-by: Leonardo Bras <leobras@redhat.com>

Although I think it would be great if a more experienced person could
also review.

Best regards,
Leonardo Bras



>
>
> >
> >  migration/multifd.c | 6 +++++-
> >  migration/multifd.h | 2 ++
> >  2 files changed, 7 insertions(+), 1 deletion(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 377da78f5b..5a4f158f3c 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
> >          MultiFDSendParams *p = &multifd_send_state->params[i];
> >          Error *local_err = NULL;
> >
> > +        if (p->registered_yank) {
> > +            migration_ioc_unregister_yank(p->c);
> > +        }
> >          socket_send_channel_destroy(p->c);
> >          p->c = NULL;
> >          qemu_mutex_destroy(&p->mutex);
> > @@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
> >                  return false;
> >              }
> >          } else {
> > -            /* update for tls qio channel */
> > +            migration_ioc_register_yank(ioc);
> > +            p->registered_yank = true;
> >              p->c = ioc;
> >              qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
> >                                     QEMU_THREAD_JOINABLE);
> > diff --git a/migration/multifd.h b/migration/multifd.h
> > index 8d6751f5ed..16c4d112d1 100644
> > --- a/migration/multifd.h
> > +++ b/migration/multifd.h
> > @@ -85,6 +85,8 @@ typedef struct {
> >      bool running;
> >      /* should this thread finish */
> >      bool quit;
> > +    /* is the yank function registered */
> > +    bool registered_yank;
> >      /* thread has work to do */
> >      int pending_job;
> >      /* array of pages to sent */
> > --
> > 2.32.0



^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] multifd: Implement yank for multifd send side
  2021-08-04 19:27 Lukas Straub
@ 2021-08-16  5:44 ` Leonardo Bras Soares Passos
  2021-08-17 17:55   ` Leonardo Bras Soares Passos
  0 siblings, 1 reply; 6+ messages in thread
From: Leonardo Bras Soares Passos @ 2021-08-16  5:44 UTC (permalink / raw)
  To: Lukas Straub
  Cc: Li Xiaohui, Juan Quintela, qemu-devel, Dr. David Alan Gilbert

Hello Lukas,

On Wed, Aug 4, 2021 at 4:27 PM Lukas Straub <lukasstraub2@web.de> wrote:
>
> When introducing yank functionality in the migration code I forgot
> to cover the multifd send side.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>
> @Leonardo Could you check if this fixes your issue?

In the same scenario I was testing my previous patch,
(http://patchwork.ozlabs.org/project/qemu-devel/patch/20210730074043.54260-1-leobras@redhat.com/)
this patch also seems to fix the issue .
(https://bugzilla.redhat.com/show_bug.cgi?id=1970337).


>
>  migration/multifd.c | 6 +++++-
>  migration/multifd.h | 2 ++
>  2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..5a4f158f3c 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
>          MultiFDSendParams *p = &multifd_send_state->params[i];
>          Error *local_err = NULL;
>
> +        if (p->registered_yank) {
> +            migration_ioc_unregister_yank(p->c);
> +        }
>          socket_send_channel_destroy(p->c);
>          p->c = NULL;
>          qemu_mutex_destroy(&p->mutex);
> @@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
>                  return false;
>              }
>          } else {
> -            /* update for tls qio channel */
> +            migration_ioc_register_yank(ioc);
> +            p->registered_yank = true;
>              p->c = ioc;
>              qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
>                                     QEMU_THREAD_JOINABLE);
> diff --git a/migration/multifd.h b/migration/multifd.h
> index 8d6751f5ed..16c4d112d1 100644
> --- a/migration/multifd.h
> +++ b/migration/multifd.h
> @@ -85,6 +85,8 @@ typedef struct {
>      bool running;
>      /* should this thread finish */
>      bool quit;
> +    /* is the yank function registered */
> +    bool registered_yank;
>      /* thread has work to do */
>      int pending_job;
>      /* array of pages to sent */
> --
> 2.32.0



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH] multifd: Implement yank for multifd send side
@ 2021-08-04 19:27 Lukas Straub
  2021-08-16  5:44 ` Leonardo Bras Soares Passos
  0 siblings, 1 reply; 6+ messages in thread
From: Lukas Straub @ 2021-08-04 19:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Li Xiaohui, Leonardo Bras Soares Passos, Dr. David Alan Gilbert,
	Juan Quintela

[-- Attachment #1: Type: text/plain, Size: 1722 bytes --]

When introducing yank functionality in the migration code I forgot
to cover the multifd send side.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---

@Leonardo Could you check if this fixes your issue?

 migration/multifd.c | 6 +++++-
 migration/multifd.h | 2 ++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 377da78f5b..5a4f158f3c 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -546,6 +546,9 @@ void multifd_save_cleanup(void)
         MultiFDSendParams *p = &multifd_send_state->params[i];
         Error *local_err = NULL;
 
+        if (p->registered_yank) {
+            migration_ioc_unregister_yank(p->c);
+        }
         socket_send_channel_destroy(p->c);
         p->c = NULL;
         qemu_mutex_destroy(&p->mutex);
@@ -813,7 +816,8 @@ static bool multifd_channel_connect(MultiFDSendParams *p,
                 return false;
             }
         } else {
-            /* update for tls qio channel */
+            migration_ioc_register_yank(ioc);
+            p->registered_yank = true;
             p->c = ioc;
             qemu_thread_create(&p->thread, p->name, multifd_send_thread, p,
                                    QEMU_THREAD_JOINABLE);
diff --git a/migration/multifd.h b/migration/multifd.h
index 8d6751f5ed..16c4d112d1 100644
--- a/migration/multifd.h
+++ b/migration/multifd.h
@@ -85,6 +85,8 @@ typedef struct {
     bool running;
     /* should this thread finish */
     bool quit;
+    /* is the yank function registered */
+    bool registered_yank;
     /* thread has work to do */
     int pending_job;
     /* array of pages to sent */
-- 
2.32.0

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-09-09  7:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 15:58 [PATCH] multifd: Implement yank for multifd send side Lukas Straub
2021-09-01 17:57 ` Peter Xu
2021-09-09  7:18 ` Juan Quintela
  -- strict thread matches above, loose matches on Subject: below --
2021-08-04 19:27 Lukas Straub
2021-08-16  5:44 ` Leonardo Bras Soares Passos
2021-08-17 17:55   ` Leonardo Bras Soares Passos

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.