[Qemu-devel] [PATCH 0/4] RFC: make chardev context switching safer

[Qemu-devel] [PATCH 0/4] RFC: make chardev context switching safer

[Marc-André Lureau]

Hi,

The chardev context switching code is a bit fragile, as it works as if
the current context is properly synchronized with the new context.  It
isn't so obvious to me that concurrent usage of chardev can't happen,
as there might be various main loop sources being dispatched during
the switch.

Worried about the situation, I wrote those patches a while ago, I
think they are still worth to consider. I used to have some basic
test, but it now conflicts a lot with recent changes. I would like to
get some feedback about the series before I rewrite it.

The importat patch is "chardev: make qemu_chr_fe_set_handlers()
context switching safer". It works by "freezing" the given contexts
while the chardev will "move" (recreate to the new context) the
various sources it owns. This looks quite ugly to me overall, but still
safer than today.

This should allow to simplify the scary code from "monitor: set the
chardev context from the main context/thread".

Finally, "char-socket: restart the reconnect timer to switch context"
shows that we have chardev backends that do not switch fully yet.

(there should be a meme "using chardev from multiple threads is
considered harmful")

Marc-André Lureau (4):
  iothread: wait until the glib context is acquired
  chardev: make qemu_chr_fe_set_handlers() context switching safer
  monitor: set the chardev context from the main context/thread
  char-socket: restart the reconnect timer to switch context

 include/chardev/char-fe.h |  23 +++++++++
 chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
 chardev/char-mux.c        |  14 +++---
 chardev/char-socket.c     |   5 ++
 iothread.c                |   7 +++
 monitor.c                 |  39 +++------------
 6 files changed, 139 insertions(+), 52 deletions(-)

-- 
2.21.0.rc1

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

[Marc-André Lureau]

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

[Peter Xu]

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

[Marc-André Lureau]

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

[Peter Xu]

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

[Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Marc-André Lureau]

qemu_chr_fe_set_handlers() may switch the context of various
sources. In order to prevent dispatch races from different threads,
let's acquire or freeze the context, do all the source switches, and
then release/resume the contexts. This should help to make context
switching safer.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char-fe.h |  23 +++++++++
 chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
 chardev/char-mux.c        |  14 +++---
 3 files changed, 121 insertions(+), 19 deletions(-)

diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
index aa1b864ccd..4051435a1c 100644
--- a/include/chardev/char-fe.h
+++ b/include/chardev/char-fe.h
@@ -84,6 +84,14 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
  * Set the front end char handlers. The front end takes the focus if
  * any of the handler is non-NULL.
  *
+ * A chardev may have multiple main loop sources. In order to prevent
+ * races when switching contexts, the function will temporarily block
+ * the contexts before the source switch to prevent them from
+ * dispatching in different threads concurrently.
+ *
+ * The current and the new @context must be acquirable or
+ * running & dispatched in a loop (the function will hang otherwise).
+ *
  * Without associated Chardev, nothing is changed.
  */
 void qemu_chr_fe_set_handlers_full(CharBackend *b,
@@ -110,6 +118,21 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               GMainContext *context,
                               bool set_open);
 
+/**
+ * qemu_chr_fe_set_handlers_internal:
+ *
+ * Same as qemu_chr_fe_set_handlers(), without context freezing.
+ */
+void qemu_chr_fe_set_handlers_internal(CharBackend *b,
+                                       IOCanReadHandler *fd_can_read,
+                                       IOReadHandler *fd_read,
+                                       IOEventHandler *fd_event,
+                                       BackendChangeHandler *be_change,
+                                       void *opaque,
+                                       GMainContext *context,
+                                       bool set_open,
+                                       bool sync_state);
+
 /**
  * qemu_chr_fe_take_focus:
  *
diff --git a/chardev/char-fe.c b/chardev/char-fe.c
index f3530a90e6..90cd7db007 100644
--- a/chardev/char-fe.c
+++ b/chardev/char-fe.c
@@ -246,15 +246,67 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
     }
 }
 
-void qemu_chr_fe_set_handlers_full(CharBackend *b,
-                                   IOCanReadHandler *fd_can_read,
-                                   IOReadHandler *fd_read,
-                                   IOEventHandler *fd_event,
-                                   BackendChangeHandler *be_change,
-                                   void *opaque,
-                                   GMainContext *context,
-                                   bool set_open,
-                                   bool sync_state)
+struct MainContextWait {
+    QemuCond cond;
+    QemuMutex lock;
+};
+
+static gboolean
+main_context_wait_cb(gpointer user_data)
+{
+    struct MainContextWait *w = user_data;
+
+    qemu_mutex_lock(&w->lock);
+    qemu_cond_signal(&w->cond);
+    /* wait until switching is over */
+    qemu_cond_wait(&w->cond, &w->lock);
+    qemu_mutex_unlock(&w->lock);
+
+    qemu_mutex_destroy(&w->lock);
+    qemu_cond_destroy(&w->cond);
+    g_free(w);
+
+    return G_SOURCE_REMOVE;
+}
+
+static void
+main_context_wait(struct MainContextWait **wait, GMainContext *ctxt)
+{
+    struct MainContextWait *w = NULL;
+
+    if (!g_main_context_acquire(ctxt)) {
+        w = g_new0(struct MainContextWait, 1);
+        qemu_mutex_init(&w->lock);
+        qemu_cond_init(&w->cond);
+        qemu_mutex_lock(&w->lock);
+        g_main_context_invoke(ctxt, main_context_wait_cb, w);
+        /* wait for the context to freeze */
+        qemu_cond_wait(&w->cond, &w->lock);
+        qemu_mutex_unlock(&w->lock);
+    }
+
+    *wait = w;
+}
+
+static void
+main_context_resume(struct MainContextWait *wait, GMainContext *ctxt)
+{
+    if (wait) {
+        qemu_cond_signal(&wait->cond);
+    } else {
+        g_main_context_release(ctxt);
+    }
+}
+
+void qemu_chr_fe_set_handlers_internal(CharBackend *b,
+                                       IOCanReadHandler *fd_can_read,
+                                       IOReadHandler *fd_read,
+                                       IOEventHandler *fd_event,
+                                       BackendChangeHandler *be_change,
+                                       void *opaque,
+                                       GMainContext *new_context,
+                                       bool set_open,
+                                       bool sync_state)
 {
     Chardev *s;
     int fe_open;
@@ -276,7 +328,7 @@ void qemu_chr_fe_set_handlers_full(CharBackend *b,
     b->chr_be_change = be_change;
     b->opaque = opaque;
 
-    qemu_chr_be_update_read_handlers(s, context);
+    qemu_chr_be_update_read_handlers(s, new_context);
 
     if (set_open) {
         qemu_chr_fe_set_open(b, fe_open);
@@ -292,6 +344,34 @@ void qemu_chr_fe_set_handlers_full(CharBackend *b,
     }
 }
 
+void qemu_chr_fe_set_handlers_full(CharBackend *b,
+                                   IOCanReadHandler *fd_can_read,
+                                   IOReadHandler *fd_read,
+                                   IOEventHandler *fd_event,
+                                   BackendChangeHandler *be_change,
+                                   void *opaque,
+                                   GMainContext *new_context,
+                                   bool set_open,
+                                   bool sync_state)
+{
+    GMainContext *old_context = b->chr->gcontext;
+    struct MainContextWait *old_ctxt_wait, *new_ctxt_wait;
+
+    main_context_wait(&old_ctxt_wait, old_context);
+    if (old_context != new_context) {
+        main_context_wait(&new_ctxt_wait, new_context);
+    }
+
+    qemu_chr_fe_set_handlers_internal(b, fd_can_read, fd_read, fd_event,
+                                      be_change, opaque, new_context,
+                                      set_open, sync_state);
+
+    main_context_resume(old_ctxt_wait, old_context);
+    if (old_context != new_context) {
+        main_context_resume(new_ctxt_wait, new_context);
+    }
+}
+
 void qemu_chr_fe_set_handlers(CharBackend *b,
                               IOCanReadHandler *fd_can_read,
                               IOReadHandler *fd_read,
@@ -302,8 +382,7 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
                               bool set_open)
 {
     qemu_chr_fe_set_handlers_full(b, fd_can_read, fd_read, fd_event, be_change,
-                                  opaque, context, set_open,
-                                  true);
+                                  opaque, context, set_open, true);
 }
 
 void qemu_chr_fe_take_focus(CharBackend *b)
diff --git a/chardev/char-mux.c b/chardev/char-mux.c
index 23aa82125d..9830cc4b37 100644
--- a/chardev/char-mux.c
+++ b/chardev/char-mux.c
@@ -283,13 +283,13 @@ static void mux_chr_update_read_handlers(Chardev *chr)
     MuxChardev *d = MUX_CHARDEV(chr);
 
     /* Fix up the real driver with mux routines */
-    qemu_chr_fe_set_handlers_full(&d->chr,
-                                  mux_chr_can_read,
-                                  mux_chr_read,
-                                  mux_chr_event,
-                                  NULL,
-                                  chr,
-                                  chr->gcontext, true, false);
+    qemu_chr_fe_set_handlers_internal(&d->chr,
+                                      mux_chr_can_read,
+                                      mux_chr_read,
+                                      mux_chr_event,
+                                      NULL,
+                                      chr,
+                                      chr->gcontext, true, false);
 }
 
 void mux_set_focus(Chardev *chr, int focus)
