| Message ID | 3291bc9d7372e2b50c517efd92404ae5437e65fb.1590129793.git.lukasstraub2@web.de |
|---|---|
| State | New |
| Headers | show |
| Series | colo-compare bugfixes | expand |
> -----Original Message----- > From: Lukas Straub <lukasstraub2@web.de> > Sent: Friday, May 22, 2020 2:48 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> > Subject: [PATCH v6 5/6] net/colo-compare.c: Check that colo-compare is > active > > If the colo-compare object is removed before failover and a checkpoint > happens, qemu crashes because it tries to lock the destroyed event_mtx in > colo_notify_compares_event. > > Fix this by checking if everything is initialized by introducing a new variable > colo_compare_active which is protected by a new mutex > colo_compare_mutex. The new mutex also protects against concurrent > access of the net_compares list and makes sure that > colo_notify_compares_event isn't active while we destroy event_mtx and > event_complete_cond. > > With this it also is again possible to use colo without colo-compare (periodic > mode) and to use multiple colo-compare for multiple network interfaces. > > Signed-off-by: Lukas Straub <lukasstraub2@web.de> > Tested-by: Lukas Straub <lukasstraub2@web.de> > --- > net/colo-compare.c | 35 +++++++++++++++++++++++++++++------ > 1 file changed, 29 insertions(+), 6 deletions(-) > > diff --git a/net/colo-compare.c b/net/colo-compare.c index > 7886444cdf..64d2453450 100644 > --- a/net/colo-compare.c > +++ b/net/colo-compare.c > @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers = #define > REGULAR_PACKET_CHECK_MS 3000 #define DEFAULT_TIME_OUT_MS 3000 > > +static QemuMutex colo_compare_mutex; > +static bool colo_compare_active; > static QemuMutex event_mtx; > static QemuCond event_complete_cond; > static int event_unhandled_count; > @@ -906,6 +908,12 @@ static void check_old_packet_regular(void *opaque) > void colo_notify_compares_event(void *opaque, int event, Error **errp) { > CompareState *s; > + qemu_mutex_lock(&colo_compare_mutex); > + > + if (!colo_compare_active) { > + qemu_mutex_unlock(&colo_compare_mutex); > + return; > + } > > qemu_mutex_lock(&event_mtx); > QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@ void > colo_notify_compares_event(void *opaque, int event, Error **errp) > } > > qemu_mutex_unlock(&event_mtx); > + qemu_mutex_unlock(&colo_compare_mutex); > } > > static void colo_compare_timer_init(CompareState *s) @@ -1274,7 +1283,14 > @@ 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; > @@ -1292,9 +1308,6 @@ static void colo_compare_complete(UserCreatable > *uc, Error **errp) > > g_queue_init(&s->conn_list); > > - qemu_mutex_init(&event_mtx); > - qemu_cond_init(&event_complete_cond); > - > s->connection_track_table = > g_hash_table_new_full(connection_key_hash, > connection_key_equal, > g_free, @@ -1386,12 +1399,19 @@ static void > colo_compare_finalize(Object *obj) > > qemu_bh_delete(s->event_bh); > > + qemu_mutex_lock(&colo_compare_mutex); > QTAILQ_FOREACH(tmp, &net_compares, next) { > if (tmp == s) { > QTAILQ_REMOVE(&net_compares, s, next); > break; > } > } > + if (QTAILQ_EMPTY(&net_compares)) { > + colo_compare_active = false; > + qemu_mutex_destroy(&event_mtx); > + qemu_cond_destroy(&event_complete_cond); > + } > + qemu_mutex_unlock(&colo_compare_mutex); > > AioContext *ctx = iothread_get_aio_context(s->iothread); > aio_context_acquire(ctx); > @@ -1419,15 +1439,18 @@ static void colo_compare_finalize(Object *obj) > object_unref(OBJECT(s->iothread)); > } > > - qemu_mutex_destroy(&event_mtx); > - qemu_cond_destroy(&event_complete_cond); > - > g_free(s->pri_indev); > g_free(s->sec_indev); > g_free(s->outdev); > g_free(s->notify_dev); > } > > +static void __attribute__((__constructor__)) > +colo_compare_init_globals(void) { > + colo_compare_active = false; > + qemu_mutex_init(&colo_compare_mutex); > +} > + Looks good for me. I will queue this series. Reviewed-by: Zhang Chen <chen.zhang@intel.com> Thanks Zhang Chen > static const TypeInfo colo_compare_info = { > .name = TYPE_COLO_COMPARE, > .parent = TYPE_OBJECT, > -- > 2.20.1
diff --git a/net/colo-compare.c b/net/colo-compare.c index 7886444cdf..64d2453450 100644 --- a/net/colo-compare.c +++ b/net/colo-compare.c @@ -54,6 +54,8 @@ static NotifierList colo_compare_notifiers = #define REGULAR_PACKET_CHECK_MS 3000 #define DEFAULT_TIME_OUT_MS 3000 +static QemuMutex colo_compare_mutex; +static bool colo_compare_active; static QemuMutex event_mtx; static QemuCond event_complete_cond; static int event_unhandled_count; @@ -906,6 +908,12 @@ static void check_old_packet_regular(void *opaque) void colo_notify_compares_event(void *opaque, int event, Error **errp) { CompareState *s; + qemu_mutex_lock(&colo_compare_mutex); + + if (!colo_compare_active) { + qemu_mutex_unlock(&colo_compare_mutex); + return; + } qemu_mutex_lock(&event_mtx); QTAILQ_FOREACH(s, &net_compares, next) { @@ -919,6 +927,7 @@ void colo_notify_compares_event(void *opaque, int event, Error **errp) } qemu_mutex_unlock(&event_mtx); + qemu_mutex_unlock(&colo_compare_mutex); } static void colo_compare_timer_init(CompareState *s) @@ -1274,7 +1283,14 @@ 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; @@ -1292,9 +1308,6 @@ static void colo_compare_complete(UserCreatable *uc, Error **errp) g_queue_init(&s->conn_list); - qemu_mutex_init(&event_mtx); - qemu_cond_init(&event_complete_cond); - s->connection_track_table = g_hash_table_new_full(connection_key_hash, connection_key_equal, g_free, @@ -1386,12 +1399,19 @@ static void colo_compare_finalize(Object *obj) qemu_bh_delete(s->event_bh); + qemu_mutex_lock(&colo_compare_mutex); QTAILQ_FOREACH(tmp, &net_compares, next) { if (tmp == s) { QTAILQ_REMOVE(&net_compares, s, next); break; } } + if (QTAILQ_EMPTY(&net_compares)) { + colo_compare_active = false; + qemu_mutex_destroy(&event_mtx); + qemu_cond_destroy(&event_complete_cond); + } + qemu_mutex_unlock(&colo_compare_mutex); AioContext *ctx = iothread_get_aio_context(s->iothread); aio_context_acquire(ctx); @@ -1419,15 +1439,18 @@ static void colo_compare_finalize(Object *obj) object_unref(OBJECT(s->iothread)); } - qemu_mutex_destroy(&event_mtx); - qemu_cond_destroy(&event_complete_cond); - g_free(s->pri_indev); g_free(s->sec_indev); g_free(s->outdev); g_free(s->notify_dev); } +static void __attribute__((__constructor__)) colo_compare_init_globals(void) +{ + colo_compare_active = false; + qemu_mutex_init(&colo_compare_mutex); +} + static const TypeInfo colo_compare_info = { .name = TYPE_COLO_COMPARE, .parent = TYPE_OBJECT,