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.
