| Message ID | 20210804212632.77221bcf@gecko.fritz.box (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for,6.1] multifd: Unconditionally unregister yank function | expand |
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
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 > > > --
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... --
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>
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);
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(-)