-- 
2.21.0.rc1

Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Peter Xu]

On Wed, Feb 20, 2019 at 05:06:26PM +0100, Marc-André Lureau wrote:
> qemu_chr_fe_set_handlers() may switch the context of various
> sources. In order to prevent dispatch races from different threads,
> let's acquire or freeze the context, do all the source switches, and
> then release/resume the contexts. This should help to make context
> switching safer.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char-fe.h |  23 +++++++++
>  chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
>  chardev/char-mux.c        |  14 +++---
>  3 files changed, 121 insertions(+), 19 deletions(-)
> 
> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> index aa1b864ccd..4051435a1c 100644
> --- a/include/chardev/char-fe.h
> +++ b/include/chardev/char-fe.h
> @@ -84,6 +84,14 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
>   * Set the front end char handlers. The front end takes the focus if
>   * any of the handler is non-NULL.
>   *
> + * A chardev may have multiple main loop sources. In order to prevent
> + * races when switching contexts, the function will temporarily block
> + * the contexts before the source switch to prevent them from
> + * dispatching in different threads concurrently.
> + *
> + * The current and the new @context must be acquirable or
> + * running & dispatched in a loop (the function will hang otherwise).
> + *
>   * Without associated Chardev, nothing is changed.
>   */
>  void qemu_chr_fe_set_handlers_full(CharBackend *b,
> @@ -110,6 +118,21 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>                                GMainContext *context,
>                                bool set_open);
>  
> +/**
> + * qemu_chr_fe_set_handlers_internal:
> + *
> + * Same as qemu_chr_fe_set_handlers(), without context freezing.
> + */
> +void qemu_chr_fe_set_handlers_internal(CharBackend *b,
> +                                       IOCanReadHandler *fd_can_read,
> +                                       IOReadHandler *fd_read,
> +                                       IOEventHandler *fd_event,
> +                                       BackendChangeHandler *be_change,
> +                                       void *opaque,
> +                                       GMainContext *context,
> +                                       bool set_open,
> +                                       bool sync_state);
> +
>  /**
>   * qemu_chr_fe_take_focus:
>   *
> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> index f3530a90e6..90cd7db007 100644
> --- a/chardev/char-fe.c
> +++ b/chardev/char-fe.c
> @@ -246,15 +246,67 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
>      }
>  }
>  
> -void qemu_chr_fe_set_handlers_full(CharBackend *b,
> -                                   IOCanReadHandler *fd_can_read,
> -                                   IOReadHandler *fd_read,
> -                                   IOEventHandler *fd_event,
> -                                   BackendChangeHandler *be_change,
> -                                   void *opaque,
> -                                   GMainContext *context,
> -                                   bool set_open,
> -                                   bool sync_state)
> +struct MainContextWait {
> +    QemuCond cond;
> +    QemuMutex lock;
> +};
> +
> +static gboolean
> +main_context_wait_cb(gpointer user_data)
> +{
> +    struct MainContextWait *w = user_data;
> +
> +    qemu_mutex_lock(&w->lock);
> +    qemu_cond_signal(&w->cond);
> +    /* wait until switching is over */
> +    qemu_cond_wait(&w->cond, &w->lock);

