| Message ID | 20180925081507.11873-3-w.bumiller@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | delay monitor iothread creation | expand |
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.bumiller@proxmox.com> > Fixes: d32749deb615 ("monitor: move init global earlier") Reviewed-by: Peter Xu <peterx@redhat.com> Tested-by: Peter Xu <peterx@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... Thanks,
> On September 25, 2018 at 12:31 PM Peter Xu <peterx@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.bumiller@proxmox.com> > > Fixes: d32749deb615 ("monitor: move init global earlier") > > Reviewed-by: Peter Xu <peterx@redhat.com> > Tested-by: Peter Xu <peterx@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(). 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.)
On Tue, Sep 25, 2018 at 01:09:57PM +0200, Wolfgang Bumiller wrote: > > > On September 25, 2018 at 12:31 PM Peter Xu <peterx@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.bumiller@proxmox.com> > > > Fixes: d32749deb615 ("monitor: move init global earlier") > > > > Reviewed-by: Peter Xu <peterx@redhat.com> > > Tested-by: Peter Xu <peterx@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. > > 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. Regards,
Peter Xu <peterx@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 <peterx@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.bumiller@proxmox.com> >> > > Fixes: d32749deb615 ("monitor: move init global earlier") >> > >> > Reviewed-by: Peter Xu <peterx@redhat.com> >> > Tested-by: Peter Xu <peterx@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.fsf@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?
On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@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 <peterx@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.bumiller@proxmox.com> > >> > > Fixes: d32749deb615 ("monitor: move init global earlier") > >> > > >> > Reviewed-by: Peter Xu <peterx@redhat.com> > >> > Tested-by: Peter Xu <peterx@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.fsf@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 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)? Regards,
Peter Xu <peterx@redhat.com> writes: > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@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 <peterx@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.bumiller@proxmox.com> >> >> > > Fixes: d32749deb615 ("monitor: move init global earlier") >> >> > >> >> > Reviewed-by: Peter Xu <peterx@redhat.com> >> >> > Tested-by: Peter Xu <peterx@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.fsf@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.
On Thu, Sep 27, 2018 at 02:35:07PM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: > >> Peter Xu <peterx@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 <peterx@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.bumiller@proxmox.com> > >> >> > > Fixes: d32749deb615 ("monitor: move init global earlier") > >> >> > > >> >> > Reviewed-by: Peter Xu <peterx@redhat.com> > >> >> > Tested-by: Peter Xu <peterx@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.fsf@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,
On Fri, Sep 28, 2018 at 11:18:36AM +0800, Peter Xu wrote: > On Thu, Sep 27, 2018 at 02:35:07PM +0200, Markus Armbruster wrote: > > Peter Xu <peterx@redhat.com> writes: > > > > > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: > > >> Peter Xu <peterx@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 <peterx@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.bumiller@proxmox.com> > > >> >> > > Fixes: d32749deb615 ("monitor: move init global earlier") > > >> >> > > > >> >> > Reviewed-by: Peter Xu <peterx@redhat.com> > > >> >> > Tested-by: Peter Xu <peterx@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.fsf@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, Sounds good to me, sending a v2 with the changes.
diff --git a/monitor.c b/monitor.c index e5462567e4..53dc1dddc6 100644 --- a/monitor.c +++ b/monitor.c @@ -707,9 +707,14 @@ static void monitor_qapi_event_init(void) static void handle_hmp_command(Monitor *mon, const char *cmdline); +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(); + } memset(mon, 0, sizeof(Monitor)); qemu_mutex_init(&mon->mon_lock); qemu_mutex_init(&mon->qmp.qmp_queue_lock); @@ -4447,15 +4452,6 @@ static AioContext *monitor_get_aio_context(void) static void monitor_iothread_init(void) { mon_iothread = iothread_create("mon_iothread", &error_abort); - - /* - * The dispatcher BH must run in the main loop thread, since we - * have commands assuming that context. It would be nice to get - * rid of those assumptions. - */ - qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), - monitor_qmp_bh_dispatcher, - NULL); } void monitor_init_globals(void) @@ -4465,7 +4461,15 @@ void monitor_init_globals(void) sortcmdlist(); qemu_mutex_init(&monitor_lock); qemu_mutex_init(&mon_fdsets_lock); - monitor_iothread_init(); + + /* + * The dispatcher BH must run in the main loop thread, since we + * have commands assuming that context. It would be nice to get + * rid of those assumptions. + */ + qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(), + monitor_qmp_bh_dispatcher, + NULL); } /* These functions just adapt the readline interface in a typesafe way. We @@ -4600,7 +4604,9 @@ void monitor_cleanup(void) * we need to unregister from chardev below in * monitor_data_destroy(), and chardev is not thread-safe yet */ - iothread_stop(mon_iothread); + if (mon_iothread) { + iothread_stop(mon_iothread); + } /* Flush output buffers and destroy monitors */ qemu_mutex_lock(&monitor_lock); @@ -4615,9 +4621,10 @@ void monitor_cleanup(void) /* QEMUBHs needs to be deleted before destroying the I/O thread */ qemu_bh_delete(qmp_dispatcher_bh); qmp_dispatcher_bh = NULL; - - iothread_destroy(mon_iothread); - mon_iothread = NULL; + if (mon_iothread) { + iothread_destroy(mon_iothread); + mon_iothread = NULL; + } } QemuOptsList qemu_mon_opts = {
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.bumiller@proxmox.com> Fixes: d32749deb615 ("monitor: move init global earlier") --- Resolved conflict from the removal of qmp_respond_bh. No functional changes otherwise. monitor.c | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-)