* Re: [Qemu-devel] [PATCH v2 0/2] delay monitor iothread creation
[not found] <20180928075832.18586-1-w.bumiller@proxmox.com>
@ 2018-10-11 0:09 ` Peter Xu
[not found] ` <20180928075832.18586-3-w.bumiller@proxmox.com>
1 sibling, 0 replies; 5+ messages in thread
From: Peter Xu @ 2018-10-11 0:09 UTC (permalink / raw)
To: Wolfgang Bumiller
Cc: qemu-devel, Markus Armbruster, Dr. David Alan Gilbert, Eric Blake
On Fri, Sep 28, 2018 at 09:58:30AM +0200, Wolfgang Bumiller wrote:
> The early monitor iothread creation conflicts with the -daemonize option
> causing crashes at shutdown of a daemonized qemu instance.
> These patches will delay the creation to when a monitor using it is
> actually spawned.
>
> While the second patch depends on the first one, the first is a
> consistency cleanup on its own, therefore split out.
>
> v2:
> This version incorporates Markus Armbruster's requested change to
> protect mon_iothread initialization by monitor_lock (and moves the
> variable declaration to reflect this), and adds a comments about
> monitor_init() expecting to be run in the main thread.
>
> Wolfgang Bumiller (2):
> monitor: guard iothread access by mon->use_io_thread
> monitor: delay monitor iothread creation
Hi, Wolfgang,
Do you have plan to repost this series?
Regards,
--
Peter Xu
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] monitor: delay monitor iothread creation
[not found] ` <20180928095543.GN9560@xz-x1>
@ 2018-10-11 6:30 ` Markus Armbruster
2018-10-11 8:23 ` Wolfgang Bumiller
2018-10-30 18:44 ` Markus Armbruster
0 siblings, 2 replies; 5+ messages in thread
From: Markus Armbruster @ 2018-10-11 6:30 UTC (permalink / raw)
To: Peter Xu
Cc: Marc-André Lureau, w.bumiller, QEMU, Dr. David Alan Gilbert,
Markus Armbruster
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] monitor: delay monitor iothread creation
2018-10-11 6:30 ` [Qemu-devel] [PATCH v2 2/2] monitor: " Markus Armbruster
@ 2018-10-11 8:23 ` Wolfgang Bumiller
2018-10-11 9:49 ` Markus Armbruster
2018-10-30 18:44 ` Markus Armbruster
1 sibling, 1 reply; 5+ messages in thread
From: Wolfgang Bumiller @ 2018-10-11 8:23 UTC (permalink / raw)
To: Markus Armbruster
Cc: Peter Xu, Marc-André Lureau, QEMU, Dr. David Alan Gilbert
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] monitor: delay monitor iothread creation
2018-10-11 8:23 ` Wolfgang Bumiller
@ 2018-10-11 9:49 ` Markus Armbruster
0 siblings, 0 replies; 5+ messages in thread
From: Markus Armbruster @ 2018-10-11 9:49 UTC (permalink / raw)
To: Wolfgang Bumiller
Cc: Marc-André Lureau, QEMU, Peter Xu, Dr. David Alan Gilbert
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH v2 2/2] monitor: delay monitor iothread creation
2018-10-11 6:30 ` [Qemu-devel] [PATCH v2 2/2] monitor: " Markus Armbruster
2018-10-11 8:23 ` Wolfgang Bumiller
@ 2018-10-30 18:44 ` Markus Armbruster
1 sibling, 0 replies; 5+ messages in thread
From: Markus Armbruster @ 2018-10-30 18:44 UTC (permalink / raw)
To: w.bumiller; +Cc: Peter Xu, Marc-André Lureau, QEMU, Dr. David Alan Gilbert
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!
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-10-30 18:44 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20180928075832.18586-1-w.bumiller@proxmox.com>
2018-10-11 0:09 ` [Qemu-devel] [PATCH v2 0/2] delay monitor iothread creation Peter Xu
[not found] ` <20180928075832.18586-3-w.bumiller@proxmox.com>
[not found] ` <CAJ+F1CL4S8aLm6ayu7GfKMfySTBTuUS3j2cuni-+ErEp5Y8VLQ@mail.gmail.com>
[not found] ` <20180928095543.GN9560@xz-x1>
2018-10-11 6:30 ` [Qemu-devel] [PATCH v2 2/2] monitor: " Markus Armbruster
2018-10-11 8:23 ` Wolfgang Bumiller
2018-10-11 9:49 ` Markus Armbruster
2018-10-30 18:44 ` Markus Armbruster
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.