Re: [Qemu-devel] [PATCH v2 2/2] monitor: delay monitor iothread creation

[Peter Xu]

On Fri, Sep 28, 2018 at 01:00:26PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Fri, Sep 28, 2018 at 12:02 PM Wolfgang Bumiller
> <address@hidden> wrote:
> >
> > Commit d32749deb615 moved the call to monitor_init_globals()
> > to before os_daemonize(), making it an unsuitable place to
> > spawn the monitor iothread as it won't be inherited over the
> > fork() in os_daemonize().
> >
> > We now spawn the thread the first time we instantiate a
> > monitor which actually has use_io_thread == true. Therefore
> > mon_iothread initialization is protected by monitor_lock.
> >
> > We still need to create the qmp_dispatcher_bh when not using
> > iothreads, so this now still happens via
> > monitor_init_globals().
> >
> > Signed-off-by: Wolfgang Bumiller <address@hidden>
> > Fixes: d32749deb615 ("monitor: move init global earlier")
> > ---
> > Changes to v1:
> >  - move mon_iothread declaration down to monitor_lock's declaration
> >    (updating monitor_lock's coverage comment)
> >  - in monitor_data_init() assert() that mon_iothread is not NULL or
> >    not used instead of initializing it there, as its usage pattern is
> >    so that it is a initialized once before being used, or never used
> >    at all.
> >  - in monitor_iothread_init(), protect mon_iothread initialization
> >    with monitor_lock
> >  - in monitor_init(): run monitor_ithread_init() in the `use_oob`
> >    branch.
> >    Note that I currently also test for mon_iothread being NULL there,
> >    which we could leave this out as spawning new monitors isn't
> >    something that happens a lot, but I like the idea of avoiding
> >    taking a lock when not required.
> >    Otherwise, I can send a v3 with this removed.
> >
> >  monitor.c | 49 ++++++++++++++++++++++++++++++-------------------
> >  1 file changed, 30 insertions(+), 19 deletions(-)
> >
> > diff --git a/monitor.c b/monitor.c
> > index d47e4259fd..870584a548 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -239,9 +239,6 @@ struct Monitor {
> >      int mux_out;
> >  };
> >
> > -/* Shared monitor I/O thread */
> > -IOThread *mon_iothread;
> > -
> >  /* Bottom half to dispatch the requests received from I/O thread */
> >  QEMUBH *qmp_dispatcher_bh;
> >
> > @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest;
> >  /* QMP checker flags */
> >  #define QMP_ACCEPT_UNKNOWNS 1
> >
> > -/* Protects mon_list, monitor_qapi_event_state.  */
> > +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */
> >  static QemuMutex monitor_lock;
> >  static GHashTable *monitor_qapi_event_state;
> >  static QTAILQ_HEAD(mon_list, Monitor) mon_list;
> > +IOThread *mon_iothread; /* Shared monitor I/O thread */
> >
> >  /* Protects mon_fdsets */
> >  static QemuMutex mon_fdsets_lock;
> > @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char 
> > *cmdline);
> >  static void monitor_data_init(Monitor *mon, bool skip_flush,
> >                                bool use_io_thread)
> >  {
> > +    assert(!use_io_thread || mon_iothread);
> >      memset(mon, 0, sizeof(Monitor));
> >      qemu_mutex_init(&mon->mon_lock);
> >      qemu_mutex_init(&mon->qmp.qmp_queue_lock);
> > @@ -4453,16 +4452,11 @@ static AioContext *monitor_get_aio_context(void)
> >
> >  static void monitor_iothread_init(void)
> >  {
> > -    mon_iothread = iothread_create("mon_iothread", &error_abort);
> > -
> > -    /*
> > -     * The dispatcher BH must run in the main loop thread, since we
> > -     * have commands assuming that context.  It would be nice to get
> > -     * rid of those assumptions.
> > -     */
> > -    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
> > -                                   monitor_qmp_bh_dispatcher,
> > -                                   NULL);
> > +    qemu_mutex_lock(&monitor_lock);
> > +    if (!mon_iothread) {
> > +        mon_iothread = iothread_create("mon_iothread", &error_abort);
> > +    }
> > +    qemu_mutex_unlock(&monitor_lock);
> >  }
> >
> >  void monitor_init_globals(void)
> > @@ -4472,7 +4466,15 @@ void monitor_init_globals(void)
> >      sortcmdlist();
> >      qemu_mutex_init(&monitor_lock);
> >      qemu_mutex_init(&mon_fdsets_lock);
> > -    monitor_iothread_init();
> > +
> > +    /*
> > +     * The dispatcher BH must run in the main loop thread, since we
> > +     * have commands assuming that context.  It would be nice to get
> > +     * rid of those assumptions.
> > +     */
> > +    qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
> > +                                   monitor_qmp_bh_dispatcher,
> > +                                   NULL);
> >  }
> >
> >  /* These functions just adapt the readline interface in a typesafe way.  We
> > @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void 
> > *opaque)
> >      monitor_list_append(mon);
> >  }
> >
> > +/*
> > + * This expects to be run in the main thread.
> > + */
> 
> I read that Markus suggested that comment, but I don't really get why.
> 
> It means that callers (chardev new) should also be restricted to main thread.

My understanding is that Markus mentioned about uncertainty on the
chardev creation paths.  Though AFAIU if we're with the lock then we
don't need this comment at all, do we?

> 
> >  void monitor_init(Chardev *chr, int flags)
> >  {
> >      Monitor *mon = g_malloc(sizeof(*mon));
> > @@ -4551,6 +4556,9 @@ void monitor_init(Chardev *chr, int flags)
> >              error_report("Monitor out-of-band is only supported by QMP");
> >              exit(1);
> >          }
> > +        if (!mon_iothread) {
> > +            monitor_iothread_init();
> > +        }
> 
> I would call it unconditonnally, to avoid TOCTOU.

Yeh agree that the "if" could be omitted; though there shouldn't be
toctou since the function will check it again.

Thanks,

> 
> >      }
> >
> >      monitor_data_init(mon, false, use_oob);
> > @@ -4607,7 +4615,9 @@ void monitor_cleanup(void)
> >       * we need to unregister from chardev below in
> >       * monitor_data_destroy(), and chardev is not thread-safe yet
> >       */
> > -    iothread_stop(mon_iothread);
> > +    if (mon_iothread) {
> > +        iothread_stop(mon_iothread);
> > +    }
> >
> 
> here the monitor_lock isn't taken, is there a reason worth a comment?
> 
> >      /* Flush output buffers and destroy monitors */
> >      qemu_mutex_lock(&monitor_lock);
> > @@ -4622,9 +4632,10 @@ void monitor_cleanup(void)
> >      /* QEMUBHs needs to be deleted before destroying the I/O thread */
> >      qemu_bh_delete(qmp_dispatcher_bh);
> >      qmp_dispatcher_bh = NULL;
> > -
> > -    iothread_destroy(mon_iothread);
> > -    mon_iothread = NULL;
> > +    if (mon_iothread) {
> > +        iothread_destroy(mon_iothread);
> > +        mon_iothread = NULL;
> > +    }
> >  }
> >
> >  QemuOptsList qemu_mon_opts = {
> > --
> > 2.11.0
> >
> >
> >
> 
> thanks
> 
> -- 
> Marc-André Lureau

-- 
Peter Xu