[Qemu-devel] [PATCH v2 0/6] monitor: misc fixes

[Qemu-devel] [PATCH v2 0/6] monitor: misc fixes

[Marc-André Lureau]

Hi,

Here is a small series of fixes for the monitor, mostly related to
threading issues.

v2: after Peter review
 - patch 2: fix resuming with oob=off
 - patch 4: keep MUX case explicit, improve commit message

Marc-André Lureau (6):
  monitor: inline ambiguous helper functions
  monitor: accept chardev input from iothread
  char: add a QEMU_CHAR_FEATURE_GCONTEXT flag
  monitor: check if chardev can switch gcontext for OOB
  monitor: prevent inserting new monitors after cleanup
  monitor: avoid potential dead-lock when cleaning up

 include/chardev/char.h |  3 +++
 chardev/char.c         | 11 +++++++++
 monitor.c              | 51 +++++++++++++++++++++++++++---------------
 3 files changed, 47 insertions(+), 18 deletions(-)

-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH v2 1/6] monitor: inline ambiguous helper functions

[Marc-André Lureau]

The function were not named with "mon_iothread", or following the AIO
vs GMainContext distinction. Inline them instead.

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

diff --git a/monitor.c b/monitor.c
index 823b5a1099..07712d89f9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4448,16 +4448,6 @@ static void sortcmdlist(void)
     qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd);
 }
 
-static GMainContext *monitor_get_io_context(void)
-{
-    return iothread_get_g_main_context(mon_iothread);
-}
-
-static AioContext *monitor_get_aio_context(void)
-{
-    return iothread_get_aio_context(mon_iothread);
-}
-
 static void monitor_iothread_init(void)
 {
     mon_iothread = iothread_create("mon_iothread", &error_abort);
@@ -4545,7 +4535,7 @@ static void monitor_qmp_setup_handlers_bh(void *opaque)
     GMainContext *context;
 
     assert(mon->use_io_thread);
-    context = monitor_get_io_context();
+    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);
@@ -4597,7 +4587,7 @@ void monitor_init(Chardev *chr, int flags)
              * since chardev might be running in the monitor I/O
              * thread.  Schedule a bottom half.
              */
-            aio_bh_schedule_oneshot(monitor_get_aio_context(),
+            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;
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread

[Marc-André Lureau]

Chardev backends may not handle safely IO events from concurrent
threads. Better to wake up the chardev from the monitor IO thread if
it's being used as the chardev context.

Unify code paths by using a BH in all cases.

Drop the now redundant aio_notify() call.

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

diff --git a/monitor.c b/monitor.c
index 07712d89f9..511dd11d1c 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
     return 0;
 }
 
+static void monitor_accept_input(void *opaque)
+{
+    Monitor *mon = opaque;
+
+    qemu_chr_fe_accept_input(&mon->chr);
+}
+
 void monitor_resume(Monitor *mon)
 {
     if (monitor_is_hmp_non_interactive(mon)) {
@@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
     }
 
     if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
+        AioContext *ctx = qemu_get_aio_context();
+
         if (monitor_is_qmp(mon)) {
             /*
              * For QMP monitors that are running in the I/O thread,
              * let's kick the thread in case it's sleeping.
              */
             if (mon->use_io_thread) {
-                aio_notify(iothread_get_aio_context(mon_iothread));
+                ctx = iothread_get_aio_context(mon_iothread);
             }
         } else {
             assert(mon->rs);
             readline_show_prompt(mon->rs);
         }
-        qemu_chr_fe_accept_input(&mon->chr);
+
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
     }
+
     trace_monitor_suspend(mon, -1);
 }
 
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH v2 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Marc-André Lureau]

The feature should be set if the chardev is able to switch
GMainContext. Callers that want to put a chardev in a different thread
context can/should check this capabilities.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 include/chardev/char.h |  3 +++
 chardev/char.c         | 11 +++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/chardev/char.h b/include/chardev/char.h
index 7becd8c80c..014566c3de 100644
--- a/include/chardev/char.h
+++ b/include/chardev/char.h
@@ -47,6 +47,9 @@ typedef enum {
     QEMU_CHAR_FEATURE_FD_PASS,
     /* Whether replay or record mode is enabled */
     QEMU_CHAR_FEATURE_REPLAY,
+    /* Whether the gcontext can be changed after calling
+     * qemu_chr_be_update_read_handlers() */
+    QEMU_CHAR_FEATURE_GCONTEXT,
 
     QEMU_CHAR_FEATURE_LAST,
 } ChardevFeature;
diff --git a/chardev/char.c b/chardev/char.c
index 7f07a1bfbd..b5ee58b7d2 100644
--- a/chardev/char.c
+++ b/chardev/char.c
@@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
     s->gcontext = context;
     if (cc->chr_update_read_handler) {
         cc->chr_update_read_handler(s);
+    } else if (s->gcontext) {
+        error_report("switching context isn't supported by this chardev");
     }
 }
 
@@ -240,6 +242,15 @@ static void char_init(Object *obj)
 
     chr->logfd = -1;
     qemu_mutex_init(&chr->chr_write_lock);
+
+    /*
+     * Assume if chr_update_read_handler is implemented it will
+     * take the updated gcontext into account.
+     */
+    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
+        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
+    }
+
 }
 
 static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH v2 4/6] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Not all backends are able to switch gcontext. Those backends cannot
drive a OOB monitor (the monitor would then be blocking on main
thread).

For example, ringbuf, spice, or more esoteric input chardevs like
braille or MUX.

We currently forbid MUX because not all frontends are ready to run
outside main loop. Extend to add a context-switching feature check.

Note: this patch will conflict with Peter "[PATCH v9 3/6] monitor:
remove "x-oob", turn oob on by default", but can be trivially updated.

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

