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

[Markus Armbruster <armbru@redhat.com>]

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);
>  }