On Mon, Jan 06, 2020 at 02:45:52PM +0000, Peter Maydell wrote: > On older versions of glib (anything prior to glib commit 0f056ebe > from May 2019), the implementation of g_source_ref() and > g_source_unref() is not threadsafe for a GSource which is not > attached to a GMainContext. > > QEMU's real iothread.c implementation always attaches its > iothread->ctx's GSource to a GMainContext created for that iothread, > so it is OK, but the simple test framework implementation in > tests/iothread.c was not doing this. This was causing intermittent > assertion failures in the test-aio-multithread subtest > "/aio/multi/mutex/contended" test on the BSD hosts. (It's unclear > why only BSD seems to have been affected -- perhaps a combination of > the specific glib version being used in the VMs and their happening > to run on a host with a lot of CPUs). > > Borrow the iothread_init_gcontext() from the real iothread.c > and add the corresponding cleanup code and the calls to > g_main_context_push/pop_thread_default() so we actually use > the GMainContext we create. > > Signed-off-by: Peter Maydell <[email protected]>
I've no idea on the g_source_ref() issue, but if so then a patch like this makes sense to me especially if it fixes up test failures. Reviewed-by: Peter Xu <[email protected]> Still a few comments below. > --- > I don't really have a good understanding of the glib APIs here, > so I'm mostly just cribbing code from the real iothread.c; > review by people who do know the glib/iothread stuff better > welcomed. It does seem to fix the intermittent test failure > on NetBSD, at least, where we were running into an assertion > failure because a g_source_unref() incorrectly thought it > had decremented the refcount to 0 and should delete a context > that was actually still in use. > > tests/iothread.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/tests/iothread.c b/tests/iothread.c > index 13c9fdcd8df..d3a2ee9a014 100644 > --- a/tests/iothread.c > +++ b/tests/iothread.c > @@ -21,6 +21,8 @@ > > struct IOThread { > AioContext *ctx; > + GMainContext *worker_context; > + GMainLoop *main_loop; > > QemuThread thread; > QemuMutex init_done_lock; > @@ -35,6 +37,17 @@ AioContext *qemu_get_current_aio_context(void) > return my_iothread ? my_iothread->ctx : qemu_get_aio_context(); > } > > +static void iothread_init_gcontext(IOThread *iothread) > +{ > + GSource *source; > + > + iothread->worker_context = g_main_context_new(); > + source = aio_get_g_source(iothread_get_aio_context(iothread)); > + g_source_attach(source, iothread->worker_context); > + g_source_unref(source); > + iothread->main_loop = g_main_loop_new(iothread->worker_context, TRUE); IIUC the main_loop is not required because in this case we only use the aio context to run rather than the main context itself. > +} > + > static void *iothread_run(void *opaque) > { > IOThread *iothread = opaque; > @@ -44,6 +57,20 @@ static void *iothread_run(void *opaque) > my_iothread = iothread; > qemu_mutex_lock(&iothread->init_done_lock); > iothread->ctx = aio_context_new(&error_abort); > + > + /* > + * We must connect the ctx to a GMainContext, because in older versions > + * of glib the g_source_ref()/unref() functions are not threadsafe > + * on sources without a context. > + */ > + iothread_init_gcontext(iothread); > + > + /* > + * g_main_context_push_thread_default() must be called before anything > + * in this new thread uses glib. > + */ > + g_main_context_push_thread_default(iothread->worker_context); IMHO if all the users of tests/iothread.c are block layers who only uses the aio context directly, then I think this is not needed too. Thanks, > + > qemu_cond_signal(&iothread->init_done_cond); > qemu_mutex_unlock(&iothread->init_done_lock); > > @@ -51,6 +78,7 @@ static void *iothread_run(void *opaque) > aio_poll(iothread->ctx, true); > } > > + g_main_context_pop_thread_default(iothread->worker_context); > rcu_unregister_thread(); > return NULL; > } > @@ -66,6 +94,8 @@ void iothread_join(IOThread *iothread) > { > aio_bh_schedule_oneshot(iothread->ctx, iothread_stop_bh, iothread); > qemu_thread_join(&iothread->thread); > + g_main_context_unref(iothread->worker_context); > + g_main_loop_unref(iothread->main_loop); > qemu_cond_destroy(&iothread->init_done_cond); > qemu_mutex_destroy(&iothread->init_done_lock); > aio_context_unref(iothread->ctx); > -- > 2.20.1 > -- Peter Xu