Could previous signal() directly wake up itself here?  Man
pthread_cond_broadcast says:

       The pthread_cond_signal() function shall unblock at least one
       of the threads that are blocked on the specified condition
       variable cond (if any threads are blocked on cond).

       If more than one thread is blocked on a condition variable, the
       scheduling policy shall determine the order in which threads
       are unblocked.

So AFAIU it could, because neither there's a restriction on ordering
of how waiters are waked up, nor there's a limitation on how many
waiters will be waked up by a single signal().

Why not simply use two semaphores?  Then locks can be avoided too.

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Philippe Mathieu-Daudé]

On 2/21/19 9:03 AM, Peter Xu wrote:
> On Wed, Feb 20, 2019 at 05:06:26PM +0100, Marc-André Lureau wrote:
>> qemu_chr_fe_set_handlers() may switch the context of various
>> sources. In order to prevent dispatch races from different threads,
>> let's acquire or freeze the context, do all the source switches, and
>> then release/resume the contexts. This should help to make context
>> switching safer.
>>
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> ---
>>  include/chardev/char-fe.h |  23 +++++++++
>>  chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
>>  chardev/char-mux.c        |  14 +++---
>>  3 files changed, 121 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
>> index aa1b864ccd..4051435a1c 100644
>> --- a/include/chardev/char-fe.h
>> +++ b/include/chardev/char-fe.h
>> @@ -84,6 +84,14 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
>>   * Set the front end char handlers. The front end takes the focus if
>>   * any of the handler is non-NULL.
>>   *
>> + * A chardev may have multiple main loop sources. In order to prevent
>> + * races when switching contexts, the function will temporarily block
>> + * the contexts before the source switch to prevent them from
>> + * dispatching in different threads concurrently.
>> + *
>> + * The current and the new @context must be acquirable or
>> + * running & dispatched in a loop (the function will hang otherwise).
>> + *
>>   * Without associated Chardev, nothing is changed.
>>   */
>>  void qemu_chr_fe_set_handlers_full(CharBackend *b,
>> @@ -110,6 +118,21 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
>>                                GMainContext *context,
>>                                bool set_open);
>>  
>> +/**
>> + * qemu_chr_fe_set_handlers_internal:
>> + *
>> + * Same as qemu_chr_fe_set_handlers(), without context freezing.
>> + */
>> +void qemu_chr_fe_set_handlers_internal(CharBackend *b,
>> +                                       IOCanReadHandler *fd_can_read,
>> +                                       IOReadHandler *fd_read,
>> +                                       IOEventHandler *fd_event,
>> +                                       BackendChangeHandler *be_change,
>> +                                       void *opaque,
>> +                                       GMainContext *context,
>> +                                       bool set_open,
>> +                                       bool sync_state);

