[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] tests/iothread: Always connect iothread GSource to a GMainCo
From: |
Peter Xu |
Subject: |
Re: [PATCH] tests/iothread: Always connect iothread GSource to a GMainContext |
Date: |
Mon, 6 Jan 2020 12:28:08 -0500 |
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 <address@hidden>
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 <address@hidden>
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