Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
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(). 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 Reviewed-by: Juan Quintela
Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
On Wed, 4 Aug 2021 21:26:32 +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(). 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 > --- > > This is similar to Peter Xu's > 39675b3394d44b880d083a214c5e44786170 > "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... -- pgpdfLxCe6G_v.pgp Description: OpenPGP digital signature
Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
On Wed, 4 Aug 2021 15:39:55 -0400 Peter Xu 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 > > --- > > > > This is similar to Peter Xu's > > 39675b3394d44b880d083a214c5e44786170 > > "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 > > > -- pgpdhmp4PD2Xh.pgp Description: OpenPGP digital signature
Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function
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 > --- > > This is similar to Peter Xu's > 39675b3394d44b880d083a214c5e44786170 > "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
[PATCH for 6.1] multifd: Unconditionally unregister yank function
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 --- This is similar to Peter Xu's 39675b3394d44b880d083a214c5e44786170 "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 pgp1ecaqbRLj8.pgp Description: OpenPGP digital signature