RFC: make chardev context switching safer

[Qemu-devel] [PATCH 1/4] iothread: wait until the glib context is acquired

Posted by Marc-André Lureau 6 years, 8 months ago

Another thread may acquire the glib context (temporarily) before
g_main_context_push_thread_default().

This can happen with the following qemu_chr_fe_set_handlers()
modifications.

Unfortunately, g_main_context_wait() is deprecated in glib
2.58 (apparently it was a broken interface). Use a polling loop.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 iothread.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/iothread.c b/iothread.c
index e615b7ae52..93cc3aa875 100644
--- a/iothread.c
+++ b/iothread.c
@@ -70,6 +70,11 @@ static void *iothread_run(void *opaque)
         if (iothread->running && atomic_read(&iothread->worker_context)) {
             GMainLoop *loop;
 
+            /* we may race with another thread acquiring the context */
+            while (!g_main_context_acquire(iothread->worker_context)) {
+                g_usleep(10000);
+            }
+
             g_main_context_push_thread_default(iothread->worker_context);
             iothread->main_loop =
                 g_main_loop_new(iothread->worker_context, TRUE);
@@ -80,6 +85,8 @@ static void *iothread_run(void *opaque)
             g_main_loop_unref(loop);
 
             g_main_context_pop_thread_default(iothread->worker_context);
+
+            g_main_context_release(iothread->worker_context);
         }
     }
 
-- 
2.21.0.rc1

Re: [Qemu-devel] [PATCH 1/4] iothread: wait until the glib context is acquired

Posted by Peter Xu 6 years, 8 months ago

On Wed, Feb 20, 2019 at 05:06:25PM +0100, Marc-André Lureau wrote:
> Another thread may acquire the glib context (temporarily) before
> g_main_context_push_thread_default().
> 
> This can happen with the following qemu_chr_fe_set_handlers()
> modifications.
> 
> Unfortunately, g_main_context_wait() is deprecated in glib
> 2.58 (apparently it was a broken interface). Use a polling loop.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  iothread.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/iothread.c b/iothread.c
> index e615b7ae52..93cc3aa875 100644
> --- a/iothread.c
> +++ b/iothread.c
> @@ -70,6 +70,11 @@ static void *iothread_run(void *opaque)
>          if (iothread->running && atomic_read(&iothread->worker_context)) {
>              GMainLoop *loop;
>  
> +            /* we may race with another thread acquiring the context */
> +            while (!g_main_context_acquire(iothread->worker_context)) {
> +                g_usleep(10000);
> +            }

Could you help explain why need this explicitly?  Since AFAIU
g_main_loop_run() below will do context acquire too so IIUC you're
taking it twice (while g_main_context_acquire should allow it to
happen, though)?

> +
>              g_main_context_push_thread_default(iothread->worker_context);
>              iothread->main_loop =
>                  g_main_loop_new(iothread->worker_context, TRUE);
> @@ -80,6 +85,8 @@ static void *iothread_run(void *opaque)
>              g_main_loop_unref(loop);
>  
>              g_main_context_pop_thread_default(iothread->worker_context);
> +
> +            g_main_context_release(iothread->worker_context);
>          }
>      }
>  
> -- 
> 2.21.0.rc1
> 
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/4] iothread: wait until the glib context is acquired

Posted by Marc-André Lureau 6 years, 8 months ago

Hi

On Thu, Feb 21, 2019 at 9:00 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Feb 20, 2019 at 05:06:25PM +0100, Marc-André Lureau wrote:
> > Another thread may acquire the glib context (temporarily) before
> > g_main_context_push_thread_default().
> >
> > This can happen with the following qemu_chr_fe_set_handlers()
> > modifications.
> >
> > Unfortunately, g_main_context_wait() is deprecated in glib
> > 2.58 (apparently it was a broken interface). Use a polling loop.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  iothread.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/iothread.c b/iothread.c
> > index e615b7ae52..93cc3aa875 100644
> > --- a/iothread.c
> > +++ b/iothread.c
> > @@ -70,6 +70,11 @@ static void *iothread_run(void *opaque)
> >          if (iothread->running && atomic_read(&iothread->worker_context)) {
> >              GMainLoop *loop;
> >
> > +            /* we may race with another thread acquiring the context */
> > +            while (!g_main_context_acquire(iothread->worker_context)) {
> > +                g_usleep(10000);
> > +            }
>
> Could you help explain why need this explicitly?  Since AFAIU
> g_main_loop_run() below will do context acquire too so IIUC you're
> taking it twice (while g_main_context_acquire should allow it to
> happen, though)?
>

We call g_main_context_push_thread_default() before run(). It will
fail if the context is not acquirable.

> > +
> >              g_main_context_push_thread_default(iothread->worker_context);
> >              iothread->main_loop =
> >                  g_main_loop_new(iothread->worker_context, TRUE);
> > @@ -80,6 +85,8 @@ static void *iothread_run(void *opaque)
> >              g_main_loop_unref(loop);
> >
> >              g_main_context_pop_thread_default(iothread->worker_context);
> > +
> > +            g_main_context_release(iothread->worker_context);
> >          }
> >      }
> >
> > --
> > 2.21.0.rc1
> >
> >
>
> Regards,
>
> --
> Peter Xu

Re: [Qemu-devel] [PATCH 1/4] iothread: wait until the glib context is acquired

Posted by Peter Xu 6 years, 8 months ago

On Thu, Feb 21, 2019 at 11:39:32AM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Feb 21, 2019 at 9:00 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > On Wed, Feb 20, 2019 at 05:06:25PM +0100, Marc-André Lureau wrote:
> > > Another thread may acquire the glib context (temporarily) before
> > > g_main_context_push_thread_default().
> > >
> > > This can happen with the following qemu_chr_fe_set_handlers()
> > > modifications.
> > >
> > > Unfortunately, g_main_context_wait() is deprecated in glib
> > > 2.58 (apparently it was a broken interface). Use a polling loop.
> > >
> > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > > ---
> > >  iothread.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/iothread.c b/iothread.c
> > > index e615b7ae52..93cc3aa875 100644
> > > --- a/iothread.c
> > > +++ b/iothread.c
> > > @@ -70,6 +70,11 @@ static void *iothread_run(void *opaque)
> > >          if (iothread->running && atomic_read(&iothread->worker_context)) {
> > >              GMainLoop *loop;
> > >
> > > +            /* we may race with another thread acquiring the context */
> > > +            while (!g_main_context_acquire(iothread->worker_context)) {
> > > +                g_usleep(10000);
> > > +            }
> >
> > Could you help explain why need this explicitly?  Since AFAIU
> > g_main_loop_run() below will do context acquire too so IIUC you're
> > taking it twice (while g_main_context_acquire should allow it to
> > happen, though)?
> >
> 
> We call g_main_context_push_thread_default() before run(). It will
> fail if the context is not acquirable.

Thanks for explaining.  It wasn't obvious to me.

I've posted another series to refactor iothread a bit and it should be
able to drop this patch if based on that series (that series should
even remove code instead of adding new).  Please feel free to have a
look, or give it a shot:

  [PATCH 0/4] iothread: create gcontext unconditionally

Regards,

-- 
Peter Xu