| Message ID | 20180928075832.18586-3-w.bumiller@proxmox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | delay monitor iothread creation | expand |
Hi On Fri, Sep 28, 2018 at 12:02 PM Wolfgang Bumiller <w.bumiller@proxmox.com> 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. Therefore > mon_iothread initialization is protected by monitor_lock. > > We still need to create the qmp_dispatcher_bh when not using > iothreads, so this now still happens via > monitor_init_globals(). > > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> > Fixes: d32749deb615 ("monitor: move init global earlier") > --- > Changes to v1: > - move mon_iothread declaration down to monitor_lock's declaration > (updating monitor_lock's coverage comment) > - in monitor_data_init() assert() that mon_iothread is not NULL or > not used instead of initializing it there, as its usage pattern is > so that it is a initialized once before being used, or never used > at all. > - in monitor_iothread_init(), protect mon_iothread initialization > with monitor_lock > - in monitor_init(): run monitor_ithread_init() in the `use_oob` > branch. > Note that I currently also test for mon_iothread being NULL there, > which we could leave this out as spawning new monitors isn't > something that happens a lot, but I like the idea of avoiding > taking a lock when not required. > Otherwise, I can send a v3 with this removed. > > monitor.c | 49 ++++++++++++++++++++++++++++++------------------- > 1 file changed, 30 insertions(+), 19 deletions(-) > > diff --git a/monitor.c b/monitor.c > index d47e4259fd..870584a548 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -239,9 +239,6 @@ struct Monitor { > int mux_out; > }; > > -/* Shared monitor I/O thread */ > -IOThread *mon_iothread; > - > /* Bottom half to dispatch the requests received from I/O thread */ > QEMUBH *qmp_dispatcher_bh; > > @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest; > /* QMP checker flags */ > #define QMP_ACCEPT_UNKNOWNS 1 > > -/* Protects mon_list, monitor_qapi_event_state. */ > +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */ > static QemuMutex monitor_lock; > static GHashTable *monitor_qapi_event_state; > static QTAILQ_HEAD(mon_list, Monitor) mon_list; > +IOThread *mon_iothread; /* Shared monitor I/O thread */ > > /* Protects mon_fdsets */ > static QemuMutex mon_fdsets_lock; > @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline); > static void monitor_data_init(Monitor *mon, bool skip_flush, > bool use_io_thread) > { > + 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); > @@ -4453,16 +4452,11 @@ 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); > + qemu_mutex_lock(&monitor_lock); > + if (!mon_iothread) { > + mon_iothread = iothread_create("mon_iothread", &error_abort); > + } > + qemu_mutex_unlock(&monitor_lock); > } > > void monitor_init_globals(void) > @@ -4472,7 +4466,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 > @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) > monitor_list_append(mon); > } > > +/* > + * This expects to be run in the main thread. > + */ I read that Markus suggested that comment, but I don't really get why. It means that callers (chardev new) should also be restricted to main thread. > void monitor_init(Chardev *chr, int flags) > { > Monitor *mon = g_malloc(sizeof(*mon)); > @@ -4551,6 +4556,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(); > + } I would call it unconditonnally, to avoid TOCTOU. > } > > monitor_data_init(mon, false, use_oob); > @@ -4607,7 +4615,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); > + } > here the monitor_lock isn't taken, is there a reason worth a comment? > /* Flush output buffers and destroy monitors */ > qemu_mutex_lock(&monitor_lock); > @@ -4622,9 +4632,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 = { > -- > 2.11.0 > > > thanks
On Fri, Sep 28, 2018 at 01:00:26PM +0400, Marc-André Lureau wrote: > Hi > > On Fri, Sep 28, 2018 at 12:02 PM Wolfgang Bumiller > <w.bumiller@proxmox.com> 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. Therefore > > mon_iothread initialization is protected by monitor_lock. > > > > We still need to create the qmp_dispatcher_bh when not using > > iothreads, so this now still happens via > > monitor_init_globals(). > > > > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> > > Fixes: d32749deb615 ("monitor: move init global earlier") > > --- > > Changes to v1: > > - move mon_iothread declaration down to monitor_lock's declaration > > (updating monitor_lock's coverage comment) > > - in monitor_data_init() assert() that mon_iothread is not NULL or > > not used instead of initializing it there, as its usage pattern is > > so that it is a initialized once before being used, or never used > > at all. > > - in monitor_iothread_init(), protect mon_iothread initialization > > with monitor_lock > > - in monitor_init(): run monitor_ithread_init() in the `use_oob` > > branch. > > Note that I currently also test for mon_iothread being NULL there, > > which we could leave this out as spawning new monitors isn't > > something that happens a lot, but I like the idea of avoiding > > taking a lock when not required. > > Otherwise, I can send a v3 with this removed. > > > > monitor.c | 49 ++++++++++++++++++++++++++++++------------------- > > 1 file changed, 30 insertions(+), 19 deletions(-) > > > > diff --git a/monitor.c b/monitor.c > > index d47e4259fd..870584a548 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -239,9 +239,6 @@ struct Monitor { > > int mux_out; > > }; > > > > -/* Shared monitor I/O thread */ > > -IOThread *mon_iothread; > > - > > /* Bottom half to dispatch the requests received from I/O thread */ > > QEMUBH *qmp_dispatcher_bh; > > > > @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest; > > /* QMP checker flags */ > > #define QMP_ACCEPT_UNKNOWNS 1 > > > > -/* Protects mon_list, monitor_qapi_event_state. */ > > +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */ > > static QemuMutex monitor_lock; > > static GHashTable *monitor_qapi_event_state; > > static QTAILQ_HEAD(mon_list, Monitor) mon_list; > > +IOThread *mon_iothread; /* Shared monitor I/O thread */ > > > > /* Protects mon_fdsets */ > > static QemuMutex mon_fdsets_lock; > > @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline); > > static void monitor_data_init(Monitor *mon, bool skip_flush, > > bool use_io_thread) > > { > > + 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); > > @@ -4453,16 +4452,11 @@ 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); > > + qemu_mutex_lock(&monitor_lock); > > + if (!mon_iothread) { > > + mon_iothread = iothread_create("mon_iothread", &error_abort); > > + } > > + qemu_mutex_unlock(&monitor_lock); > > } > > > > void monitor_init_globals(void) > > @@ -4472,7 +4466,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 > > @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) > > monitor_list_append(mon); > > } > > > > +/* > > + * This expects to be run in the main thread. > > + */ > > I read that Markus suggested that comment, but I don't really get why. > > It means that callers (chardev new) should also be restricted to main thread. My understanding is that Markus mentioned about uncertainty on the chardev creation paths. Though AFAIU if we're with the lock then we don't need this comment at all, do we? > > > void monitor_init(Chardev *chr, int flags) > > { > > Monitor *mon = g_malloc(sizeof(*mon)); > > @@ -4551,6 +4556,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(); > > + } > > I would call it unconditonnally, to avoid TOCTOU. Yeh agree that the "if" could be omitted; though there shouldn't be toctou since the function will check it again. Thanks, > > > } > > > > monitor_data_init(mon, false, use_oob); > > @@ -4607,7 +4615,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); > > + } > > > > here the monitor_lock isn't taken, is there a reason worth a comment? > > > /* Flush output buffers and destroy monitors */ > > qemu_mutex_lock(&monitor_lock); > > @@ -4622,9 +4632,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 = { > > -- > > 2.11.0 > > > > > > > > thanks > > -- > Marc-André Lureau
Peter Xu <peterx@redhat.com> writes: > On Fri, Sep 28, 2018 at 01:00:26PM +0400, Marc-André Lureau wrote: >> Hi >> >> On Fri, Sep 28, 2018 at 12:02 PM Wolfgang Bumiller >> <w.bumiller@proxmox.com> 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. Therefore >> > mon_iothread initialization is protected by monitor_lock. >> > >> > We still need to create the qmp_dispatcher_bh when not using >> > iothreads, so this now still happens via >> > monitor_init_globals(). >> > >> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> >> > Fixes: d32749deb615 ("monitor: move init global earlier") >> > --- >> > Changes to v1: >> > - move mon_iothread declaration down to monitor_lock's declaration >> > (updating monitor_lock's coverage comment) >> > - in monitor_data_init() assert() that mon_iothread is not NULL or >> > not used instead of initializing it there, as its usage pattern is >> > so that it is a initialized once before being used, or never used >> > at all. >> > - in monitor_iothread_init(), protect mon_iothread initialization >> > with monitor_lock >> > - in monitor_init(): run monitor_ithread_init() in the `use_oob` >> > branch. >> > Note that I currently also test for mon_iothread being NULL there, >> > which we could leave this out as spawning new monitors isn't >> > something that happens a lot, but I like the idea of avoiding >> > taking a lock when not required. >> > Otherwise, I can send a v3 with this removed. >> > >> > monitor.c | 49 ++++++++++++++++++++++++++++++------------------- >> > 1 file changed, 30 insertions(+), 19 deletions(-) >> > >> > diff --git a/monitor.c b/monitor.c >> > index d47e4259fd..870584a548 100644 >> > --- a/monitor.c >> > +++ b/monitor.c >> > @@ -239,9 +239,6 @@ struct Monitor { >> > int mux_out; >> > }; >> > >> > -/* Shared monitor I/O thread */ >> > -IOThread *mon_iothread; >> > - >> > /* Bottom half to dispatch the requests received from I/O thread */ >> > QEMUBH *qmp_dispatcher_bh; >> > >> > @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest; >> > /* QMP checker flags */ >> > #define QMP_ACCEPT_UNKNOWNS 1 >> > >> > -/* Protects mon_list, monitor_qapi_event_state. */ >> > +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */ >> > static QemuMutex monitor_lock; >> > static GHashTable *monitor_qapi_event_state; >> > static QTAILQ_HEAD(mon_list, Monitor) mon_list; >> > +IOThread *mon_iothread; /* Shared monitor I/O thread */ >> > >> > /* Protects mon_fdsets */ >> > static QemuMutex mon_fdsets_lock; >> > @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline); >> > static void monitor_data_init(Monitor *mon, bool skip_flush, >> > bool use_io_thread) >> > { >> > + 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); >> > @@ -4453,16 +4452,11 @@ 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); >> > + qemu_mutex_lock(&monitor_lock); >> > + if (!mon_iothread) { >> > + mon_iothread = iothread_create("mon_iothread", &error_abort); >> > + } >> > + qemu_mutex_unlock(&monitor_lock); >> > } >> > >> > void monitor_init_globals(void) >> > @@ -4472,7 +4466,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 >> > @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) >> > monitor_list_append(mon); >> > } >> > >> > +/* >> > + * This expects to be run in the main thread. >> > + */ >> >> I read that Markus suggested that comment, but I don't really get why. >> >> It means that callers (chardev new) should also be restricted to main thread. > > My understanding is that Markus mentioned about uncertainty on the > chardev creation paths. Though AFAIU if we're with the lock then we > don't need this comment at all, do we? The conversation (Message-ID: <87d0sz86f8.fsf@dusky.pond.sub.org>) was: Peter Xu <peterx@redhat.com> writes: > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: [...] >> 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. v2 uses this approach. 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. Acceptable alternative 1, with a few optional variations. The comment makes sense because if monitor_init can run in other threads, the creation of @iothread is racy. Acceptable since it's really no worse than before (see the full message for why). 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. Acceptable alternative 2. v2 appears to combine both alternatives. Not what I asked for. I figure the comment still makes sense, since @iothread creation is just one of the issues, and protecting it with a lock leaves the other issues unaddressed. If we actually run it in other threads, the comment needs to be augmented with a suitable FIXME stating the problem. Marc-André, does this make sense? >> >> > void monitor_init(Chardev *chr, int flags) >> > { >> > Monitor *mon = g_malloc(sizeof(*mon)); >> > @@ -4551,6 +4556,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(); >> > + } >> >> I would call it unconditonnally, to avoid TOCTOU. > > Yeh agree that the "if" could be omitted; though there shouldn't be > toctou since the function will check it again. Really? [...] >> > } >> > >> > monitor_data_init(mon, false, use_oob); >> > @@ -4607,7 +4615,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); >> > + } >> > >> >> here the monitor_lock isn't taken, is there a reason worth a comment? I don't know. What I know is that locking something only some of the times (not counting a single-threaded initial stretch of initialization code) is usually wrong. >> > /* Flush output buffers and destroy monitors */ >> > qemu_mutex_lock(&monitor_lock); [...] Since the bug is inconveniencing people, should I merge v1 for now? We can then figure out how to improve on it.
On Thu, Oct 11, 2018 at 08:30:24AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Fri, Sep 28, 2018 at 01:00:26PM +0400, Marc-André Lureau wrote: > >> Hi > >> > >> On Fri, Sep 28, 2018 at 12:02 PM Wolfgang Bumiller > >> <w.bumiller@proxmox.com> 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. Therefore > >> > mon_iothread initialization is protected by monitor_lock. > >> > > >> > We still need to create the qmp_dispatcher_bh when not using > >> > iothreads, so this now still happens via > >> > monitor_init_globals(). > >> > > >> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> > >> > Fixes: d32749deb615 ("monitor: move init global earlier") > >> > --- > >> > Changes to v1: > >> > - move mon_iothread declaration down to monitor_lock's declaration > >> > (updating monitor_lock's coverage comment) > >> > - in monitor_data_init() assert() that mon_iothread is not NULL or > >> > not used instead of initializing it there, as its usage pattern is > >> > so that it is a initialized once before being used, or never used > >> > at all. > >> > - in monitor_iothread_init(), protect mon_iothread initialization > >> > with monitor_lock > >> > - in monitor_init(): run monitor_ithread_init() in the `use_oob` > >> > branch. > >> > Note that I currently also test for mon_iothread being NULL there, > >> > which we could leave this out as spawning new monitors isn't > >> > something that happens a lot, but I like the idea of avoiding > >> > taking a lock when not required. > >> > Otherwise, I can send a v3 with this removed. > >> > > >> > monitor.c | 49 ++++++++++++++++++++++++++++++------------------- > >> > 1 file changed, 30 insertions(+), 19 deletions(-) > >> > > >> > diff --git a/monitor.c b/monitor.c > >> > index d47e4259fd..870584a548 100644 > >> > --- a/monitor.c > >> > +++ b/monitor.c > >> > @@ -239,9 +239,6 @@ struct Monitor { > >> > int mux_out; > >> > }; > >> > > >> > -/* Shared monitor I/O thread */ > >> > -IOThread *mon_iothread; > >> > - > >> > /* Bottom half to dispatch the requests received from I/O thread */ > >> > QEMUBH *qmp_dispatcher_bh; > >> > > >> > @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest; > >> > /* QMP checker flags */ > >> > #define QMP_ACCEPT_UNKNOWNS 1 > >> > > >> > -/* Protects mon_list, monitor_qapi_event_state. */ > >> > +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */ > >> > static QemuMutex monitor_lock; > >> > static GHashTable *monitor_qapi_event_state; > >> > static QTAILQ_HEAD(mon_list, Monitor) mon_list; > >> > +IOThread *mon_iothread; /* Shared monitor I/O thread */ > >> > > >> > /* Protects mon_fdsets */ > >> > static QemuMutex mon_fdsets_lock; > >> > @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline); > >> > static void monitor_data_init(Monitor *mon, bool skip_flush, > >> > bool use_io_thread) > >> > { > >> > + 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); > >> > @@ -4453,16 +4452,11 @@ 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); > >> > + qemu_mutex_lock(&monitor_lock); > >> > + if (!mon_iothread) { > >> > + mon_iothread = iothread_create("mon_iothread", &error_abort); > >> > + } > >> > + qemu_mutex_unlock(&monitor_lock); > >> > } > >> > > >> > void monitor_init_globals(void) > >> > @@ -4472,7 +4466,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 > >> > @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) > >> > monitor_list_append(mon); > >> > } > >> > > >> > +/* > >> > + * This expects to be run in the main thread. > >> > + */ > >> > >> I read that Markus suggested that comment, but I don't really get why. > >> > >> It means that callers (chardev new) should also be restricted to main thread. > > > > My understanding is that Markus mentioned about uncertainty on the > > chardev creation paths. Though AFAIU if we're with the lock then we > > don't need this comment at all, do we? > > The conversation (Message-ID: <87d0sz86f8.fsf@dusky.pond.sub.org>) was: > > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: > [...] > >> 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. > > v2 uses this approach. > > 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. > > Acceptable alternative 1, with a few optional variations. > > The comment makes sense because if monitor_init can run in other > threads, the creation of @iothread is racy. Acceptable since it's > really no worse than before (see the full message for why). > > 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. > > Acceptable alternative 2. > > v2 appears to combine both alternatives. Not what I asked for. I > figure the comment still makes sense, since @iothread creation is just > one of the issues, and protecting it with a lock leaves the other issues > unaddressed. > > If we actually run it in other threads, the comment needs to be > augmented with a suitable FIXME stating the problem. > > Marc-André, does this make sense? > > >> > >> > void monitor_init(Chardev *chr, int flags) > >> > { > >> > Monitor *mon = g_malloc(sizeof(*mon)); > >> > @@ -4551,6 +4556,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(); > >> > + } > >> > >> I would call it unconditonnally, to avoid TOCTOU. > > > > Yeh agree that the "if" could be omitted; though there shouldn't be > > toctou since the function will check it again. > > Really? Yes, it's a cheap check followed by a lock followed by another check. Too much since the code is only hit on user interaction anyway, so I probably shouldn't have kept that. monitor_iothread_init() does: lock(); if (!mon_iothread) mon_iothread = ... unlock(); With mon_iothread only ever being written to once, either the caller sees the correct value, or enters a locked section to verify. > > [...] > >> > } > >> > > >> > monitor_data_init(mon, false, use_oob); > >> > @@ -4607,7 +4615,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); > >> > + } > >> > > >> > >> here the monitor_lock isn't taken, is there a reason worth a comment? > > I don't know. What I know is that locking something only some of the > times (not counting a single-threaded initial stretch of initialization > code) is usually wrong. monitor_cleanup() runs at the end of vl.c's main(), so the main loop responsible for most of the competing In the end, given that monitor_lock never gets destroyed, locking shouldn't hurt either. > > >> > /* Flush output buffers and destroy monitors */ > >> > qemu_mutex_lock(&monitor_lock); > [...] > > Since the bug is inconveniencing people, should I merge v1 for now? We > can then figure out how to improve on it. Tbh I'm unsure which way to proceed at this point.
Wolfgang Bumiller <w.bumiller@proxmox.com> writes: > On Thu, Oct 11, 2018 at 08:30:24AM +0200, Markus Armbruster wrote: [...] >> Since the bug is inconveniencing people, should I merge v1 for now? We >> can then figure out how to improve on it. > > Tbh I'm unsure which way to proceed at this point. Don't worry, ball's in my court for now.
Markus Armbruster <armbru@redhat.com> writes: > Peter Xu <peterx@redhat.com> writes: > >> On Fri, Sep 28, 2018 at 01:00:26PM +0400, Marc-André Lureau wrote: >>> Hi >>> >>> On Fri, Sep 28, 2018 at 12:02 PM Wolfgang Bumiller >>> <w.bumiller@proxmox.com> 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. Therefore >>> > mon_iothread initialization is protected by monitor_lock. >>> > >>> > We still need to create the qmp_dispatcher_bh when not using >>> > iothreads, so this now still happens via >>> > monitor_init_globals(). >>> > >>> > Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> >>> > Fixes: d32749deb615 ("monitor: move init global earlier") >>> > --- >>> > Changes to v1: >>> > - move mon_iothread declaration down to monitor_lock's declaration >>> > (updating monitor_lock's coverage comment) >>> > - in monitor_data_init() assert() that mon_iothread is not NULL or >>> > not used instead of initializing it there, as its usage pattern is >>> > so that it is a initialized once before being used, or never used >>> > at all. >>> > - in monitor_iothread_init(), protect mon_iothread initialization >>> > with monitor_lock >>> > - in monitor_init(): run monitor_ithread_init() in the `use_oob` >>> > branch. >>> > Note that I currently also test for mon_iothread being NULL there, >>> > which we could leave this out as spawning new monitors isn't >>> > something that happens a lot, but I like the idea of avoiding >>> > taking a lock when not required. >>> > Otherwise, I can send a v3 with this removed. >>> > >>> > monitor.c | 49 ++++++++++++++++++++++++++++++------------------- >>> > 1 file changed, 30 insertions(+), 19 deletions(-) >>> > >>> > diff --git a/monitor.c b/monitor.c >>> > index d47e4259fd..870584a548 100644 >>> > --- a/monitor.c >>> > +++ b/monitor.c >>> > @@ -239,9 +239,6 @@ struct Monitor { >>> > int mux_out; >>> > }; >>> > >>> > -/* Shared monitor I/O thread */ >>> > -IOThread *mon_iothread; >>> > - >>> > /* Bottom half to dispatch the requests received from I/O thread */ >>> > QEMUBH *qmp_dispatcher_bh; >>> > >>> > @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest; >>> > /* QMP checker flags */ >>> > #define QMP_ACCEPT_UNKNOWNS 1 >>> > >>> > -/* Protects mon_list, monitor_qapi_event_state. */ >>> > +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */ >>> > static QemuMutex monitor_lock; >>> > static GHashTable *monitor_qapi_event_state; >>> > static QTAILQ_HEAD(mon_list, Monitor) mon_list; >>> > +IOThread *mon_iothread; /* Shared monitor I/O thread */ >>> > >>> > /* Protects mon_fdsets */ >>> > static QemuMutex mon_fdsets_lock; >>> > @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline); >>> > static void monitor_data_init(Monitor *mon, bool skip_flush, >>> > bool use_io_thread) >>> > { >>> > + 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); >>> > @@ -4453,16 +4452,11 @@ 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); >>> > + qemu_mutex_lock(&monitor_lock); >>> > + if (!mon_iothread) { >>> > + mon_iothread = iothread_create("mon_iothread", &error_abort); >>> > + } >>> > + qemu_mutex_unlock(&monitor_lock); >>> > } >>> > >>> > void monitor_init_globals(void) >>> > @@ -4472,7 +4466,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 >>> > @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) >>> > monitor_list_append(mon); >>> > } >>> > >>> > +/* >>> > + * This expects to be run in the main thread. >>> > + */ >>> >>> I read that Markus suggested that comment, but I don't really get why. >>> >>> It means that callers (chardev new) should also be restricted to main thread. >> >> My understanding is that Markus mentioned about uncertainty on the >> chardev creation paths. Though AFAIU if we're with the lock then we >> don't need this comment at all, do we? > > The conversation (Message-ID: <87d0sz86f8.fsf@dusky.pond.sub.org>) was: > > Peter Xu <peterx@redhat.com> writes: > > > On Thu, Sep 27, 2018 at 10:46:34AM +0200, Markus Armbruster wrote: > [...] > >> 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. > > v2 uses this approach. > > 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. > > Acceptable alternative 1, with a few optional variations. > > The comment makes sense because if monitor_init can run in other > threads, the creation of @iothread is racy. Acceptable since it's > really no worse than before (see the full message for why). > > 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. > > Acceptable alternative 2. > > v2 appears to combine both alternatives. Not what I asked for. I > figure the comment still makes sense, since @iothread creation is just > one of the issues, and protecting it with a lock leaves the other issues > unaddressed. > > If we actually run it in other threads, the comment needs to be > augmented with a suitable FIXME stating the problem. > > Marc-André, does this make sense? > >>> >>> > void monitor_init(Chardev *chr, int flags) >>> > { >>> > Monitor *mon = g_malloc(sizeof(*mon)); >>> > @@ -4551,6 +4556,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(); >>> > + } >>> >>> I would call it unconditonnally, to avoid TOCTOU. >> >> Yeh agree that the "if" could be omitted; though there shouldn't be >> toctou since the function will check it again. > > Really? > > [...] >>> > } >>> > >>> > monitor_data_init(mon, false, use_oob); >>> > @@ -4607,7 +4615,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); >>> > + } >>> > >>> >>> here the monitor_lock isn't taken, is there a reason worth a comment? > > I don't know. What I know is that locking something only some of the > times (not counting a single-threaded initial stretch of initialization > code) is usually wrong. > >>> > /* Flush output buffers and destroy monitors */ >>> > qemu_mutex_lock(&monitor_lock); > [...] > > Since the bug is inconveniencing people, should I merge v1 for now? We > can then figure out how to improve on it. I'm queuing v1, and will post a follow-up patch to address its minor shortcomings. Thanks, and sorry for the delay!
diff --git a/monitor.c b/monitor.c index d47e4259fd..870584a548 100644 --- a/monitor.c +++ b/monitor.c @@ -239,9 +239,6 @@ struct Monitor { int mux_out; }; -/* Shared monitor I/O thread */ -IOThread *mon_iothread; - /* Bottom half to dispatch the requests received from I/O thread */ QEMUBH *qmp_dispatcher_bh; @@ -262,10 +259,11 @@ typedef struct QMPRequest QMPRequest; /* QMP checker flags */ #define QMP_ACCEPT_UNKNOWNS 1 -/* Protects mon_list, monitor_qapi_event_state. */ +/* Protects mon_list, monitor_qapi_event_state and mon_iothread. */ static QemuMutex monitor_lock; static GHashTable *monitor_qapi_event_state; static QTAILQ_HEAD(mon_list, Monitor) mon_list; +IOThread *mon_iothread; /* Shared monitor I/O thread */ /* Protects mon_fdsets */ static QemuMutex mon_fdsets_lock; @@ -710,6 +708,7 @@ static void handle_hmp_command(Monitor *mon, const char *cmdline); static void monitor_data_init(Monitor *mon, bool skip_flush, bool use_io_thread) { + 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); @@ -4453,16 +4452,11 @@ 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); + qemu_mutex_lock(&monitor_lock); + if (!mon_iothread) { + mon_iothread = iothread_create("mon_iothread", &error_abort); + } + qemu_mutex_unlock(&monitor_lock); } void monitor_init_globals(void) @@ -4472,7 +4466,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 @@ -4535,6 +4537,9 @@ static void monitor_qmp_setup_handlers_bh(void *opaque) monitor_list_append(mon); } +/* + * This expects to be run in the main thread. + */ void monitor_init(Chardev *chr, int flags) { Monitor *mon = g_malloc(sizeof(*mon)); @@ -4551,6 +4556,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); @@ -4607,7 +4615,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); @@ -4622,9 +4632,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. Therefore mon_iothread initialization is protected by monitor_lock. We still need to create the qmp_dispatcher_bh when not using iothreads, so this now still happens via monitor_init_globals(). Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com> Fixes: d32749deb615 ("monitor: move init global earlier") --- Changes to v1: - move mon_iothread declaration down to monitor_lock's declaration (updating monitor_lock's coverage comment) - in monitor_data_init() assert() that mon_iothread is not NULL or not used instead of initializing it there, as its usage pattern is so that it is a initialized once before being used, or never used at all. - in monitor_iothread_init(), protect mon_iothread initialization with monitor_lock - in monitor_init(): run monitor_ithread_init() in the `use_oob` branch. Note that I currently also test for mon_iothread being NULL there, which we could leave this out as spawning new monitors isn't something that happens a lot, but I like the idea of avoiding taking a lock when not required. Otherwise, I can send a v3 with this removed. monitor.c | 49 ++++++++++++++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 19 deletions(-)