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

[Markus Armbruster]

Peter Xu <address@hidden> writes:

> On Tue, Sep 25, 2018 at 01:09:57PM +0200, Wolfgang Bumiller wrote:
>> 
>> > On September 25, 2018 at 12:31 PM Peter Xu <address@hidden> wrote:
>> > 
>> > 
>> > On Tue, Sep 25, 2018 at 10:15:07AM +0200, Wolfgang Bumiller 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.
>> > > Instantiation of monitors happens only after os_daemonize().
>> > > We still need to create the qmp_dispatcher_bh when not using
>> > > iothreads, so this now still happens in
>> > > monitor_init_globals().
>> > > 
>> > > Signed-off-by: Wolfgang Bumiller <address@hidden>
>> > > Fixes: d32749deb615 ("monitor: move init global earlier")
>> > 
>> > Reviewed-by: Peter Xu <address@hidden>
>> > Tested-by: Peter Xu <address@hidden>
>> > 
>> > Though note that after this patch monitor_data_init() is not thread
>> > safe any more (while it was), so we may need to be careful...
>> 
>> Is there a way to create monitors concurrently? If so, we could
>> add a mutex initialized in monitor_globals_init().
>
> qmp_human_monitor_command() creates monitors dynamically, though not
> concurrently since currently it's only possible to happen on the main
> thread (and it's always setting use_io_thread to false now).  So we
> should be safe now.

Until recently, monitor code ran entirely in the main loop.
Undesirable, because it lets monitors hog the main loop.

Moving stuff out of the main loop is non-trivial, because it may break
unstated assumptions.

Peter's OOB work moved the monitor core from the main loop into
@mon_iothread.

Moving commands is harder: you have to audit each command for
assumptions that no longer hold.  A common one is of course "thread
safety is not an issue".  Peter's OOB work provides for OOB command
execution in @mon_iothread.

As long as monitors get created dynamically only in monitor commands,
the lack of synchronization around the creation of @mon_iothread is an
instance of "monitor commands assume they're running in the main loop".

>> Another way would be to only defer to after os_daemonize() but still
>> stick to spawning the thread unconditionally. (Iow. call
>> monitor_iothread_init() after os_daemonize() from vl.c.)
>
> Yeah I think that should work too (and seems good).  I'll see how
> Markus think.

My first thought was:

    diff --git a/monitor.c b/monitor.c
    index 44d068b106..50f7d1230f 100644
    --- a/monitor.c
    +++ b/monitor.c
    @@ -712,9 +712,7 @@ static void monitor_iothread_init(void);
     static void monitor_data_init(Monitor *mon, bool skip_flush,
                                   bool use_io_thread)
     {
    -    if (use_io_thread && !mon_iothread) {
    -        monitor_iothread_init();
    -    }
    +    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);
    @@ -4555,6 +4553,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();
    +        }
         }

         monitor_data_init(mon, false, use_oob);

This limits monitor_data_init() to initialization of *mon.  I like that.

It also makes it obvious that qmp_human_monitor_command() can't mess
with @mon_iothread.  Sadly, that doesn't really buy us anything, since
the other callers of monitor_init() can still mess with it.  These are:

* gdbserver_start()

  CLI option -gdb, HMP command gdbserver, linux user CLI option -g and
  environment variable QEMU_GDB

* mon_init_func()

  CLI option -mon and its convenience buddies -monitor, -qmp,
  -qmp-pretty

* qemu_chr_new_noreplay()

  gdbserver_start() again, and qemu_chr_new(), which is called all over
  the place.

These should all run in the main loop (anything else would be a bug).
They (more or less) obviously do, except for qemu_chr_new(), where we
aren't sure.

Please see
Subject: Re: [PATCH] monitor: avoid potential dead-lock when cleaning up
Message-ID: <address@hidden>
https://lists.gnu.org/archive/html/qemu-devel/2018-08/msg03821.html

The conclusion reached there was "I'm afraid we need to rethink the set
of locks protecting shared monitor state" and "probably change a bit
monitor/chardev creation to be under tighter control..."

Should we put @mon_iothread under @monitor_lock?

Could we accept this patch without doing that, on the theory that it
doesn't make things worse than they already are?