Re: [Qemu-devel] [PATCH 4/4] iothread: push gcontext earlier in the thread_fn

[Peter Xu <peterx@redhat.com>]

On Mon, Mar 04, 2019 at 10:12:10AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Mar 4, 2019 at 3:27 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Fri, Mar 01, 2019 at 04:25:17PM +0000, Stefan Hajnoczi wrote:
> > > On Thu, Feb 28, 2019 at 01:58:56PM +0800, Peter Xu wrote:
> > > > On Wed, Feb 27, 2019 at 01:38:38PM +0000, Stefan Hajnoczi wrote:
> > > > > On Fri, Feb 22, 2019 at 02:57:24PM +0800, Peter Xu wrote:
> > > > > > On Fri, Feb 22, 2019 at 07:37:02AM +0100, Marc-André Lureau wrote:
> > > > > > > Hi
> > > > > > >
> > > > > > > On Fri, Feb 22, 2019 at 4:14 AM Peter Xu <peterx@redhat.com> wrote:
> > > > > > > >
> > > > > > > > We were pushing the context until right before running the gmainloop.
> > > > > > > > Now since we have everything unconditionally, we can move this
> > > > > > > > earlier.
> > > > > > > >
> > > > > > > > One benefit is that now it's done even before init_done_sem, so as
> > > > > > > > long as the iothread user calls iothread_create() and completes, we
> > > > > > > > know that the thread stack is ready.
> > > > > > > >
> > > > > > >
> > > > > > > This will change the default context in the iothread, for code running
> > > > > > > there. This may not be a good idea. Until now, only sources dispatched
> > > > > > > from iothread_get_g_main_context() would have default context
> > > > > > > associated to it.
> > > > > > >
> > > > > > > I don't know if the current behaviour is intentional, but it has some
> > > > > > > logic. With this change, you may create hidden races, by changing the
> > > > > > > default context of sources to the iothread.
> > > > > >
> > > > > > Yes I agree that the behavior will be changed in this patch that even
> > > > > > if the iothread user does not use the gcontext they'll also have the
> > > > > > context set.  I would think it should be ok because IMHO events hooked
> > > > > > onto the aio context should not depend on the gcontext, but indeed I'd
> > > > > > like to get some confirmation from others, especially the block layer.
> > > > >
> > > > > I don't understand why Patch 4 is desirable.  The comment about
> > > > > init_done_sem isn't clear to me but I also wondered the same thing as
> > > > > Marc-André.
> > > > >
> > > > > Can you explain why we should apply this patch?
> > > >
> > > > Hi, Stefan,
> > > >
> > > > The patch 4 itself does not help much for current QEMU, but it should
> > > > be required to replace the patch that Marc-Andre proposed below:
> > > >
> > > > https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg05460.html [1]

[1]

> > > >
> > > > And IMHO patch 4 along with this whole series should be a cleaner
> > > > approach comparing to the one proposed in [1].  Here if my
> > > > understanding is correct the problem is that
> > > > g_main_context_push_thread_default() is really designed to be called
> > > > at the very beginning of a thread creation but not dynamically called
> > > > during the execution of a thread (prove is that it even does not have
> > > > any error to return when failed to acquire the context so the caller
> > > > will never know if it failed! see [2] below), in that sense this patch
> > > > 4 can be seen as a tiny cleanup too.
> > > >
> > > > g_main_context_push_thread_default (GMainContext *context)
> > > > {
> > > >   GQueue *stack;
> > > >   gboolean acquired_context;
> > > >
> > > >   acquired_context = g_main_context_acquire (context);
> > > >   g_return_if_fail (acquired_context);  <------------- [2]
> > > >
> > > >   ...
> > > > }
> > >
> > > I see.  This explains why you want to call it early.  If you're worried
> > > about that then there should also be a comment warning people that this
> > > must happen first before anything implicitly uses the thread's
> > > GMainContext.
> >
> > Sure, I can add a comment above g_main_context_push_thread_default()
> > to emphasize why it's preferred at the entry.
> >
> > >
> > > What about Marc-André's concern about the change in behavior?  Now this
> > > thread is associated with the GMainContext that isn't processed at in
> > > aio_poll().  Previously the default main context would be used.
> >
> > IIUC Paolo has answered this question (Message-ID:
> > <0faeceb2-68fa-59b0-48c3-b8e907b2a75f@redhat.com>) - if the block
> > layer (or say, the explicit aio_poll in the iothread_run) does not use
> > the GMainContext at all then it should affect nothing, and with that
> > there should have no real functional change.
> >
> 
> It's a bold claim though, iothread isn't used only by the block layer.
> It's used also by colo in qemu tree, and I used to have a branch for
> virgl rendering for example. There might be other experimental work or
> usage I missed. Furthermore, even the block layer, and its various
> dependencies, is hard to review thoroughly. But it looks like it
> doesn't rely on glib main context, I agree. But I would get prepared
> for some weird (difficult to debug) regressions eventually.

For COLO: it should be using GMainContext all the time (IIRC it's the
one who introduced this "push default gcontext" stuff to iothread
world...) so it should be fine too?  After all it's the only real user
of that so far!

For virgl rendering: I'm not sure whether there's assumption on
running them on the main thread and whether there would be race
against it if running on the iothread (AFAIU that's what the "push
default gcontext will affect"), but I would assume there's no such
requirement otherwise IMHO it should not need a dedicated iothread at
all if it needs serialization on main thread tasks...

For other "experimental work": I can't tell because I never know them.

I'm fine to drop this patch if any of us worries that this patch will
break something and I cannot guarantee anything for sure if we're even
considering experimental works... but still if any of us wants to move
on with [1] above I would still prefer to consider this patch 4.

Regards,

-- 
Peter Xu