[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and
From: |
Lukas Straub |
Subject: |
Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize |
Date: |
Thu, 7 May 2020 18:09:09 +0200 |
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.
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
pgpTG_TurY4Kp.pgp
Description: OpenPGP digital signature
- Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, (continued)
- Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Lukas Straub, 2020/05/07
- RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Zhang, Chen, 2020/05/07
- Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Derek Su, 2020/05/07
- Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Lukas Straub, 2020/05/08
- RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Zhang, Chen, 2020/05/08
- Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Lukas Straub, 2020/05/09
- RE: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Zhang, Chen, 2020/05/11
- Re: [PATCH v4 5/6] net/colo-compare.c, softmmu/vl.c: Check that colo-compare is active, Dr. David Alan Gilbert, 2020/05/12
[PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize, Lukas Straub, 2020/05/04
RE: [PATCH v4 0/6] colo-compare bugfixes, Zhang, Chen, 2020/05/15