Can we add this function into a new header "chardev/char-internal.h"
(internal to chardev/) rather than "include/chardev/char-fe.h" (public)?

>> +
>>  /**
>>   * qemu_chr_fe_take_focus:
>>   *
>> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
>> index f3530a90e6..90cd7db007 100644
>> --- a/chardev/char-fe.c
>> +++ b/chardev/char-fe.c
>> @@ -246,15 +246,67 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
>>      }
>>  }
>>  
>> -void qemu_chr_fe_set_handlers_full(CharBackend *b,
>> -                                   IOCanReadHandler *fd_can_read,
>> -                                   IOReadHandler *fd_read,
>> -                                   IOEventHandler *fd_event,
>> -                                   BackendChangeHandler *be_change,
>> -                                   void *opaque,
>> -                                   GMainContext *context,
>> -                                   bool set_open,
>> -                                   bool sync_state)
>> +struct MainContextWait {
>> +    QemuCond cond;
>> +    QemuMutex lock;
>> +};
>> +
>> +static gboolean
>> +main_context_wait_cb(gpointer user_data)
>> +{
>> +    struct MainContextWait *w = user_data;
>> +
>> +    qemu_mutex_lock(&w->lock);
>> +    qemu_cond_signal(&w->cond);
>> +    /* wait until switching is over */
>> +    qemu_cond_wait(&w->cond, &w->lock);
> 
> Could previous signal() directly wake up itself here?  Man
> pthread_cond_broadcast says:
> 
>        The pthread_cond_signal() function shall unblock at least one
>        of the threads that are blocked on the specified condition
>        variable cond (if any threads are blocked on cond).
> 
>        If more than one thread is blocked on a condition variable, the
>        scheduling policy shall determine the order in which threads
>        are unblocked.
> 
> So AFAIU it could, because neither there's a restriction on ordering
> of how waiters are waked up, nor there's a limitation on how many
> waiters will be waked up by a single signal().
> 
> Why not simply use two semaphores?  Then locks can be avoided too.
> 
> Regards,
> 

Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Marc-André Lureau]

