Chardev backends may not handle safely IO events from concurrent
threads (they are not thread-safe in general, only the write path is
since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). 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.

Simplify the condition, based on mon->use_io_thread (only QMP so far).

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 monitor.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/monitor.c b/monitor.c
index d531e8ccc9..79afe99079 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4297,7 +4297,7 @@ int monitor_suspend(Monitor *mon)
 
     atomic_inc(&mon->suspend_cnt);
 
-    if (monitor_is_qmp(mon) && mon->use_io_thread) {
+    if (mon->use_io_thread) {
         /*
          * Kick I/O thread to make sure this takes effect.  It'll be
          * evaluated again in prepare() of the watch object.
@@ -4309,6 +4309,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)) {
@@ -4316,20 +4323,22 @@ void monitor_resume(Monitor *mon)
     }
 
     if (atomic_dec_fetch(&mon->suspend_cnt) == 0) {
-        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));
-            }
+        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);
         }
-        qemu_chr_fe_accept_input(&mon->chr);
+
+        aio_bh_schedule_oneshot(ctx, monitor_accept_input, mon);
     }
+
     trace_monitor_suspend(mon, -1);
 }
 
-- 
2.20.0.rc1

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

> Chardev backends may not handle safely IO events from concurrent
> threads (they are not thread-safe in general, only the write path is

Suggest "may not handle I/O events from concurrent threads safely".

> since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). 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.
>
> Simplify the condition, based on mon->use_io_thread (only QMP so far).

Suggest

  Clean up control flow not to rely on mon->use_io_thread implying
  monitor_is_qmp(mon).

> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>

Happy to improve the commit message in my tree.

Preferably with commit message improvements:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

On Wed, Dec 5, 2018 at 12:21 PM 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 (they are not thread-safe in general, only the write path is
>
> Suggest "may not handle I/O events from concurrent threads safely".
>
> > since commit > 9005b2a7589540a3733b3abdcfbccfe7746cd1a1). 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.
> >
> > Simplify the condition, based on mon->use_io_thread (only QMP so far).
>
> Suggest
>
>   Clean up control flow not to rely on mon->use_io_thread implying
>   monitor_is_qmp(mon).
>
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
>
> Happy to improve the commit message in my tree.
>
> Preferably with commit message improvements:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
>


ack, thanks


-- 
Marc-André Lureau