On Thu, Sep 27, 2018 at 02:35:07PM +0200, Markus Armbruster wrote: > Peter Xu <pet...@redhat.com> writes: > > > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: > >> 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.) > > > > [1] > > > >> > > >> > 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..." > > > > Yeah that would be nice... > > > >> > >> Should we put @mon_iothread under @monitor_lock? > > > > IMHO we can when we create the thread. I guess we don't need that > > lock when reading @mon_iothread, after all it's a very special > > variable in that: > > > > - it is only set once, or never > > > > - when reading @mon_iothread only, we must have it set or it should > > be a programming error, so it's more like an assert(mon_iothread) > > not a contention > > > >> > >> Could we accept this patch without doing that, on the theory that it > >> doesn't make things worse than they already are? > > > > If this bothers us that much, how about we just choose the option that > > Wolfgang offered at [1] above to create the iothread after daemonize > > (so we pick that out from monitor_global_init)? > > I'd prefer this patch's approach, because it keeps the interface > simpler. > > I can accept this patch as is, or with my incremental patch squashed > in. A comment explaining monitor_init() expects to run in the main > thread would be nice. > > I'd also accept a patch that wraps > > if (!mon_iothread) { > monitor_iothread_init(); > } > > in a critical section. Using @monitor_lock is fine. A new lock feels > unnecessarily fine-grained. If using @monitor_lock, move the definition > of @mon_iothread next to @monitor_lock, and update the comment there.
Looks ok at least to me! Thanks, -- Peter Xu