| Message ID | ac784f17537184c3ab8c745a1d593b02bde85738.1588587700.git.lukasstraub2@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | colo-compare bugfixes | expand |
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Monday, May 4, 2020 6:28 PM > To: qemu-devel <qemu-devel@nongnu.org> > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini > <pbonzini@redhat.com> > 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? Or current code have some issue? Thanks Zhang Chen > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > --- > 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
On Thu, 7 May 2020 13:26:11 +0000 "Zhang, Chen" <chen.zhang@intel.com> wrote: > > -----Original Message----- > > From: Lukas Straub <lukasstraub2@web.de> > > Sent: Monday, May 4, 2020 6:28 PM > > To: qemu-devel <qemu-devel@nongnu.org> > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini > > <pbonzini@redhat.com> > > 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 <lukasstraub2@web.de> > > --- > > 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
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Friday, May 8, 2020 12:09 AM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini > <pbonzini@redhat.com> > 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" <chen.zhang@intel.com> wrote: > > > > -----Original Message----- > > > From: Lukas Straub <lukasstraub2@web.de> > > > Sent: Monday, May 4, 2020 6:28 PM > > > To: qemu-devel <qemu-devel@nongnu.org> > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- > > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini > > > <pbonzini@redhat.com> > > > 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 <lukasstraub2@web.de> > > > --- > > > 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
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Friday, May 8, 2020 12:09 AM > To: Zhang, Chen <chen.zhang@intel.com> > Cc: qemu-devel <qemu-devel@nongnu.org>; Li Zhijian > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini > <pbonzini@redhat.com> > 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" <chen.zhang@intel.com> wrote: > > > > -----Original Message----- > > > From: Lukas Straub <lukasstraub2@web.de> > > > Sent: Monday, May 4, 2020 6:28 PM > > > To: qemu-devel <qemu-devel@nongnu.org> > > > Cc: Zhang, Chen <chen.zhang@intel.com>; Li Zhijian > > > <lizhijian@cn.fujitsu.com>; Jason Wang <jasowang@redhat.com>; Marc- > > > André Lureau <marcandre.lureau@redhat.com>; Paolo Bonzini > > > <pbonzini@redhat.com> > > > 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. > Reviewed-by: Zhang Chen <chen.zhang@intel.com> Thanks Zhang Chen > Regards, > Lukas Straub > > > Or current code have some issue? > > > > Thanks > > Zhang Chen > > > > > > > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > > > --- > > > 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
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);
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. Signed-off-by: Lukas Straub <lukasstraub2@web.de> --- net/colo-compare.c | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-)