[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread cre
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation |
Date: |
Fri, 28 Sep 2018 11:18:36 +0800 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Thu, Sep 27, 2018 at 02:35:07PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
>
> > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote:
> >> 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.)
> >
> > [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: <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..."
> >
> > 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
- [Qemu-devel] [PATCH rebased 0/2] delay monitor iothread creation, Wolfgang Bumiller, 2018/09/25
- [Qemu-devel] [PATCH rebased 1/2] monitor: guard iothread access by mon->use_io_thread, Wolfgang Bumiller, 2018/09/25
- [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Wolfgang Bumiller, 2018/09/25
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Peter Xu, 2018/09/25
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Wolfgang Bumiller, 2018/09/25
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Peter Xu, 2018/09/25
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Markus Armbruster, 2018/09/27
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Peter Xu, 2018/09/27
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Markus Armbruster, 2018/09/27
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation,
Peter Xu <=
- Re: [Qemu-devel] [PATCH rebased 2/2] monitor: delay monitor iothread creation, Wolfgang Bumiller, 2018/09/28