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

[Marc-André Lureau]

Hi

On Wed, Oct 10, 2018 at 7:45 AM Peter Xu <address@hidden> 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 <address@hidden>
> > ---
> >  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