diff --git a/monitor.c b/monitor.c
index 511dd11d1c..fffeb27ef9 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4560,9 +4560,11 @@ void monitor_init(Chardev *chr, int flags)
     bool use_oob = flags & MONITOR_USE_OOB;
 
     if (use_oob) {
-        if (CHARDEV_IS_MUX(chr)) {
+        if (CHARDEV_IS_MUX(chr) ||
+            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
             error_report("Monitor out-of-band is not supported with "
-                         "MUX typed chardev backend");
+                         "%s typed chardev backend",
+                         object_get_typename(OBJECT(chr)));
             exit(1);
         }
         if (use_readline) {
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH v2 5/6] monitor: prevent inserting new monitors after cleanup

[Marc-André Lureau]

Add a monitor_destroyed global to check if monitor_cleanup() has been
already called. In this case, don't insert the new monitor in the
list, but free it instead.

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

diff --git a/monitor.c b/monitor.c
index fffeb27ef9..7fe89daa87 100644
--- a/monitor.c
+++ b/monitor.c
@@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest;
 /* QMP checker flags */
 #define QMP_ACCEPT_UNKNOWNS 1
 
-/* Protects mon_list, monitor_qapi_event_state.  */
+/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
 static QemuMutex monitor_lock;
 static GHashTable *monitor_qapi_event_state;
 static QTAILQ_HEAD(mon_list, Monitor) mon_list;
+static bool monitor_destroyed;
 
 /* Protects mon_fdsets */
 static QemuMutex mon_fdsets_lock;
@@ -4536,8 +4537,16 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
 static void monitor_list_append(Monitor *mon)
 {
     qemu_mutex_lock(&monitor_lock);
-    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+    if (!monitor_destroyed) {
+        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
+        mon = NULL;
+    }
     qemu_mutex_unlock(&monitor_lock);
+
+    if (mon) {
+        monitor_data_destroy(mon);
+        g_free(mon);
+    }
 }
 
 static void monitor_qmp_setup_handlers_bh(void *opaque)
@@ -4631,6 +4640,7 @@ void monitor_cleanup(void)
 
     /* Flush output buffers and destroy monitors */
     qemu_mutex_lock(&monitor_lock);
+    monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
         monitor_flush(mon);
-- 
2.19.0.271.gfe8321ec05

[Qemu-devel] [PATCH v2 6/6] monitor: avoid potential dead-lock when cleaning up

[Marc-André Lureau]

When a monitor is connected to a Spice chardev, the monitor cleanup
can dead-lock:

 #0  0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0
 #1  0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0
 #2  0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 <monitor_lock>, file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66
 #3  0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645
 #4  0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 <error_abort>) at qapi/qapi-events-ui.c:149
 #5  0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235
 #6  0x00007f434560a6bb in reds_handle_channel_event (reds=<optimized out>, event=3, info=0x556ddad1b590) at reds.c:316
 #7  0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197
 #8  0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197
 #9  0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414
 #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388
 #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347
 #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0
 #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341
 #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241
 #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340
 #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280
 #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233
 #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786
 #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683
 #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660

Because spice code tries to emit a "disconnected" signal on the
monitors. Fix this dead-lock by releasing the monitor lock for
flush/destroy.

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

diff --git a/monitor.c b/monitor.c
index 7fe89daa87..b55e604a98 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4643,8 +4643,10 @@ void monitor_cleanup(void)
     monitor_destroyed = true;
     QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
         QTAILQ_REMOVE(&mon_list, mon, entry);
+        qemu_mutex_unlock(&monitor_lock);
         monitor_flush(mon);
         monitor_data_destroy(mon);
+        qemu_mutex_lock(&monitor_lock);
         g_free(mon);
     }
     qemu_mutex_unlock(&monitor_lock);
-- 
2.19.0.271.gfe8321ec05

Re: [Qemu-devel] [PATCH v2 1/6] monitor: inline ambiguous helper functions

[Peter Xu]

On Mon, Oct 29, 2018 at 04:57:28PM +0400, Marc-André Lureau wrote:
> The function were not named with "mon_iothread", or following the AIO
> vs GMainContext distinction. Inline them instead.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread

[Peter Xu]

On Mon, Oct 29, 2018 at 04:57:29PM +0400, Marc-André Lureau wrote:
> Chardev backends may not handle safely IO events from concurrent
> threads. Better to wake up the chardev from the monitor IO thread if
> it's being used as the chardev context.
> 
> Unify code paths by using a BH in all cases.
> 
> Drop the now redundant aio_notify() call.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 07712d89f9..511dd11d1c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
>      return 0;
>  }
>  
> +static void monitor_accept_input(void *opaque)
> +{
> +    Monitor *mon = opaque;
> +
> +    qemu_chr_fe_accept_input(&mon->chr);
> +}
> +
>  void monitor_resume(Monitor *mon)
>  {
>      if (monitor_is_hmp_non_interactive(mon)) {
> @@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
>      }
>  
>      if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> +        AioContext *ctx = qemu_get_aio_context();
> +
>          if (monitor_is_qmp(mon)) {
>              /*
>               * For QMP monitors that are running in the I/O thread,
>               * let's kick the thread in case it's sleeping.

This comment seems stall, you may consider to touch it up, otherwise
it looks sane to me:

Reviewed-by: Peter Xu <peterx@redhat.com>

>               */
>              if (mon->use_io_thread) {
> -                aio_notify(iothread_get_aio_context(mon_iothread));
> +                ctx = iothread_get_aio_context(mon_iothread);
>              }
>          } else {
>              assert(mon->rs);
>              readline_show_prompt(mon->rs);
>          }
> -        qemu_chr_fe_accept_input(&mon->chr);
> +
> +        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
>      }
> +
>      trace_monitor_suspend(mon, -1);
>  }
>  
> -- 
> 2.19.0.271.gfe8321ec05
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 5/6] monitor: prevent inserting new monitors after cleanup

[Peter Xu]

On Mon, Oct 29, 2018 at 04:57:32PM +0400, Marc-André Lureau wrote:
> Add a monitor_destroyed global to check if monitor_cleanup() has been
> already called. In this case, don't insert the new monitor in the
> list, but free it instead.

Pure question: how to trigger the condition when doing monitor
cleanups?

I can understand the problem to be fixed in the follow-up patch, but I
don't know whether it's that helpful to have this patch though,
especially considering that we're reaching softfreeze.

> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index fffeb27ef9..7fe89daa87 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest;
>  /* QMP checker flags */
>  #define QMP_ACCEPT_UNKNOWNS 1
>  
> -/* Protects mon_list, monitor_qapi_event_state.  */
> +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
>  static QemuMutex monitor_lock;
>  static GHashTable *monitor_qapi_event_state;
>  static QTAILQ_HEAD(mon_list, Monitor) mon_list;
> +static bool monitor_destroyed;
>  
>  /* Protects mon_fdsets */
>  static QemuMutex mon_fdsets_lock;
> @@ -4536,8 +4537,16 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
>  static void monitor_list_append(Monitor *mon)
>  {
>      qemu_mutex_lock(&monitor_lock);
> -    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> +    if (!monitor_destroyed) {
> +        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> +        mon = NULL;
> +    }
>      qemu_mutex_unlock(&monitor_lock);
> +
> +    if (mon) {
> +        monitor_data_destroy(mon);
> +        g_free(mon);
> +    }
>  }
>  
>  static void monitor_qmp_setup_handlers_bh(void *opaque)
> @@ -4631,6 +4640,7 @@ void monitor_cleanup(void)
>  
>      /* Flush output buffers and destroy monitors */
>      qemu_mutex_lock(&monitor_lock);
> +    monitor_destroyed = true;
>      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>          QTAILQ_REMOVE(&mon_list, mon, entry);
>          monitor_flush(mon);
> -- 
> 2.19.0.271.gfe8321ec05
> 

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 0/6] monitor: misc fixes