Hi

Le ven. 22 févr. 2019 à 09:33, Philippe Mathieu-Daudé <philmd@redhat.com> a
écrit :

>
>
> On 2/21/19 9:03 AM, Peter Xu wrote:
> > On Wed, Feb 20, 2019 at 05:06:26PM +0100, Marc-André Lureau wrote:
> >> qemu_chr_fe_set_handlers() may switch the context of various
> >> sources. In order to prevent dispatch races from different threads,
> >> let's acquire or freeze the context, do all the source switches, and
> >> then release/resume the contexts. This should help to make context
> >> switching safer.
> >>
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >> ---
> >>  include/chardev/char-fe.h |  23 +++++++++
> >>  chardev/char-fe.c         | 103 +++++++++++++++++++++++++++++++++-----
> >>  chardev/char-mux.c        |  14 +++---
> >>  3 files changed, 121 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/include/chardev/char-fe.h b/include/chardev/char-fe.h
> >> index aa1b864ccd..4051435a1c 100644
> >> --- a/include/chardev/char-fe.h
> >> +++ b/include/chardev/char-fe.h
> >> @@ -84,6 +84,14 @@ bool qemu_chr_fe_backend_open(CharBackend *be);
> >>   * Set the front end char handlers. The front end takes the focus if
> >>   * any of the handler is non-NULL.
> >>   *
> >> + * A chardev may have multiple main loop sources. In order to prevent
> >> + * races when switching contexts, the function will temporarily block
> >> + * the contexts before the source switch to prevent them from
> >> + * dispatching in different threads concurrently.
> >> + *
> >> + * The current and the new @context must be acquirable or
> >> + * running & dispatched in a loop (the function will hang otherwise).
> >> + *
> >>   * Without associated Chardev, nothing is changed.
> >>   */
> >>  void qemu_chr_fe_set_handlers_full(CharBackend *b,
> >> @@ -110,6 +118,21 @@ void qemu_chr_fe_set_handlers(CharBackend *b,
> >>                                GMainContext *context,
> >>                                bool set_open);
> >>
> >> +/**
> >> + * qemu_chr_fe_set_handlers_internal:
> >> + *
> >> + * Same as qemu_chr_fe_set_handlers(), without context freezing.
> >> + */
> >> +void qemu_chr_fe_set_handlers_internal(CharBackend *b,
> >> +                                       IOCanReadHandler *fd_can_read,
> >> +                                       IOReadHandler *fd_read,
> >> +                                       IOEventHandler *fd_event,
> >> +                                       BackendChangeHandler *be_change,
> >> +                                       void *opaque,
> >> +                                       GMainContext *context,
> >> +                                       bool set_open,
> >> +                                       bool sync_state);
>
> Can we add this function into a new header "chardev/char-internal.h"
> (internal to chardev/) rather than "include/chardev/char-fe.h" (public)?
>

That should be possible, good idea


> >> +
> >>  /**
> >>   * qemu_chr_fe_take_focus:
> >>   *
> >> diff --git a/chardev/char-fe.c b/chardev/char-fe.c
> >> index f3530a90e6..90cd7db007 100644
> >> --- a/chardev/char-fe.c
> >> +++ b/chardev/char-fe.c
> >> @@ -246,15 +246,67 @@ void qemu_chr_fe_deinit(CharBackend *b, bool del)
> >>      }
> >>  }
> >>
> >> -void qemu_chr_fe_set_handlers_full(CharBackend *b,
> >> -                                   IOCanReadHandler *fd_can_read,
> >> -                                   IOReadHandler *fd_read,
> >> -                                   IOEventHandler *fd_event,
> >> -                                   BackendChangeHandler *be_change,
> >> -                                   void *opaque,
> >> -                                   GMainContext *context,
> >> -                                   bool set_open,
> >> -                                   bool sync_state)
> >> +struct MainContextWait {
> >> +    QemuCond cond;
> >> +    QemuMutex lock;
> >> +};
> >> +
> >> +static gboolean
> >> +main_context_wait_cb(gpointer user_data)
> >> +{
> >> +    struct MainContextWait *w = user_data;
> >> +
> >> +    qemu_mutex_lock(&w->lock);
> >> +    qemu_cond_signal(&w->cond);
> >> +    /* wait until switching is over */
> >> +    qemu_cond_wait(&w->cond, &w->lock);
> >
> > Could previous signal() directly wake up itself here?  Man
> > pthread_cond_broadcast says:
> >
> >        The pthread_cond_signal() function shall unblock at least one
> >        of the threads that are blocked on the specified condition
> >        variable cond (if any threads are blocked on cond).
> >
> >        If more than one thread is blocked on a condition variable, the
> >        scheduling policy shall determine the order in which threads
> >        are unblocked.
> >
> > So AFAIU it could, because neither there's a restriction on ordering
> > of how waiters are waked up, nor there's a limitation on how many
> > waiters will be waked up by a single signal().
> >
> > Why not simply use two semaphores?  Then locks can be avoided too.
> >
> > Regards,
> >
>
>

Re: [Qemu-devel] [PATCH 2/4] chardev: make qemu_chr_fe_set_handlers() context switching safer

[Peter Xu]

On Thu, Feb 21, 2019 at 04:03:57PM +0800, Peter Xu wrote:

[...]

> > +static gboolean
> > +main_context_wait_cb(gpointer user_data)
> > +{
> > +    struct MainContextWait *w = user_data;
> > +
> > +    qemu_mutex_lock(&w->lock);
> > +    qemu_cond_signal(&w->cond);
> > +    /* wait until switching is over */
> > +    qemu_cond_wait(&w->cond, &w->lock);
> 
> Could previous signal() directly wake up itself here?  Man
> pthread_cond_broadcast says:
> 
>        The pthread_cond_signal() function shall unblock at least one
>        of the threads that are blocked on the specified condition
>        variable cond (if any threads are blocked on cond).
> 
>        If more than one thread is blocked on a condition variable, the
>        scheduling policy shall determine the order in which threads
>        are unblocked.
> 
> So AFAIU it could, because neither there's a restriction on ordering
> of how waiters are waked up, nor there's a limitation on how many
> waiters will be waked up by a single signal().
> 
> Why not simply use two semaphores?  Then locks can be avoided too.

Please feel free to skip this question.  I think when cond_signal()
right before cond_wait() this thread is not yet in the waiting list so
at least my question seems invalid.  Then cond+lock looks fine
comparing to sems.  Sorry for the noise.

Regards,

-- 
Peter Xu

[Qemu-devel] [PATCH 3/4] monitor: set the chardev context from the main context/thread

[Marc-André Lureau]

With the previous changes to qemu_chr_fe_set_handlers(), it is no
longer necessary to call qemu_chr_fe_set_handlers() from the target
context.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 39 ++++++---------------------------------
 1 file changed, 6 insertions(+), 33 deletions(-)

diff --git a/monitor.c b/monitor.c
index 33ccbf3957..2312573300 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4513,19 +4513,6 @@ static void monitor_list_append(Monitor *mon)
     }
 }
 
