Re: [PATCH for 6.1] multifd: Unconditionally unregister yank function

[Lukas Straub]

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  
> 
> 
> 



--

pgpJ6X7TLuHKF.pgp
Description: OpenPGP digital signature