[Peter Xu]

On Mon, Oct 29, 2018 at 04:57:27PM +0400, Marc-André Lureau wrote:
> Hi,
> 
> Here is a small series of fixes for the monitor, mostly related to
> threading issues.

Hi, Marc-André, Markus,

I'd be glad to know how you think about these monitor series,
considering that I just noticed it's exactly softfreeze for QEMU 3.1
today...

I gave a quick look on this series, IMHO patch 1,2,6 could be
candidate for 3.1 (though I'm not sure for patch 6; it seems even fine
to me with some comments there but I'd like to see how other people
think about it too, and whether it's really possible to insert a new
monitor at that time) and I'm not sure whether the rest can be
postponed (say, patch 3,4,5).  Meanwhile, I think we need Wolfgang's
patches to fix the daemonize chaos, and hopefully I also like to know
your thoughts on my oob enablement series (especially Marc-André,
since I talked to Markus face to face during the forum after all :).
I would like it to be in 3.1 as well but I'll see my luck.

Thoughts?

Regards,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 0/6] monitor: misc fixes

[Marc-André Lureau]

Hi

On Tue, Oct 30, 2018 at 9:48 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Oct 29, 2018 at 04:57:27PM +0400, Marc-André Lureau wrote:
> > Hi,
> >
> > Here is a small series of fixes for the monitor, mostly related to
> > threading issues.
>
> Hi, Marc-André, Markus,
>
> I'd be glad to know how you think about these monitor series,
> considering that I just noticed it's exactly softfreeze for QEMU 3.1
> today...
>
> I gave a quick look on this series, IMHO patch 1,2,6 could be
> candidate for 3.1 (though I'm not sure for patch 6; it seems even fine
> to me with some comments there but I'd like to see how other people
> think about it too, and whether it's really possible to insert a new
> monitor at that time) and I'm not sure whether the rest can be

Patch 5 & 6 are the result of the discussion of the previous proposed fixed:
https://lists.nongnu.org/archive/html/qemu-devel/2018-08/msg00018.html

They fix a dead-lock on cleanup, so it should be considered for 3.1.

> postponed (say, patch 3,4,5).  Meanwhile, I think we need Wolfgang's

Patch 3 & 4 should also be considered, as OOB won't have the expected
behaviour with all chardevs. With non-gcontext feature chardev, they
will behave more or less like non-oob.

> patches to fix the daemonize chaos, and hopefully I also like to know
> your thoughts on my oob enablement series (especially Marc-André,
> since I talked to Markus face to face during the forum after all :).
> I would like it to be in 3.1 as well but I'll see my luck.

I acked the series already (except "monitor: resume the monitor
earlier if needed" which we agreed could be dropped for now), so it's
in Markus hand.

Thanks

Re: [Qemu-devel] [PATCH v2 0/6] monitor: misc fixes

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi,
>
> Here is a small series of fixes for the monitor, mostly related to
> threading issues.

Series looks good on first glance, but thread safety is subtle business,
so I'd like to give it a bit more time for review.  Bug fixes like these
can go in after soft freeze.

Re: [Qemu-devel] [PATCH v2 1/6] monitor: inline ambiguous helper functions

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> The function were not named with "mon_iothread", or following the AIO
> vs GMainContext distinction. Inline them instead.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Chardev backends may not handle safely IO events from concurrent
> threads.

What exactly could go wrong?  Or is this a well-known fact that doesn't
need further elaboration?

"safely handle I/O events"

>          Better to wake up the chardev from the monitor IO thread if
> it's being used as the chardev context.
>
> Unify code paths by using a BH in all cases.
>
> Drop the now redundant aio_notify() call.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 07712d89f9..511dd11d1c 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
>      return 0;
>  }
>  
> +static void monitor_accept_input(void *opaque)
> +{
> +    Monitor *mon = opaque;
> +
> +    qemu_chr_fe_accept_input(&mon->chr);
> +}
> +
>  void monitor_resume(Monitor *mon)
>  {
>      if (monitor_is_hmp_non_interactive(mon)) {
> @@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
>      }
>  
>      if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> +        AioContext *ctx = qemu_get_aio_context();
> +
>          if (monitor_is_qmp(mon)) {
>              /*
>               * For QMP monitors that are running in the I/O thread,
>               * let's kick the thread in case it's sleeping.
>               */
>              if (mon->use_io_thread) {
> -                aio_notify(iothread_get_aio_context(mon_iothread));
> +                ctx = iothread_get_aio_context(mon_iothread);
>              }
>          } else {
>              assert(mon->rs);
>              readline_show_prompt(mon->rs);
>          }

Correct, since mon->use_io_thread can only be true in a QMP monitor so
far.  But there's no need to depend on that here:

           AioContext *ctx;

           if (mon->use_io_thread) {
               ctx = iothread_get_aio_context(mon_iothread);
           } else {
               ctx = qemu_get_aio_context();
           }

           if (!monitor_is_qmp(mon)) {
               assert(mon->rs);
               readline_show_prompt(mon->rs);
           }

Same in monitor_suspend(), by the way.

The dependence in monitor_init() is tolerable, because it's the place
where the dependence is established.

> -        qemu_chr_fe_accept_input(&mon->chr);
> +
> +        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
>      }
> +
>      trace_monitor_suspend(mon, -1);
>  }

Re: [Qemu-devel] [PATCH v2 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Markus Armbruster]

