Re: [PATCH] multifd: Implement yank for multifd send side
Lukas Straub wrote: > When introducing yank functionality in the migration code I forgot > to cover the multifd send side. > > Signed-off-by: Lukas Straub > Tested-by: Leonardo Bras > Reviewed-by: Leonardo Bras Reviewed-by: Juan Quintela
Re: [PATCH] multifd: Implement yank for multifd send side
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
> Tested-by: Leonardo Bras
> Reviewed-by: Leonardo Bras
> ---
>
> -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
[PATCH] multifd: Implement yank for multifd send side
When introducing yank functionality in the migration code I forgot
to cover the multifd send side.
Signed-off-by: Lukas Straub
Tested-by: Leonardo Bras
Reviewed-by: Leonardo Bras
---
-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
pgpLt11uT5f9F.pgp
Description: OpenPGP digital signature
Re: [PATCH] multifd: Implement yank for multifd send side
On Mon, Aug 16, 2021 at 2:44 AM Leonardo Bras Soares Passos
wrote:
>
> Hello Lukas,
>
> On Wed, Aug 4, 2021 at 4:27 PM Lukas Straub wrote:
> >
> > When introducing yank functionality in the migration code I forgot
> > to cover the multifd send side.
> >
> > Signed-off-by: Lukas Straub
> > ---
> >
> > @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-leob...@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
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
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
Re: [PATCH] multifd: Implement yank for multifd send side
Hello Lukas,
On Wed, Aug 4, 2021 at 4:27 PM Lukas Straub wrote:
>
> When introducing yank functionality in the migration code I forgot
> to cover the multifd send side.
>
> Signed-off-by: Lukas Straub
> ---
>
> @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-leob...@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
[PATCH] multifd: Implement yank for multifd send side
When introducing yank functionality in the migration code I forgot
to cover the multifd send side.
Signed-off-by: Lukas Straub
---
@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
pgpDmXO7TErHE.pgp
Description: OpenPGP digital signature
