[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothr
From: |
Marc-André Lureau |
Subject: |
Re: [Qemu-devel] [PATCH v2 2/6] monitor: accept chardev input from iothread |
Date: |
Mon, 3 Dec 2018 12:04:00 +0400 |
Hi
On Mon, Dec 3, 2018 at 11:26 AM Markus Armbruster <address@hidden> wrote:
>
> Marc-André Lureau <address@hidden> 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 <address@hidden>
> > ---
> > 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