-static void monitor_qmp_setup_handlers_bh(void *opaque)
-{
-    Monitor *mon = opaque;
-    GMainContext *context;
-
-    assert(mon->use_io_thread);
-    context = iothread_get_g_main_context(mon_iothread);
-    assert(context);
-    qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_qmp_read,
-                             monitor_qmp_event, NULL, mon, context, true);
-    monitor_list_append(mon);
-}
-
 void monitor_init(Chardev *chr, int flags)
 {
     Monitor *mon = g_malloc(sizeof(*mon));
@@ -4548,29 +4535,15 @@ void monitor_init(Chardev *chr, int flags)
     }
 
     if (monitor_is_qmp(mon)) {
+        GMainContext *ctxt = mon->use_io_thread ?
+            iothread_get_g_main_context(mon_iothread) : NULL;
+
         qemu_chr_fe_set_echo(&mon->chr, true);
         json_message_parser_init(&mon->qmp.parser, handle_qmp_command,
                                  mon, NULL);
-        if (mon->use_io_thread) {
-            /*
-             * Make sure the old iowatch is gone.  It's possible when
-             * e.g. the chardev is in client mode, with wait=on.
-             */
-            remove_fd_in_watch(chr);
-            /*
-             * We can't call qemu_chr_fe_set_handlers() directly here
-             * since chardev might be running in the monitor I/O
-             * thread.  Schedule a bottom half.
-             */
-            aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
-                                    monitor_qmp_setup_handlers_bh, mon);
-            /* The bottom half will add @mon to @mon_list */
-            return;
-        } else {
-            qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
-                                     monitor_qmp_read, monitor_qmp_event,
-                                     NULL, mon, NULL, true);
-        }
+        qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read,
+                                 monitor_qmp_read, monitor_qmp_event,
+                                 NULL, mon, ctxt, true);
     } else {
         qemu_chr_fe_set_handlers(&mon->chr, monitor_can_read, monitor_read,
                                  monitor_event, NULL, mon, NULL, true);
-- 
2.21.0.rc1

[Qemu-devel] [PATCH 4/4] char-socket: restart the reconnect timer to switch context

[Marc-André Lureau]

The reconnect timer is not switched to the given context, fix it.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 chardev/char-socket.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 4fcdd8aedd..671861f98c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -632,6 +632,11 @@ static void tcp_chr_update_read_handler(Chardev *chr)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
 
+    if (s->reconnect_timer) {
+        tcp_chr_reconn_timer_cancel(s);
+        qemu_chr_socket_restart_timer(chr);
+    }
+
     if (s->listener) {
         /*
          * It's possible that chardev context is changed in
-- 
2.21.0.rc1

Re: [Qemu-devel] [PATCH 0/4] RFC: make chardev context switching safer

[Peter Xu]

On Wed, Feb 20, 2019 at 05:06:24PM +0100, Marc-André Lureau wrote:
> Hi,

Hi,

> 
> The chardev context switching code is a bit fragile, as it works as if
> the current context is properly synchronized with the new context.  It
> isn't so obvious to me that concurrent usage of chardev can't happen,
> as there might be various main loop sources being dispatched during
> the switch.
> 
> Worried about the situation, I wrote those patches a while ago, I
> think they are still worth to consider. I used to have some basic
> test, but it now conflicts a lot with recent changes. I would like to
> get some feedback about the series before I rewrite it.
> 
> The importat patch is "chardev: make qemu_chr_fe_set_handlers()
> context switching safer". It works by "freezing" the given contexts
> while the chardev will "move" (recreate to the new context) the
> various sources it owns. This looks quite ugly to me overall, but still
> safer than today.
> 
> This should allow to simplify the scary code from "monitor: set the
> chardev context from the main context/thread".

Indeed it's at least not that friendly to readers... so out of
curiosity - is this the only reason for this series?

> 
> Finally, "char-socket: restart the reconnect timer to switch context"
> shows that we have chardev backends that do not switch fully yet.

This seems irrelevant to the series, or am I wrong?

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 0/4] RFC: make chardev context switching safer

[Marc-André Lureau]

Hi

On Thu, Feb 21, 2019 at 8:58 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Wed, Feb 20, 2019 at 05:06:24PM +0100, Marc-André Lureau wrote:
> > Hi,
>
> Hi,
>
> >
> > The chardev context switching code is a bit fragile, as it works as if
> > the current context is properly synchronized with the new context.  It
> > isn't so obvious to me that concurrent usage of chardev can't happen,
> > as there might be various main loop sources being dispatched during
> > the switch.
> >
> > Worried about the situation, I wrote those patches a while ago, I
> > think they are still worth to consider. I used to have some basic
> > test, but it now conflicts a lot with recent changes. I would like to
> > get some feedback about the series before I rewrite it.
> >
> > The importat patch is "chardev: make qemu_chr_fe_set_handlers()
> > context switching safer". It works by "freezing" the given contexts
> > while the chardev will "move" (recreate to the new context) the
> > various sources it owns. This looks quite ugly to me overall, but still
> > safer than today.
> >
> > This should allow to simplify the scary code from "monitor: set the
> > chardev context from the main context/thread".
>
> Indeed it's at least not that friendly to readers... so out of
> curiosity - is this the only reason for this series?

One of the reasons. The main reason is that chardev are *not*
thread-safe, yet we are treating them like if it would be fine to
manipulate from different threads (this oneshot code in the monitor is
one example indeed).

Since the sources are attached to different contexts, if we freeze the
old/new contexts, we have *some* guarantee that concurrent usage will
be a bit more limited...


>
> >
> > Finally, "char-socket: restart the reconnect timer to switch context"
> > shows that we have chardev backends that do not switch fully yet.
>
> This seems irrelevant to the series, or am I wrong?

Kinda related, even with the context freeze proposed here, there may
be concurrent access because we forgot some sources, or other threads
etc.

thanks