This one needs review by a chardev guy, with an eye on its use in the
next patch.  Paolo?

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> The feature should be set if the chardev is able to switch
> GMainContext. Callers that want to put a chardev in a different thread
> context can/should check this capabilities.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  include/chardev/char.h |  3 +++
>  chardev/char.c         | 11 +++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/include/chardev/char.h b/include/chardev/char.h
> index 7becd8c80c..014566c3de 100644
> --- a/include/chardev/char.h
> +++ b/include/chardev/char.h
> @@ -47,6 +47,9 @@ typedef enum {
>      QEMU_CHAR_FEATURE_FD_PASS,
>      /* Whether replay or record mode is enabled */
>      QEMU_CHAR_FEATURE_REPLAY,
> +    /* Whether the gcontext can be changed after calling
> +     * qemu_chr_be_update_read_handlers() */
> +    QEMU_CHAR_FEATURE_GCONTEXT,
>  
>      QEMU_CHAR_FEATURE_LAST,
>  } ChardevFeature;
> diff --git a/chardev/char.c b/chardev/char.c
> index 7f07a1bfbd..b5ee58b7d2 100644
> --- a/chardev/char.c
> +++ b/chardev/char.c
> @@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
>      s->gcontext = context;
>      if (cc->chr_update_read_handler) {
>          cc->chr_update_read_handler(s);
> +    } else if (s->gcontext) {
> +        error_report("switching context isn't supported by this chardev");

Code smell: error_report() without returning failure.

If it's truly an error, then the function should fail.  We'd still need
to decide how: error_report() and return -1, or error_setg().

Except if it's a programming error; then we should assert().

If it's not an error, it's probably a warning, and we should use
warn_report().

>      }
>  }
>  
> @@ -240,6 +242,15 @@ static void char_init(Object *obj)
>  
>      chr->logfd = -1;
>      qemu_mutex_init(&chr->chr_write_lock);
> +
> +    /*
> +     * Assume if chr_update_read_handler is implemented it will
> +     * take the updated gcontext into account.
> +     */
> +    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
> +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
> +    }
> +
>  }
>  
>  static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)

Re: [Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread

[Marc-André Lureau]

Hi

On Mon, Dec 3, 2018 at 11:26 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Chardev backends may not handle safely IO events from concurrent
> > threads.
>
> What exactly could go wrong?  Or is this a well-known fact that doesn't
> need further elaboration?

chardev are not thread-safe. Only the write path is, since commit
9005b2a7589540a3733b3abdcfbccfe7746cd1a1.

> "safely handle I/O events"
>
> >          Better to wake up the chardev from the monitor IO thread if
> > it's being used as the chardev context.
> >
> > Unify code paths by using a BH in all cases.
> >
> > Drop the now redundant aio_notify() call.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 07712d89f9..511dd11d1c 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4304,6 +4304,13 @@ int monitor_suspend(Monitor *mon)
> >      return 0;
> >  }
> >
> > +static void monitor_accept_input(void *opaque)
> > +{
> > +    Monitor *mon = opaque;
> > +
> > +    qemu_chr_fe_accept_input(&mon->chr);
> > +}
> > +
> >  void monitor_resume(Monitor *mon)
> >  {
> >      if (monitor_is_hmp_non_interactive(mon)) {
> > @@ -4311,20 +4318,24 @@ void monitor_resume(Monitor *mon)
> >      }
> >
> >      if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
> > +        AioContext *ctx = qemu_get_aio_context();
> > +
> >          if (monitor_is_qmp(mon)) {
> >              /*
> >               * For QMP monitors that are running in the I/O thread,
> >               * let's kick the thread in case it's sleeping.
> >               */
> >              if (mon->use_io_thread) {
> > -                aio_notify(iothread_get_aio_context(mon_iothread));
> > +                ctx = iothread_get_aio_context(mon_iothread);
> >              }
> >          } else {
> >              assert(mon->rs);
> >              readline_show_prompt(mon->rs);
> >          }
>
> Correct, since mon->use_io_thread can only be true in a QMP monitor so
> far.  But there's no need to depend on that here:
>
>            AioContext *ctx;
>
>            if (mon->use_io_thread) {
>                ctx = iothread_get_aio_context(mon_iothread);
>            } else {
>                ctx = qemu_get_aio_context();
>            }
>
>            if (!monitor_is_qmp(mon)) {
>                assert(mon->rs);
>                readline_show_prompt(mon->rs);
>            }
>
> Same in monitor_suspend(), by the way.
>

ok


> The dependence in monitor_init() is tolerable, because it's the place
> where the dependence is established.
>
> > -        qemu_chr_fe_accept_input(&mon->chr);
> > +
> > +        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
> >      }
> > +
> >      trace_monitor_suspend(mon, -1);
> >  }
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 3/6] char: add a QEMU_CHAR_FEATURE_GCONTEXT flag

[Marc-André Lureau]

Hi

On Mon, Dec 3, 2018 at 11:25 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> This one needs review by a chardev guy, with an eye on its use in the
> next patch.  Paolo?
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > The feature should be set if the chardev is able to switch
> > GMainContext. Callers that want to put a chardev in a different thread
> > context can/should check this capabilities.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  include/chardev/char.h |  3 +++
> >  chardev/char.c         | 11 +++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/include/chardev/char.h b/include/chardev/char.h
> > index 7becd8c80c..014566c3de 100644
> > --- a/include/chardev/char.h
> > +++ b/include/chardev/char.h
> > @@ -47,6 +47,9 @@ typedef enum {
> >      QEMU_CHAR_FEATURE_FD_PASS,
> >      /* Whether replay or record mode is enabled */
> >      QEMU_CHAR_FEATURE_REPLAY,
> > +    /* Whether the gcontext can be changed after calling
> > +     * qemu_chr_be_update_read_handlers() */
> > +    QEMU_CHAR_FEATURE_GCONTEXT,
> >
> >      QEMU_CHAR_FEATURE_LAST,
> >  } ChardevFeature;
> > diff --git a/chardev/char.c b/chardev/char.c
> > index 7f07a1bfbd..b5ee58b7d2 100644
> > --- a/chardev/char.c
> > +++ b/chardev/char.c
> > @@ -196,6 +196,8 @@ void qemu_chr_be_update_read_handlers(Chardev *s,
> >      s->gcontext = context;
> >      if (cc->chr_update_read_handler) {
> >          cc->chr_update_read_handler(s);
> > +    } else if (s->gcontext) {
> > +        error_report("switching context isn't supported by this chardev");
>
> Code smell: error_report() without returning failure.
>
> If it's truly an error, then the function should fail.  We'd still need
> to decide how: error_report() and return -1, or error_setg().
>
> Except if it's a programming error; then we should assert().
>

After this patch, it's an error.
There should be a preliminary check for qemu_chr_has_feature(chr,
QEMU_CHAR_FEATURE_GCONTEXT) before switching context.

I'll replace the error_report() with an assert.

> If it's not an error, it's probably a warning, and we should use
> warn_report().
>
> >      }
> >  }
> >
> > @@ -240,6 +242,15 @@ static void char_init(Object *obj)
> >
> >      chr->logfd = -1;
> >      qemu_mutex_init(&chr->chr_write_lock);
> > +
> > +    /*
> > +     * Assume if chr_update_read_handler is implemented it will
> > +     * take the updated gcontext into account.
> > +     */
> > +    if (CHARDEV_GET_CLASS(chr)->chr_update_read_handler) {
> > +        qemu_chr_set_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT);
> > +    }
> > +
> >  }
> >
> >  static int null_chr_write(Chardev *chr, const uint8_t *buf, int len)
>


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH v2 4/6] monitor: check if chardev can switch gcontext for OOB

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Not all backends are able to switch gcontext. Those backends cannot
> drive a OOB monitor (the monitor would then be blocking on main
> thread).
>
> For example, ringbuf, spice, or more esoteric input chardevs like
> braille or MUX.
>
> We currently forbid MUX because not all frontends are ready to run
> outside main loop. Extend to add a context-switching feature check.

