* [PATCH for 6.1] multifd: Unconditionally unregister yank function
@ 2021-08-04 19:26 Lukas Straub
2021-08-04 19:39 ` Peter Xu
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Lukas Straub @ 2021-08-04 19:26 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: 1433 bytes --]
Unconditionally unregister yank function in multifd_load_cleanup().
If it is not unregistered here, it will leak and cause a crash
in yank_unregister_instance(). Now if the ioc is still in use
afterwards, it will only lead to qemu not being able to recover
from a hang related to that ioc.
After checking the code, i am pretty sure that ref is always 1
when arriving here. So all this currently does is remove the
unneeded check.
Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
This is similar to Peter Xu's
39675ffffb3394d44b880d083a214c5e44786170
"migration: Move the yank unregister of channel_close out"
in that it removes the "OBJECT(p->c)->ref == 1" hack. So it
makes sense for 6.1 so these patches are together.
migration/multifd.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/migration/multifd.c b/migration/multifd.c
index 377da78f5b..a37805e17e 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -987,10 +987,7 @@ int multifd_load_cleanup(Error **errp)
for (i = 0; i < migrate_multifd_channels(); i++) {
MultiFDRecvParams *p = &multifd_recv_state->params[i];
- if (OBJECT(p->c)->ref == 1) {
- migration_ioc_unregister_yank(p->c);
- }
-
+ migration_ioc_unregister_yank(p->c);
object_unref(OBJECT(p->c));
p->c = NULL;
qemu_mutex_destroy(&p->mutex);
--
2.32.0
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
2021-08-04 19:26 [PATCH for 6.1] multifd: Unconditionally unregister yank function Lukas Straub
@ 2021-08-04 19:39 ` Peter Xu
2021-08-04 20:13 ` Lukas Straub
2021-09-01 15:48 ` Lukas Straub
2021-09-09 6:34 ` Juan Quintela
2 siblings, 1 reply; 5+ messages in thread
From: Peter Xu @ 2021-08-04 19:39 UTC (permalink / raw)
To: Lukas Straub
Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Leonardo Bras Soares Passos
On Wed, Aug 04, 2021 at 09:26:32PM +0200, Lukas Straub wrote:
> Unconditionally unregister yank function in multifd_load_cleanup().
> If it is not unregistered here, it will leak and cause a crash
> in yank_unregister_instance().
Curious whether there was a crash somewhere that you hit?
> Now if the ioc is still in use
> afterwards, it will only lead to qemu not being able to recover
> from a hang related to that ioc.
>
> After checking the code, i am pretty sure that ref is always 1
> when arriving here. So all this currently does is remove the
> unneeded check.
Thanks for checking and removing the 2nd ref==1. I wanted to do that but I
didn't dare before I look more closely or test more.
The patch looks good to me, it's just that I am not sure whether it suites 6.1
material as we've just released rc2 today. Maybe 6.2 is more suitable?
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>
> This is similar to Peter Xu's
> 39675ffffb3394d44b880d083a214c5e44786170
> "migration: Move the yank unregister of channel_close out"
> in that it removes the "OBJECT(p->c)->ref == 1" hack. So it
> makes sense for 6.1 so these patches are together.
>
> migration/multifd.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..a37805e17e 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,10 +987,7 @@ int multifd_load_cleanup(Error **errp)
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDRecvParams *p = &multifd_recv_state->params[i];
>
> - if (OBJECT(p->c)->ref == 1) {
> - migration_ioc_unregister_yank(p->c);
> - }
> -
> + migration_ioc_unregister_yank(p->c);
> object_unref(OBJECT(p->c));
> p->c = NULL;
> qemu_mutex_destroy(&p->mutex);
> --
> 2.32.0
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
2021-08-04 19:39 ` Peter Xu
@ 2021-08-04 20:13 ` Lukas Straub
0 siblings, 0 replies; 5+ messages in thread
From: Lukas Straub @ 2021-08-04 20:13 UTC (permalink / raw)
To: Peter Xu
Cc: Juan Quintela, qemu-devel, Dr. David Alan Gilbert,
Leonardo Bras Soares Passos
[-- Attachment #1: Type: text/plain, Size: 2273 bytes --]
On Wed, 4 Aug 2021 15:39:55 -0400
Peter Xu <peterx@redhat.com> wrote:
> On Wed, Aug 04, 2021 at 09:26:32PM +0200, Lukas Straub wrote:
> > Unconditionally unregister yank function in multifd_load_cleanup().
> > If it is not unregistered here, it will leak and cause a crash
> > in yank_unregister_instance().
>
> Curious whether there was a crash somewhere that you hit?
No, I just noticed this while working on a different patch.
> > Now if the ioc is still in use
> > afterwards, it will only lead to qemu not being able to recover
> > from a hang related to that ioc.
> >
> > After checking the code, i am pretty sure that ref is always 1
> > when arriving here. So all this currently does is remove the
> > unneeded check.
>
> Thanks for checking and removing the 2nd ref==1. I wanted to do that but I
> didn't dare before I look more closely or test more.
>
> The patch looks good to me, it's just that I am not sure whether it suites 6.1
> material as we've just released rc2 today. Maybe 6.2 is more suitable?
Yeah, if my assessment of the code is correct it's more of a cleanup.
> >
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >
> > This is similar to Peter Xu's
> > 39675ffffb3394d44b880d083a214c5e44786170
> > "migration: Move the yank unregister of channel_close out"
> > in that it removes the "OBJECT(p->c)->ref == 1" hack. So it
> > makes sense for 6.1 so these patches are together.
> >
> > migration/multifd.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 377da78f5b..a37805e17e 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -987,10 +987,7 @@ int multifd_load_cleanup(Error **errp)
> > for (i = 0; i < migrate_multifd_channels(); i++) {
> > MultiFDRecvParams *p = &multifd_recv_state->params[i];
> >
> > - if (OBJECT(p->c)->ref == 1) {
> > - migration_ioc_unregister_yank(p->c);
> > - }
> > -
> > + migration_ioc_unregister_yank(p->c);
> > object_unref(OBJECT(p->c));
> > p->c = NULL;
> > qemu_mutex_destroy(&p->mutex);
> > --
> > 2.32.0
>
>
>
--
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
2021-08-04 19:26 [PATCH for 6.1] multifd: Unconditionally unregister yank function Lukas Straub
2021-08-04 19:39 ` Peter Xu
@ 2021-09-01 15:48 ` Lukas Straub
2021-09-09 6:34 ` Juan Quintela
2 siblings, 0 replies; 5+ messages in thread
From: Lukas Straub @ 2021-09-01 15:48 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: 1596 bytes --]
On Wed, 4 Aug 2021 21:26:32 +0200
Lukas Straub <lukasstraub2@web.de> wrote:
> Unconditionally unregister yank function in multifd_load_cleanup().
> If it is not unregistered here, it will leak and cause a crash
> in yank_unregister_instance(). Now if the ioc is still in use
> afterwards, it will only lead to qemu not being able to recover
> from a hang related to that ioc.
>
> After checking the code, i am pretty sure that ref is always 1
> when arriving here. So all this currently does is remove the
> unneeded check.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>
> This is similar to Peter Xu's
> 39675ffffb3394d44b880d083a214c5e44786170
> "migration: Move the yank unregister of channel_close out"
> in that it removes the "OBJECT(p->c)->ref == 1" hack. So it
> makes sense for 6.1 so these patches are together.
>
> migration/multifd.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 377da78f5b..a37805e17e 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -987,10 +987,7 @@ int multifd_load_cleanup(Error **errp)
> for (i = 0; i < migrate_multifd_channels(); i++) {
> MultiFDRecvParams *p = &multifd_recv_state->params[i];
>
> - if (OBJECT(p->c)->ref == 1) {
> - migration_ioc_unregister_yank(p->c);
> - }
> -
> + migration_ioc_unregister_yank(p->c);
> object_unref(OBJECT(p->c));
> p->c = NULL;
> qemu_mutex_destroy(&p->mutex);
ping...
--
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
2021-08-04 19:26 [PATCH for 6.1] multifd: Unconditionally unregister yank function Lukas Straub
2021-08-04 19:39 ` Peter Xu
2021-09-01 15:48 ` Lukas Straub
@ 2021-09-09 6:34 ` Juan Quintela
2 siblings, 0 replies; 5+ messages in thread
From: Juan Quintela @ 2021-09-09 6:34 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:
> Unconditionally unregister yank function in multifd_load_cleanup().
> If it is not unregistered here, it will leak and cause a crash
> in yank_unregister_instance(). Now if the ioc is still in use
> afterwards, it will only lead to qemu not being able to recover
> from a hang related to that ioc.
>
> After checking the code, i am pretty sure that ref is always 1
> when arriving here. So all this currently does is remove the
> unneeded check.
>
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
Reviewed-by: Juan Quintela <quintela@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-09 7:27 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04 19:26 [PATCH for 6.1] multifd: Unconditionally unregister yank function Lukas Straub
2021-08-04 19:39 ` Peter Xu
2021-08-04 20:13 ` Lukas Straub
2021-09-01 15:48 ` Lukas Straub
2021-09-09 6:34 ` Juan Quintela
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.