Peter Xu <pet...@redhat.com> writes: > On Tue, Sep 25, 2018 at 01:09:57PM +0200, Wolfgang Bumiller wrote: >> >> > On September 25, 2018 at 12:31 PM Peter Xu <pet...@redhat.com> 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 <w.bumil...@proxmox.com> >> > > Fixes: d32749deb615 ("monitor: move init global earlier") >> > >> > Reviewed-by: Peter Xu <pet...@redhat.com> >> > Tested-by: Peter Xu <pet...@redhat.com> >> > >> > 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: <87a7phtti5....@dusky.pond.sub.org> 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?