diff mbox series

[4/4] iothread: push gcontext earlier in the thread_fn

Message ID 20190222031413.20250-5-peterx@redhat.com
State New
Headers show
Series iothread: create gcontext unconditionally | expand

Commit Message

Peter Xu Feb. 22, 2019, 3:14 a.m. UTC
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.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 iothread.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Marc-André Lureau Feb. 22, 2019, 6:37 a.m. UTC | #1
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.

> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  iothread.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/iothread.c b/iothread.c
> index 9abdbace66..7b7cba5d04 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -53,7 +53,7 @@ static void *iothread_run(void *opaque)
>      IOThread *iothread = opaque;
>
>      rcu_register_thread();
> -
> +    g_main_context_push_thread_default(iothread->worker_context);
>      my_iothread = iothread;
>      iothread->thread_id = qemu_get_thread_id();
>      qemu_sem_post(&iothread->init_done_sem);
> @@ -66,12 +66,11 @@ static void *iothread_run(void *opaque)
>           * changed in previous aio_poll()
>           */
>          if (iothread->running && atomic_read(&iothread->run_gcontext)) {
> -            g_main_context_push_thread_default(iothread->worker_context);
>              g_main_loop_run(iothread->main_loop);
> -            g_main_context_pop_thread_default(iothread->worker_context);
>          }
>      }
>
> +    g_main_context_pop_thread_default(iothread->worker_context);
>      rcu_unregister_thread();
>      return NULL;
>  }
> --
> 2.17.1
>
Peter Xu Feb. 22, 2019, 6:57 a.m. UTC | #2
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.

Stefan?

Thanks,
Paolo Bonzini Feb. 22, 2019, 9:24 a.m. UTC | #3
On 22/02/19 07:57, 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.

The block layer does not use GSource at all.

Paolo
Stefan Hajnoczi Feb. 27, 2019, 1:38 p.m. UTC | #4
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?

Stefan
Peter Xu Feb. 28, 2019, 5:58 a.m. UTC | #5
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]

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]

  ...
}

Regards,
Stefan Hajnoczi March 1, 2019, 4:25 p.m. UTC | #6
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]
> 
> 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.

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.

Stefan
Peter Xu March 4, 2019, 2:26 a.m. UTC | #7
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]
> > 
> > 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.

Regards,
Marc-André Lureau March 4, 2019, 9:12 a.m. UTC | #8
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]
> > >
> > > 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.
Peter Xu March 4, 2019, 9:37 a.m. UTC | #9
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,
diff mbox series

Patch

diff --git a/iothread.c b/iothread.c
index 9abdbace66..7b7cba5d04 100644
--- a/iothread.c
+++ b/iothread.c
@@ -53,7 +53,7 @@  static void *iothread_run(void *opaque)
     IOThread *iothread = opaque;
 
     rcu_register_thread();
-
+    g_main_context_push_thread_default(iothread->worker_context);
     my_iothread = iothread;
     iothread->thread_id = qemu_get_thread_id();
     qemu_sem_post(&iothread->init_done_sem);
@@ -66,12 +66,11 @@  static void *iothread_run(void *opaque)
          * changed in previous aio_poll()
          */
         if (iothread->running && atomic_read(&iothread->run_gcontext)) {
-            g_main_context_push_thread_default(iothread->worker_context);
             g_main_loop_run(iothread->main_loop);
-            g_main_context_pop_thread_default(iothread->worker_context);
         }
     }
 
+    g_main_context_pop_thread_default(iothread->worker_context);
     rcu_unregister_thread();
     return NULL;
 }