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.

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

diff --git a/monitor.c b/monitor.c
index ab60c9f87e..a25514490a 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4297,6 +4297,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)) {
@@ -4310,13 +4317,14 @@ void monitor_resume(Monitor *mon)
              * let's kick the thread in case it's sleeping.
              */
             if (mon->use_io_thread) {
-                aio_notify(iothread_get_aio_context(mon_iothread));
+                aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
+                                        monitor_accept_input, mon);
             }
         } else {
             assert(mon->rs);
             readline_show_prompt(mon->rs);
+            monitor_accept_input(mon);
         }
-        qemu_chr_fe_accept_input(&mon->chr);
     }
     trace_monitor_suspend(mon, -1);
 }
-- 
2.19.0.271.gfe8321ec05

On Tue, Oct 09, 2018 at 05:12:47PM +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.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  monitor.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index ab60c9f87e..a25514490a 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4297,6 +4297,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)) {
> @@ -4310,13 +4317,14 @@ void monitor_resume(Monitor *mon)
>               * let's kick the thread in case it's sleeping.
>               */
>              if (mon->use_io_thread) {
> -                aio_notify(iothread_get_aio_context(mon_iothread));
> +                aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> +                                        monitor_accept_input, mon);

Just to make sure: this will definitely cover the previous
aio_notify(), am I right?

Imho some comment would always be nice here because QMPs with
use_io_thread=true seems special anyway.

>              }
>          } else {
>              assert(mon->rs);
>              readline_show_prompt(mon->rs);
> +            monitor_accept_input(mon);
>          }
> -        qemu_chr_fe_accept_input(&mon->chr);

How about the QMP monitors with oob=off?  Will it miss the call?

I would consider caching the aio context into Monitor struct when
monitor init, then call aio_bh_schedule_oneshot() always with the
per-monitor aio cache.  This could unify the code paths too so we keep
the oob special path as less as possible.

>      }
>      trace_monitor_suspend(mon, -1);
>  }
> -- 
> 2.19.0.271.gfe8321ec05
> 

Regards,

-- 
Peter Xu

Hi

On Wed, Oct 10, 2018 at 7:45 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Tue, Oct 09, 2018 at 05:12:47PM +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.
> >
> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > ---
> >  monitor.c | 12 ++++++++++--
> >  1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index ab60c9f87e..a25514490a 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4297,6 +4297,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)) {
> > @@ -4310,13 +4317,14 @@ void monitor_resume(Monitor *mon)
> >               * let's kick the thread in case it's sleeping.
> >               */
> >              if (mon->use_io_thread) {
> > -                aio_notify(iothread_get_aio_context(mon_iothread));
> > +                aio_bh_schedule_oneshot(iothread_get_aio_context(mon_iothread),
> > +                                        monitor_accept_input, mon);
>
> Just to make sure: this will definitely cover the previous
> aio_notify(), am I right?
>
> Imho some comment would always be nice here because QMPs with
> use_io_thread=true seems special anyway.
>
> >              }
> >          } else {
> >              assert(mon->rs);
> >              readline_show_prompt(mon->rs);
> > +            monitor_accept_input(mon);
> >          }
> > -        qemu_chr_fe_accept_input(&mon->chr);
>
> How about the QMP monitors with oob=off?  Will it miss the call?

good catch

>
> I would consider caching the aio context into Monitor struct when
> monitor init, then call aio_bh_schedule_oneshot() always with the
> per-monitor aio cache.  This could unify the code paths too so we keep
> the oob special path as less as possible.

Saving the aio context isn't really necessary. However, I'll unify the
code paths in v2.

thanks

>
> >      }
> >      trace_monitor_suspend(mon, -1);
> >  }
> > --
> > 2.19.0.271.gfe8321ec05
> >
>
> Regards,
>
> --
> Peter Xu
>


-- 
Marc-André Lureau