Hi On Mon, Dec 3, 2018 at 11:26 AM Markus Armbruster <[email protected]> wrote: > > Marc-André Lureau <[email protected]> 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 <[email protected]> > > --- > > 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