Double-checking: the reason for forbidding MUX and the reason for
requiring QEMU_CHAR_FEATURE_GCONTEXT are orthogonal, right?

> Note: this patch will conflict with Peter "[PATCH v9 3/6] monitor:
> remove "x-oob", turn oob on by default", but can be trivially updated.

I intend to merge this series first, and drop this sentence.

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index 511dd11d1c..fffeb27ef9 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4560,9 +4560,11 @@ void monitor_init(Chardev *chr, int flags)
>      bool use_oob = flags & MONITOR_USE_OOB;
>  
>      if (use_oob) {
> -        if (CHARDEV_IS_MUX(chr)) {
> +        if (CHARDEV_IS_MUX(chr) ||
> +            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
>              error_report("Monitor out-of-band is not supported with "
> -                         "MUX typed chardev backend");
> +                         "%s typed chardev backend",
> +                         object_get_typename(OBJECT(chr)));
>              exit(1);
>          }
>          if (use_readline) {

Re: [Qemu-devel] [PATCH v2 4/6] monitor: check if chardev can switch gcontext for OOB

[Marc-André Lureau]

Hi

On Mon, Dec 3, 2018 at 12:23 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Not all backends are able to switch gcontext. Those backends cannot
> > drive a OOB monitor (the monitor would then be blocking on main
> > thread).
> >
> > For example, ringbuf, spice, or more esoteric input chardevs like
> > braille or MUX.
> >
> > We currently forbid MUX because not all frontends are ready to run
> > outside main loop. Extend to add a context-switching feature check.
>
> Double-checking: the reason for forbidding MUX and the reason for
> requiring QEMU_CHAR_FEATURE_GCONTEXT are orthogonal, right?

Yes, to me the mux check is there because other front-end sides (other
than QMP monitor) may not handle dispatch from other threads.

Otoh, the GCONTEXT feature check is there to check if the chardev
(backend) handles switching context.


> > Note: this patch will conflict with Peter "[PATCH v9 3/6] monitor:
> > remove "x-oob", turn oob on by default", but can be trivially updated.
>
> I intend to merge this series first, and drop this sentence.
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 511dd11d1c..fffeb27ef9 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4560,9 +4560,11 @@ void monitor_init(Chardev *chr, int flags)
> >      bool use_oob = flags & MONITOR_USE_OOB;
> >
> >      if (use_oob) {
> > -        if (CHARDEV_IS_MUX(chr)) {
> > +        if (CHARDEV_IS_MUX(chr) ||
> > +            !qemu_chr_has_feature(chr, QEMU_CHAR_FEATURE_GCONTEXT)) {
> >              error_report("Monitor out-of-band is not supported with "
> > -                         "MUX typed chardev backend");
> > +                         "%s typed chardev backend",
> > +                         object_get_typename(OBJECT(chr)));
> >              exit(1);
> >          }
> >          if (use_readline) {
>

Re: [Qemu-devel] [PATCH v2 5/6] monitor: prevent inserting new monitors after cleanup

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Add a monitor_destroyed global to check if monitor_cleanup() has been
> already called. In this case, don't insert the new monitor in the
> list, but free it instead.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

The commit message explains what the patch does, but not why we want to
do it.

> ---
>  monitor.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/monitor.c b/monitor.c
> index fffeb27ef9..7fe89daa87 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest;
>  /* QMP checker flags */
>  #define QMP_ACCEPT_UNKNOWNS 1
>  
> -/* Protects mon_list, monitor_qapi_event_state.  */
> +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
>  static QemuMutex monitor_lock;
>  static GHashTable *monitor_qapi_event_state;
>  static QTAILQ_HEAD(mon_list, Monitor) mon_list;
> +static bool monitor_destroyed;
>  
>  /* Protects mon_fdsets */
>  static QemuMutex mon_fdsets_lock;
> @@ -4536,8 +4537,16 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
>  static void monitor_list_append(Monitor *mon)
>  {
>      qemu_mutex_lock(&monitor_lock);
> -    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> +    if (!monitor_destroyed) {
> +        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> +        mon = NULL;
> +    }
>      qemu_mutex_unlock(&monitor_lock);
> +
> +    if (mon) {
> +        monitor_data_destroy(mon);
> +        g_free(mon);
> +    }
>  }
>  
>  static void monitor_qmp_setup_handlers_bh(void *opaque)
> @@ -4631,6 +4640,7 @@ void monitor_cleanup(void)
>  
>      /* Flush output buffers and destroy monitors */
>      qemu_mutex_lock(&monitor_lock);
> +    monitor_destroyed = true;
>      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>          QTAILQ_REMOVE(&mon_list, mon, entry);
>          monitor_flush(mon);

monitor_cleanup() is one of the last things main() calls before it
returns.  If another thread creates a monitor afterwards, it won't be
cleaned up.  I figure that's the reason we want this patch.  There might
be more serious issues than failure to clean up.  Please explain in your
commit message.

monitor_list_append() is called by monitor_init(), either directly right
before it returns, or via a bottom half if mon->use_io_thread.
Therefore, @monitor_destroyed can only be true if monitor_init()
commonly runs in a thread other than the main thread.  Please give an
example where it does, in your commit message.

Re: [Qemu-devel] [PATCH v2 6/6] monitor: avoid potential dead-lock when cleaning up

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> When a monitor is connected to a Spice chardev, the monitor cleanup
> can dead-lock:
>
>  #0  0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0
>  #1  0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0
>  #2  0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 <monitor_lock>, file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66
>  #3  0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645
>  #4  0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 <error_abort>) at qapi/qapi-events-ui.c:149
>  #5  0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235
>  #6  0x00007f434560a6bb in reds_handle_channel_event (reds=<optimized out>, event=3, info=0x556ddad1b590) at reds.c:316
>  #7  0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197
>  #8  0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197
>  #9  0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414
>  #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388
>  #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347
>  #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0
>  #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341
>  #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241
>  #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340
>  #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280
>  #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233
>  #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786
>  #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683
>  #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660
>
> Because spice code tries to emit a "disconnected" signal on the
> monitors. Fix this dead-lock by releasing the monitor lock for
> flush/destroy.
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/monitor.c b/monitor.c
> index 7fe89daa87..b55e604a98 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4643,8 +4643,10 @@ void monitor_cleanup(void)
>      monitor_destroyed = true;
>      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>          QTAILQ_REMOVE(&mon_list, mon, entry);
> +        qemu_mutex_unlock(&monitor_lock);
>          monitor_flush(mon);
>          monitor_data_destroy(mon);
> +        qemu_mutex_lock(&monitor_lock);
>          g_free(mon);
>      }
>      qemu_mutex_unlock(&monitor_lock);

I think a comment hinting at the reason for relinquishing the lock would
be in order.  Perhaps

           /* Permit QAPI event emission from character frontend release */
           qemu_mutex_unlock(&monitor_lock);

We need to demonstrate calling monitor_flush() and
monitor_data_destroy() without holding @monitor_lock is safe.

@monitor_lock's comment states it "protects mon_list,
monitor_qapi_event_state."  Looks plausible from how it's used.

As far as I can tell, monitor_flush() and monitor_data_destroy() don't
access mon_list and monitor_qapi_event_state.

monitor_cleanup()'s loop itself is safe because it uses
QTAILQ_FOREACH_SAFE(), unlike similar loops elsewhere.

I think we're good.  I'd like you to work this argument into the commit
message.

Re: [Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@gmail.com> writes:

> Hi
>
> On Mon, Dec 3, 2018 at 11:26 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Chardev backends may not handle safely IO events from concurrent
>> > threads.
>>
>> What exactly could go wrong?  Or is this a well-known fact that doesn't
>> need further elaboration?
>
> chardev are not thread-safe. Only the write path is, since commit
> 9005b2a7589540a3733b3abdcfbccfe7746cd1a1.

Add this to your commit message?  Your call.

>> "safely handle I/O events"
>>
>> >          Better to wake up the chardev from the monitor IO thread if
>> > it's being used as the chardev context.
>> >
>> > Unify code paths by using a BH in all cases.
>> >
>> > Drop the now redundant aio_notify() call.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[...]

Re: [Qemu-devel] [PATCH v2 0/6] monitor: misc fixes

[Markus Armbruster]

Found only minor issues.  v3 should do the trick.  Thanks!

Re: [Qemu-devel] [PATCH v2 5/6] monitor: prevent inserting new monitors after cleanup

[Marc-André Lureau]

Hi
On Mon, Dec 3, 2018 at 12:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > Add a monitor_destroyed global to check if monitor_cleanup() has been
> > already called. In this case, don't insert the new monitor in the
> > list, but free it instead.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> The commit message explains what the patch does, but not why we want to
> do it.
>
> > ---
> >  monitor.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index fffeb27ef9..7fe89daa87 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest;
> >  /* QMP checker flags */
> >  #define QMP_ACCEPT_UNKNOWNS 1
> >
> > -/* Protects mon_list, monitor_qapi_event_state.  */
> > +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
> >  static QemuMutex monitor_lock;
> >  static GHashTable *monitor_qapi_event_state;
> >  static QTAILQ_HEAD(mon_list, Monitor) mon_list;
> > +static bool monitor_destroyed;
> >
> >  /* Protects mon_fdsets */
> >  static QemuMutex mon_fdsets_lock;
> > @@ -4536,8 +4537,16 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
> >  static void monitor_list_append(Monitor *mon)
> >  {
> >      qemu_mutex_lock(&monitor_lock);
> > -    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> > +    if (!monitor_destroyed) {
> > +        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
> > +        mon = NULL;
> > +    }
> >      qemu_mutex_unlock(&monitor_lock);
> > +
> > +    if (mon) {
> > +        monitor_data_destroy(mon);
> > +        g_free(mon);
> > +    }
> >  }
> >
> >  static void monitor_qmp_setup_handlers_bh(void *opaque)
> > @@ -4631,6 +4640,7 @@ void monitor_cleanup(void)
> >
> >      /* Flush output buffers and destroy monitors */
> >      qemu_mutex_lock(&monitor_lock);
> > +    monitor_destroyed = true;
> >      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
> >          QTAILQ_REMOVE(&mon_list, mon, entry);
> >          monitor_flush(mon);
>
> monitor_cleanup() is one of the last things main() calls before it
> returns.  If another thread creates a monitor afterwards, it won't be
> cleaned up.  I figure that's the reason we want this patch.  There might
> be more serious issues than failure to clean up.  Please explain in your
> commit message.

Is this clearer?

    monitor_cleanup() is one of the last things main() calls before it
    returns.  In the following patch, monitor_cleanup() will release the
    monitor_lock during flushing. There may be pending commands to insert
    new monitors, which would modify the mon_list during iteration, and
    the clean-up could thus miss those new insertions.

    Add a monitor_destroyed global to check if monitor_cleanup() has been
    already called. In this case, don't insert the new monitor in the
    list, but free it instead.

> monitor_list_append() is called by monitor_init(), either directly right
> before it returns, or via a bottom half if mon->use_io_thread.
> Therefore, @monitor_destroyed can only be true if monitor_init()
> commonly runs in a thread other than the main thread.  Please give an
> example where it does, in your commit message.

Re: [Qemu-devel] [PATCH v2 6/6] monitor: avoid potential dead-lock when cleaning up

[Marc-André Lureau]

On Mon, Dec 3, 2018 at 1:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
> > When a monitor is connected to a Spice chardev, the monitor cleanup
> > can dead-lock:
> >
> >  #0  0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0
> >  #1  0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0
> >  #2  0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 <monitor_lock>, file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66
> >  #3  0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645
> >  #4  0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 <error_abort>) at qapi/qapi-events-ui.c:149
> >  #5  0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235
> >  #6  0x00007f434560a6bb in reds_handle_channel_event (reds=<optimized out>, event=3, info=0x556ddad1b590) at reds.c:316
> >  #7  0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197
> >  #8  0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197
> >  #9  0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414
> >  #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388
> >  #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347
> >  #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0
> >  #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341
> >  #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241
> >  #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340
> >  #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280
> >  #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233
> >  #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786
> >  #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683
> >  #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660
> >
> > Because spice code tries to emit a "disconnected" signal on the
> > monitors. Fix this dead-lock by releasing the monitor lock for
> > flush/destroy.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/monitor.c b/monitor.c
> > index 7fe89daa87..b55e604a98 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4643,8 +4643,10 @@ void monitor_cleanup(void)
> >      monitor_destroyed = true;
> >      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
> >          QTAILQ_REMOVE(&mon_list, mon, entry);
> > +        qemu_mutex_unlock(&monitor_lock);
> >          monitor_flush(mon);
> >          monitor_data_destroy(mon);
> > +        qemu_mutex_lock(&monitor_lock);
> >          g_free(mon);
> >      }
> >      qemu_mutex_unlock(&monitor_lock);
>
> I think a comment hinting at the reason for relinquishing the lock would
> be in order.  Perhaps
>
>            /* Permit QAPI event emission from character frontend release */
>            qemu_mutex_unlock(&monitor_lock);
>
> We need to demonstrate calling monitor_flush() and
> monitor_data_destroy() without holding @monitor_lock is safe.
>
> @monitor_lock's comment states it "protects mon_list,
> monitor_qapi_event_state."  Looks plausible from how it's used.
>
> As far as I can tell, monitor_flush() and monitor_data_destroy() don't
> access mon_list and monitor_qapi_event_state.
>
> monitor_cleanup()'s loop itself is safe because it uses
> QTAILQ_FOREACH_SAFE(), unlike similar loops elsewhere.
>
> I think we're good.  I'd like you to work this argument into the commit
> message.

What about adding:

    monitor_lock protects mon_list, monitor_qapi_event_state and
    monitor_destroyed. monitor_flush() and monitor_data_destroy() don't
    access any of those variables.

    monitor_cleanup()'s loop is safe because it uses
    QTAILQ_FOREACH_SAFE(), and no further monitor can be added after
    calling monitor_cleanup() thanks to monitor_destroyed check in
    monitor_list_append().

Re: [Qemu-devel] [PATCH v2 5/6] monitor: prevent inserting new monitors after cleanup

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
> On Mon, Dec 3, 2018 at 12:59 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Add a monitor_destroyed global to check if monitor_cleanup() has been
>> > already called. In this case, don't insert the new monitor in the
>> > list, but free it instead.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>
>> The commit message explains what the patch does, but not why we want to
>> do it.
>>
>> > ---
>> >  monitor.c | 14 ++++++++++++--
>> >  1 file changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index fffeb27ef9..7fe89daa87 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -263,10 +263,11 @@ typedef struct QMPRequest QMPRequest;
>> >  /* QMP checker flags */
>> >  #define QMP_ACCEPT_UNKNOWNS 1
>> >
>> > -/* Protects mon_list, monitor_qapi_event_state.  */
>> > +/* Protects mon_list, monitor_qapi_event_state, monitor_destroyed.  */
>> >  static QemuMutex monitor_lock;
>> >  static GHashTable *monitor_qapi_event_state;
>> >  static QTAILQ_HEAD(mon_list, Monitor) mon_list;
>> > +static bool monitor_destroyed;
>> >
>> >  /* Protects mon_fdsets */
>> >  static QemuMutex mon_fdsets_lock;
>> > @@ -4536,8 +4537,16 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap)
>> >  static void monitor_list_append(Monitor *mon)
>> >  {
>> >      qemu_mutex_lock(&monitor_lock);
>> > -    QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
>> > +    if (!monitor_destroyed) {
>> > +        QTAILQ_INSERT_HEAD(&mon_list, mon, entry);
>> > +        mon = NULL;
>> > +    }
>> >      qemu_mutex_unlock(&monitor_lock);
>> > +
>> > +    if (mon) {
>> > +        monitor_data_destroy(mon);
>> > +        g_free(mon);
>> > +    }
>> >  }
>> >
>> >  static void monitor_qmp_setup_handlers_bh(void *opaque)
>> > @@ -4631,6 +4640,7 @@ void monitor_cleanup(void)
>> >
>> >      /* Flush output buffers and destroy monitors */
>> >      qemu_mutex_lock(&monitor_lock);
>> > +    monitor_destroyed = true;
>> >      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>> >          QTAILQ_REMOVE(&mon_list, mon, entry);
>> >          monitor_flush(mon);
>>
>> monitor_cleanup() is one of the last things main() calls before it
>> returns.  If another thread creates a monitor afterwards, it won't be
>> cleaned up.  I figure that's the reason we want this patch.  There might
>> be more serious issues than failure to clean up.  Please explain in your
>> commit message.
>
> Is this clearer?
>
>     monitor_cleanup() is one of the last things main() calls before it
>     returns.  In the following patch, monitor_cleanup() will release the
>     monitor_lock during flushing. There may be pending commands to insert
>     new monitors, which would modify the mon_list during iteration, and
>     the clean-up could thus miss those new insertions.
>
>     Add a monitor_destroyed global to check if monitor_cleanup() has been
>     already called. In this case, don't insert the new monitor in the
>     list, but free it instead.

Yes.

The solution feels a bit clunky: it duplicates part of the work
monitor_cleanup() does into monitor_list_append().  Works because the
part it doesn't duplicate needn't be done in this case: removing from
@mon_list, and monitor_flush().

I suspect a cleaner solution would involve the main thread telling other
threads to terminate, waiting for their termination with pthread_join(),
and only then do final cleanup.

I'm not asking for that solution now.  A clunky fix we have is better
than a refactoring we don't have.  A comment admitting the clunkiness
would be nice.

>> monitor_list_append() is called by monitor_init(), either directly right
>> before it returns, or via a bottom half if mon->use_io_thread.
>> Therefore, @monitor_destroyed can only be true if monitor_init()
>> commonly runs in a thread other than the main thread.  Please give an
>> example where it does, in your commit message.

Re: [Qemu-devel] [PATCH v2 6/6] monitor: avoid potential dead-lock when cleaning up

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> On Mon, Dec 3, 2018 at 1:26 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > When a monitor is connected to a Spice chardev, the monitor cleanup
>> > can dead-lock:
>> >
>> >  #0  0x00007f43446637fd in __lll_lock_wait () at /lib64/libpthread.so.0
>> >  #1  0x00007f434465ccf4 in pthread_mutex_lock () at /lib64/libpthread.so.0
>> >  #2  0x0000556dd79f22ba in qemu_mutex_lock_impl (mutex=0x556dd81c9220 <monitor_lock>, file=0x556dd7ae3648 "/home/elmarco/src/qq/monitor.c", line=645) at /home/elmarco/src/qq/util/qemu-thread-posix.c:66
>> >  #3  0x0000556dd7431bd5 in monitor_qapi_event_queue (event=QAPI_EVENT_SPICE_DISCONNECTED, qdict=0x556dd9abc850, errp=0x7fffb7bbddd8) at /home/elmarco/src/qq/monitor.c:645
>> >  #4  0x0000556dd79d476b in qapi_event_send_spice_disconnected (server=0x556dd98ee760, client=0x556ddaaa8560, errp=0x556dd82180d0 <error_abort>) at qapi/qapi-events-ui.c:149
>> >  #5  0x0000556dd7870fc1 in channel_event (event=3, info=0x556ddad1b590) at /home/elmarco/src/qq/ui/spice-core.c:235
>> >  #6  0x00007f434560a6bb in reds_handle_channel_event (reds=<optimized out>, event=3, info=0x556ddad1b590) at reds.c:316
>> >  #7  0x00007f43455f393b in main_dispatcher_self_handle_channel_event (info=0x556ddad1b590, event=3, self=0x556dd9a7d8c0) at main-dispatcher.c:197
>> >  #8  0x00007f43455f393b in main_dispatcher_channel_event (self=0x556dd9a7d8c0, event=event@entry=3, info=0x556ddad1b590) at main-dispatcher.c:197
>> >  #9  0x00007f4345612833 in red_stream_push_channel_event (s=s@entry=0x556ddae2ef40, event=event@entry=3) at red-stream.c:414
>> >  #10 0x00007f434561286b in red_stream_free (s=0x556ddae2ef40) at red-stream.c:388
>> >  #11 0x00007f43455f9ddc in red_channel_client_finalize (object=0x556dd9bb21a0) at red-channel-client.c:347
>> >  #12 0x00007f434b5f9fb9 in g_object_unref () at /lib64/libgobject-2.0.so.0
>> >  #13 0x00007f43455fc212 in red_channel_client_push (rcc=0x556dd9bb21a0) at red-channel-client.c:1341
>> >  #14 0x0000556dd76081ba in spice_port_set_fe_open (chr=0x556dd9925e20, fe_open=0) at /home/elmarco/src/qq/chardev/spice.c:241
>> >  #15 0x0000556dd796d74a in qemu_chr_fe_set_open (be=0x556dd9a37c00, fe_open=0) at /home/elmarco/src/qq/chardev/char-fe.c:340
>> >  #16 0x0000556dd796d4d9 in qemu_chr_fe_set_handlers (b=0x556dd9a37c00, fd_can_read=0x0, fd_read=0x0, fd_event=0x0, be_change=0x0, opaque=0x0, context=0x0, set_open=true) at /home/elmarco/src/qq/chardev/char-fe.c:280
>> >  #17 0x0000556dd796d359 in qemu_chr_fe_deinit (b=0x556dd9a37c00, del=false) at /home/elmarco/src/qq/chardev/char-fe.c:233
>> >  #18 0x0000556dd7432240 in monitor_data_destroy (mon=0x556dd9a37c00) at /home/elmarco/src/qq/monitor.c:786
>> >  #19 0x0000556dd743b968 in monitor_cleanup () at /home/elmarco/src/qq/monitor.c:4683
>> >  #20 0x0000556dd75ce776 in main (argc=3, argv=0x7fffb7bbe458, envp=0x7fffb7bbe478) at /home/elmarco/src/qq/vl.c:4660
>> >
>> > Because spice code tries to emit a "disconnected" signal on the
>> > monitors. Fix this dead-lock by releasing the monitor lock for
>> > flush/destroy.
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> >  monitor.c | 2 ++
>> >  1 file changed, 2 insertions(+)
>> >
>> > diff --git a/monitor.c b/monitor.c
>> > index 7fe89daa87..b55e604a98 100644
>> > --- a/monitor.c
>> > +++ b/monitor.c
>> > @@ -4643,8 +4643,10 @@ void monitor_cleanup(void)
>> >      monitor_destroyed = true;
>> >      QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {
>> >          QTAILQ_REMOVE(&mon_list, mon, entry);
>> > +        qemu_mutex_unlock(&monitor_lock);
>> >          monitor_flush(mon);
>> >          monitor_data_destroy(mon);
>> > +        qemu_mutex_lock(&monitor_lock);
>> >          g_free(mon);
>> >      }
>> >      qemu_mutex_unlock(&monitor_lock);
>>
>> I think a comment hinting at the reason for relinquishing the lock would
>> be in order.  Perhaps
>>
>>            /* Permit QAPI event emission from character frontend release */
>>            qemu_mutex_unlock(&monitor_lock);
>>
>> We need to demonstrate calling monitor_flush() and
>> monitor_data_destroy() without holding @monitor_lock is safe.
>>
>> @monitor_lock's comment states it "protects mon_list,
>> monitor_qapi_event_state."  Looks plausible from how it's used.
>>
>> As far as I can tell, monitor_flush() and monitor_data_destroy() don't
>> access mon_list and monitor_qapi_event_state.
>>
>> monitor_cleanup()'s loop itself is safe because it uses
>> QTAILQ_FOREACH_SAFE(), unlike similar loops elsewhere.
>>
>> I think we're good.  I'd like you to work this argument into the commit
>> message.
>
> What about adding:
>
>     monitor_lock protects mon_list, monitor_qapi_event_state and
>     monitor_destroyed. monitor_flush() and monitor_data_destroy() don't
>     access any of those variables.
>
>     monitor_cleanup()'s loop is safe because it uses
>     QTAILQ_FOREACH_SAFE(), and no further monitor can be added after
>     calling monitor_cleanup() thanks to monitor_destroyed check in
>     monitor_list_append().

Works for me.  Thanks!