RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize

[Zhang, Chen]

> -----Original Message-----
> From: Lukas Straub <address@hidden>
> Sent: Friday, May 8, 2020 12:09 AM
> To: Zhang, Chen <address@hidden>
> Cc: qemu-devel <address@hidden>; Li Zhijian
> <address@hidden>; Jason Wang <address@hidden>; Marc-
> André Lureau <address@hidden>; Paolo Bonzini
> <address@hidden>
> Subject: Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> complete and finalize
> 
> On Thu, 7 May 2020 13:26:11 +0000
> "Zhang, Chen" <address@hidden> wrote:
> 
> > > -----Original Message-----
> > > From: Lukas Straub <address@hidden>
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel <address@hidden>
> > > Cc: Zhang, Chen <address@hidden>; Li Zhijian
> > > <address@hidden>; Jason Wang <address@hidden>; Marc-
> > > André Lureau <address@hidden>; Paolo Bonzini
> > > <address@hidden>
> > > Subject: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> > > complete and finalize
> > >
> > > In colo_compare_complete, insert CompareState into net_compares
> only
> > > after everything has been initialized.
> > > In colo_compare_finalize, remove CompareState from net_compares
> > > before anything is deinitialized.
> >
> > S/deinitialized/finalized
> >
> > It looks no dependences on each step on initialization and finalization.
> > Do you means we just need add/remove each colo-compare module at last
> in logic?
> 
> Yes. While I didn't see any crashes here, there is the possibility that if 
> colo-
> compare is removed during checkpoint, the destroyed event_bh is called
> from colo_notify_compares_event. Same with colo_compare_complete
> (very unlikely) if colo-compare is created while colo is running,
> colo_notify_compares_event may call the uninitialized event_bh.

As we discussed on 5/6, remove colo-compare during checkpoint is an illegal 
operation.

Thanks
Zhang Chen

> 
> Regards,
> Lukas Straub
> 
> > Or current code have some issue?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Signed-off-by: Lukas Straub <address@hidden>
> > > ---
> > >  net/colo-compare.c | 45
> > > +++++++++++++++++++++++----------------------
> > >  1 file changed, 23 insertions(+), 22 deletions(-)
> > >
> > > diff --git a/net/colo-compare.c b/net/colo-compare.c index
> > > c7572d75e9..6f80bcece6 100644
> > > --- a/net/colo-compare.c
> > > +++ b/net/colo-compare.c
> > > @@ -1283,15 +1283,6 @@ static void
> > > colo_compare_complete(UserCreatable *uc, Error **errp)
> > >                             s->vnet_hdr);
> > >      }
> > >
> > > -    qemu_mutex_lock(&colo_compare_mutex);
> > > -    if (!colo_compare_active) {
> > > -        qemu_mutex_init(&event_mtx);
> > > -        qemu_cond_init(&event_complete_cond);
> > > -        colo_compare_active = true;
> > > -    }
> > > -    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > -    qemu_mutex_unlock(&colo_compare_mutex);
> > > -
> > >      s->out_sendco.s = s;
> > >      s->out_sendco.chr = &s->chr_out;
> > >      s->out_sendco.notify_remote_frame = false; @@ -1312,6 +1303,16
> > > @@ static void colo_compare_complete(UserCreatable *uc, Error **errp)
> > >
> > > connection_destroy);
> > >
> > >      colo_compare_iothread(s);
> > > +
> > > +    qemu_mutex_lock(&colo_compare_mutex);
> > > +    if (!colo_compare_active) {
> > > +        qemu_mutex_init(&event_mtx);
> > > +        qemu_cond_init(&event_complete_cond);
> > > +        colo_compare_active = true;
> > > +    }
> > > +    QTAILQ_INSERT_TAIL(&net_compares, s, next);
> > > +    qemu_mutex_unlock(&colo_compare_mutex);
> > > +
> > >      return;
> > >  }
> > >
> > > @@ -1384,19 +1385,6 @@ static void colo_compare_finalize(Object *obj)
> > >      CompareState *s = COLO_COMPARE(obj);
> > >      CompareState *tmp = NULL;
> > >
> > > -    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > -    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > -    qemu_chr_fe_deinit(&s->chr_out, false);
> > > -    if (s->notify_dev) {
> > > -        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > -    }
> > > -
> > > -    if (s->iothread) {
> > > -        colo_compare_timer_del(s);
> > > -    }
> > > -
> > > -    qemu_bh_delete(s->event_bh);
> > > -
> > >      qemu_mutex_lock(&colo_compare_mutex);
> > >      QTAILQ_FOREACH(tmp, &net_compares, next) {
> > >          if (tmp == s) {
> > > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > >      }
> > >      qemu_mutex_unlock(&colo_compare_mutex);
> > >
> > > +    qemu_chr_fe_deinit(&s->chr_pri_in, false);
> > > +    qemu_chr_fe_deinit(&s->chr_sec_in, false);
> > > +    qemu_chr_fe_deinit(&s->chr_out, false);
> > > +    if (s->notify_dev) {
> > > +        qemu_chr_fe_deinit(&s->chr_notify_dev, false);
> > > +    }
> > > +
> > > +    if (s->iothread) {
> > > +        colo_compare_timer_del(s);
> > > +    }
> > > +
> > > +    qemu_bh_delete(s->event_bh);
> > > +
> > >      AioContext *ctx = iothread_get_aio_context(s->iothread);
> > >      aio_context_acquire(ctx);
> > >      AIO_WAIT_WHILE(ctx, !s->out_sendco.done);
> > > --
> > > 2.20.1