From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-block@nongnu.org,
"Philippe Mathieu-Daudé" <philmd@redhat.com>,
qemu-devel@nongnu.org, "David Alan Gilbert" <dgilbert@redhat.com>,
marcandre.lureau@gmail.com, stefanha@redhat.com,
"Alex Bennée" <alex.bennee@linaro.org>
Subject: Re: [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands
Date: Fri, 6 Mar 2020 10:52:32 +0100 [thread overview]
Message-ID: <20200306095232.GB7240@linux.fritz.box> (raw)
In-Reply-To: <87pndq7xnj.fsf@dusky.pond.sub.org>
Am 06.03.2020 um 08:25 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 05.03.2020 um 16:30 hat Markus Armbruster geschrieben:
> >> Kevin Wolf <kwolf@redhat.com> writes:
> >>
> >> > Am 22.01.2020 um 07:32 hat Markus Armbruster geschrieben:
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >>
> >> >> > This patch adds a new 'coroutine' flag to QMP command definitions that
> >> >> > tells the QMP dispatcher that the command handler is safe to be run in a
> >> >> > coroutine.
> >> >>
> >> >> I'm afraid I missed this question in my review of v3: when is a handler
> >> >> *not* safe to be run in a coroutine?
> >> >
> >> > That's a hard one to answer fully.
> >> >
> >> > Basically, I think the biggest problem is with calling functions that
> >> > change their behaviour if run in a coroutine compared to running them
> >> > outside of coroutine context. In most cases the differences like having
> >> > a nested event loop instead of yielding are just fine, but they are
> >> > still subtly different.
> >> >
> >> > I know this is vague, but I can assure you that problematic cases exist.
> >> > I hit one of them with my initial hack that just moved everything into a
> >> > coroutine. It was related to graph modifications and bdrv_drain and
> >> > resulted in a hang. For the specifics, I would have to try and reproduce
> >> > the problem again.
> >>
> >> I'm afraid it's even more complicated.
> >>
> >> Monitors (HMP and QMP) run in the main loop. Before this patch, HMP and
> >> QMP commands run start to finish, one after the other.
> >>
> >> After this patch, QMP commands may elect to yield. QMP commands still
> >> run one after the other (the shared dispatcher ensures this even when we
> >> have multiple QMP monitors).
> >>
> >> However, *HMP* commands can now run interleaved with a coroutine-enabled
> >> QMP command, i.e. between a yield and its re-enter.
> >
> > I guess that's right. :-(
> >
> >> Consider an HMP screendump running in the middle of a coroutine-enabled
> >> QMP screendump, using Marc-André's patch. As far as I can tell, it
> >> works, because:
> >>
> >> 1. HMP does not run in a coroutine. If it did, and both dumps dumped
> >> the same @con, then it would overwrite con->screndump_co. If we ever
> >> decide to make HMP coroutine-capable so it doesn't block the main loop,
> >> this will become unsafe in a nasty way.
> >
> > At the same time, switching HMP to coroutines would give us an easy way
> > to fix the problem: Just use a CoMutex so that HMP and QMP commands
> > never run in parallel. Unless we actually _want_ to run both at the same
> > time for some commands, but I don't see why.
>
> As long as running QMP commands from *all* monitors one after the other
> is good enough, I can't see why running HMP commands interleaved is
> worth the risk.
There is one exception, actually: Obviously, human-monitor-command must
allow HMP commands to run in parallel.
> > While we don't have this, maybe it's worth considering if there is
> > another simple way to achieve the same thing. Could QMP just suspend all
> > HMP monitors while it's executing a command? At first sight it seems
> > that this would work only for "interactive" monitors.
>
> I believe the non-"interactive" HMP code is used only by gdbstub.c.
monitor_init_hmp() is called from (based on my block branch):
* monitor_init(): This is interactive.
* qemu_chr_new_noreplay(): Interactive, too.
* gdbserver_start(): Non-interactive.
There is also qmp_human_monitor_command(), which manually creates a
MonitorHMP without going through monitor_init_hmp(). It does call
monitor_data_init(), though. There are no additional callers of
monitor_data_init() and I think I would have added it to every creation
of a Monitor object when I did the QMP/HMP split of the struct.
So GDB and human-monitor-command should be the two non-interactive
cases.
> I keep forgetting our GDB server stub creates an "HMP" monitor.
> Possibly because I don't understand why. Alex, Philippe, can you
> enlighten me?
I think you can send HMP commands from within gdb with it:
(gdb) tar rem:1234
Remote debugging using :1234
[...]
(gdb) monitor info block
ide1-cd0: [not inserted]
Attached to: /machine/unattached/device[23]
Removable device: not locked, tray closed
floppy0: [not inserted]
Attached to: /machine/unattached/device[16]
Removable device: not locked, tray closed
sd0: [not inserted]
Removable device: not locked, tray closed
So we do want stop it from processing requests while a QMP command is
running.
Kevin
next prev parent reply other threads:[~2020-03-06 9:53 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-21 18:11 [PATCH v4 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-01-21 18:11 ` [PATCH v4 1/4] qapi: Add a 'coroutine' flag for commands Kevin Wolf
2020-01-22 6:32 ` Markus Armbruster
2020-01-22 10:10 ` Kevin Wolf
2020-01-22 12:15 ` Markus Armbruster
2020-01-22 14:35 ` Kevin Wolf
2020-03-05 15:30 ` Markus Armbruster
2020-03-05 16:06 ` Kevin Wolf
2020-03-06 7:25 ` Markus Armbruster
2020-03-06 9:52 ` Kevin Wolf [this message]
2020-03-06 12:38 ` Markus Armbruster
2020-03-06 14:26 ` Kevin Wolf
2020-02-17 7:08 ` Markus Armbruster
2020-01-21 18:11 ` [PATCH v4 2/4] vl: Initialise main loop earlier Kevin Wolf
2020-01-21 18:11 ` [PATCH v4 3/4] qmp: Move dispatcher to a coroutine Kevin Wolf
2020-02-17 11:08 ` Markus Armbruster
2020-02-17 12:34 ` Kevin Wolf
2020-02-18 14:12 ` Markus Armbruster
2020-02-18 15:29 ` Kevin Wolf
2020-02-19 9:03 ` Markus Armbruster
2020-02-19 10:22 ` Kevin Wolf
2020-02-19 14:21 ` Markus Armbruster
2020-02-19 14:26 ` Markus Armbruster
2020-02-19 15:27 ` Kevin Wolf
2020-01-21 18:11 ` [PATCH v4 4/4] block: Mark 'block_resize' as coroutine Kevin Wolf
2020-02-05 14:00 ` [PATCH v4 0/4] qmp: Optionally run handlers in coroutines Kevin Wolf
2020-02-12 11:40 ` Kevin Wolf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200306095232.GB7240@linux.fritz.box \
--to=kwolf@redhat.com \
--cc=alex.bennee@linaro.org \
--cc=armbru@redhat.com \
--cc=dgilbert@redhat.com \
--cc=marcandre.lureau@gmail.com \
--cc=philmd@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).