Re: [PATCH] tests/iothread: Always connect iothread GSource to a GMainContext

[Peter Maydell]

On Mon, 6 Jan 2020 at 17:28, Peter Xu <address@hidden> wrote:
>
> 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.

> > +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.

Mmm. I wasn't sure to what extent glib expects the
GMainContext and GMainLoop to match up, so I was mostly
just copying from iothread.c.

> > +    /*
> > +     * 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.

So we're OK to not do this because tests/iothread.c's
main loop doesn't call g_main_loop_run(), and it doesn't
provide an iothread_get_g_main_context() ?

I'm kind of inclined towards being lazy and sticking with
what this patch has, because:
 * it matches the real iothread.c, which reduces the possiblity
   of future surprise bugs due to things not matching up
 * it's already been reviewed
 * it saves me having to do a respin and retest

But if people would prefer these bits deleted I'll stop
being lazy :-)

thanks
-- PMM