RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize
> -Original Message-
> From: Lukas Straub
> Sent: Friday, May 8, 2020 12:09 AM
> To: Zhang, Chen
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> Subject: Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> complete and finalize
>
> On Thu, 7 May 2020 13:26:11 +
> "Zhang, Chen" wrote:
>
> > > -Original Message-
> > > From: Lukas Straub
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel
> > > Cc: Zhang, Chen ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > >
> > > 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
Thanks
Zhang Chen
> Regards,
> Lukas Straub
>
> > Or current code have some issue?
> >
> > Thanks
> > Zhang Chen
> >
> > >
> > > Signed-off-by: Lukas Straub
> > > ---
> > > 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(_compare_mutex);
> > > -if (!colo_compare_active) {
> > > -qemu_mutex_init(_mtx);
> > > -qemu_cond_init(_complete_cond);
> > > -colo_compare_active = true;
> > > -}
> > > -QTAILQ_INSERT_TAIL(_compares, s, next);
> > > -qemu_mutex_unlock(_compare_mutex);
> > > -
> > > s->out_sendco.s = s;
> > > s->out_sendco.chr = >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(_compare_mutex);
> > > +if (!colo_compare_active) {
> > > +qemu_mutex_init(_mtx);
> > > +qemu_cond_init(_complete_cond);
> > > +colo_compare_active = true;
> > > +}
> > > +QTAILQ_INSERT_TAIL(_compares, s, next);
> > > +qemu_mutex_unlock(_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(>chr_pri_in, false);
> > > -qemu_chr_fe_deinit(>chr_sec_in, false);
> > > -qemu_chr_fe_deinit(>chr_out, false);
> > > -if (s->notify_dev) {
> > > -qemu_chr_fe_deinit(>chr_notify_dev, false);
> > > -}
> > > -
> > > -if (s->iothread) {
> > > -colo_compare_timer_del(s);
> > > -}
> > > -
> > > -qemu_bh_delete(s->event_bh);
> > > -
> > > qemu_mutex_lock(_compare_mutex);
> > > QTAILQ_FOREACH(tmp, _compares, next) {
> > > if (tmp == s) {
> > > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > > }
> > > qemu_mutex_unlock(_compare_mutex);
> > >
> > > +qemu_chr_fe_deinit(>chr_pri_in, false);
> > > +qemu_chr_fe_deinit(>chr_sec_in, false);
> > > +qemu_chr_fe_deinit(>chr_out, false);
> > > +if (s->notify_dev) {
> > > +qemu_chr_fe_deinit(>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
RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize
> -Original Message-
> From: Lukas Straub
> Sent: Friday, May 8, 2020 12:09 AM
> To: Zhang, Chen
> Cc: qemu-devel ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> Subject: Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in
> complete and finalize
>
> On Thu, 7 May 2020 13:26:11 +
> "Zhang, Chen" wrote:
>
> > > -Original Message-
> > > From: Lukas Straub
> > > Sent: Monday, May 4, 2020 6:28 PM
> > > To: qemu-devel
> > > Cc: Zhang, Chen ; Li Zhijian
> > > ; Jason Wang ; Marc-
> > > André Lureau ; Paolo Bonzini
> > >
> > > 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
> > > ---
> > > 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(_compare_mutex);
> > > -if (!colo_compare_active) {
> > > -qemu_mutex_init(_mtx);
> > > -qemu_cond_init(_complete_cond);
> > > -colo_compare_active = true;
> > > -}
> > > -QTAILQ_INSERT_TAIL(_compares, s, next);
> > > -qemu_mutex_unlock(_compare_mutex);
> > > -
> > > s->out_sendco.s = s;
> > > s->out_sendco.chr = >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(_compare_mutex);
> > > +if (!colo_compare_active) {
> > > +qemu_mutex_init(_mtx);
> > > +qemu_cond_init(_complete_cond);
> > > +colo_compare_active = true;
> > > +}
> > > +QTAILQ_INSERT_TAIL(_compares, s, next);
> > > +qemu_mutex_unlock(_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(>chr_pri_in, false);
> > > -qemu_chr_fe_deinit(>chr_sec_in, false);
> > > -qemu_chr_fe_deinit(>chr_out, false);
> > > -if (s->notify_dev) {
> > > -qemu_chr_fe_deinit(>chr_notify_dev, false);
> > > -}
> > > -
> > > -if (s->iothread) {
> > > -colo_compare_timer_del(s);
> > > -}
> > > -
> > > -qemu_bh_delete(s->event_bh);
> > > -
> > > qemu_mutex_lock(_compare_mutex);
> > > QTAILQ_FOREACH(tmp, _compares, next) {
> > > if (tmp == s) {
> > > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > > }
> > > qemu_mutex_unlock(_compare_mutex);
> > >
> > > +qemu_chr_fe_deinit(>chr_pri_in, false);
> > > +qemu_chr_fe_deinit(>chr_sec_in, false);
> > > +qemu_chr_fe_deinit(>chr_out, false);
> > > +if (s->notify_dev) {
> > > +qemu_chr_fe_deinit(>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
Re: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize
On Thu, 7 May 2020 13:26:11 +
"Zhang, Chen" wrote:
> > -Original Message-
> > From: Lukas Straub
> > Sent: Monday, May 4, 2020 6:28 PM
> > To: qemu-devel
> > Cc: Zhang, Chen ; Li Zhijian
> > ; Jason Wang ; Marc-
> > André Lureau ; Paolo Bonzini
> >
> > 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
> > ---
> > 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(_compare_mutex);
> > -if (!colo_compare_active) {
> > -qemu_mutex_init(_mtx);
> > -qemu_cond_init(_complete_cond);
> > -colo_compare_active = true;
> > -}
> > -QTAILQ_INSERT_TAIL(_compares, s, next);
> > -qemu_mutex_unlock(_compare_mutex);
> > -
> > s->out_sendco.s = s;
> > s->out_sendco.chr = >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(_compare_mutex);
> > +if (!colo_compare_active) {
> > +qemu_mutex_init(_mtx);
> > +qemu_cond_init(_complete_cond);
> > +colo_compare_active = true;
> > +}
> > +QTAILQ_INSERT_TAIL(_compares, s, next);
> > +qemu_mutex_unlock(_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(>chr_pri_in, false);
> > -qemu_chr_fe_deinit(>chr_sec_in, false);
> > -qemu_chr_fe_deinit(>chr_out, false);
> > -if (s->notify_dev) {
> > -qemu_chr_fe_deinit(>chr_notify_dev, false);
> > -}
> > -
> > -if (s->iothread) {
> > -colo_compare_timer_del(s);
> > -}
> > -
> > -qemu_bh_delete(s->event_bh);
> > -
> > qemu_mutex_lock(_compare_mutex);
> > QTAILQ_FOREACH(tmp, _compares, next) {
> > if (tmp == s) {
> > @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> > }
> > qemu_mutex_unlock(_compare_mutex);
> >
> > +qemu_chr_fe_deinit(>chr_pri_in, false);
> > +qemu_chr_fe_deinit(>chr_sec_in, false);
> > +qemu_chr_fe_deinit(>chr_out, false);
> > +if (s->notify_dev) {
> > +qemu_chr_fe_deinit(>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
pgpTILdNe6FCb.pgp
Description: OpenPGP digital signature
RE: [PATCH v4 6/6] net/colo-compare.c: Correct ordering in complete and finalize
> -Original Message-
> From: Lukas Straub
> Sent: Monday, May 4, 2020 6:28 PM
> To: qemu-devel
> Cc: Zhang, Chen ; Li Zhijian
> ; Jason Wang ; Marc-
> André Lureau ; Paolo Bonzini
>
> 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
> ---
> 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(_compare_mutex);
> -if (!colo_compare_active) {
> -qemu_mutex_init(_mtx);
> -qemu_cond_init(_complete_cond);
> -colo_compare_active = true;
> -}
> -QTAILQ_INSERT_TAIL(_compares, s, next);
> -qemu_mutex_unlock(_compare_mutex);
> -
> s->out_sendco.s = s;
> s->out_sendco.chr = >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(_compare_mutex);
> +if (!colo_compare_active) {
> +qemu_mutex_init(_mtx);
> +qemu_cond_init(_complete_cond);
> +colo_compare_active = true;
> +}
> +QTAILQ_INSERT_TAIL(_compares, s, next);
> +qemu_mutex_unlock(_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(>chr_pri_in, false);
> -qemu_chr_fe_deinit(>chr_sec_in, false);
> -qemu_chr_fe_deinit(>chr_out, false);
> -if (s->notify_dev) {
> -qemu_chr_fe_deinit(>chr_notify_dev, false);
> -}
> -
> -if (s->iothread) {
> -colo_compare_timer_del(s);
> -}
> -
> -qemu_bh_delete(s->event_bh);
> -
> qemu_mutex_lock(_compare_mutex);
> QTAILQ_FOREACH(tmp, _compares, next) {
> if (tmp == s) {
> @@ -1411,6 +1399,19 @@ static void colo_compare_finalize(Object *obj)
> }
> qemu_mutex_unlock(_compare_mutex);
>
> +qemu_chr_fe_deinit(>chr_pri_in, false);
> +qemu_chr_fe_deinit(>chr_sec_in, false);
> +qemu_chr_fe_deinit(>chr_out, false);
> +if (s->notify_dev) {
> +qemu_chr_fe_deinit(>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
