From: "Marc-André Lureau" <marcandre.lureau@gmail.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: QEMU <qemu-devel@nongnu.org>,
Markus Armbruster <armbru@redhat.com>,
Gerd Hoffmann <kraxel@redhat.com>,
David Alan Gilbert <dgilbert@redhat.com>
Subject: Re: [PATCH] console: make QMP screendump use coroutine
Date: Tue, 3 Mar 2020 11:56:15 +0100 [thread overview]
Message-ID: <CAJ+F1CKpJ6dyX2bNvqFmigcG4eePdTg3_Y4UEQ7_PKSV3Naxbg@mail.gmail.com> (raw)
In-Reply-To: <875zflevh1.fsf@dusky.pond.sub.org>
Hi
On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Kevin Wolf <kwolf@redhat.com> writes:
>
> > Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> >>
> >> > Hi
> >> >
> >> > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster <armbru@redhat.com> wrote:
> >> >>
> >> >> Kevin Wolf <kwolf@redhat.com> writes:
> >> >>
> >> >> > Am 20.02.2020 um 17:01 hat Markus Armbruster geschrieben:
> >> >> >> >> > void qmp_screendump(const char *filename, bool has_device, const char *device,
> >> >> >> >> > bool has_head, int64_t head, Error **errp)
> >> >> >> >> > {
> >> >> >> >> > QemuConsole *con;
> >> >> >> >> > DisplaySurface *surface;
> >> >> >> >> > + g_autoptr(pixman_image_t) image = NULL;
> >> >> >> >> > int fd;
> >> >> >> >> >
> >> >> >> >> > if (has_device) {
> >> >> >> >> > @@ -365,7 +375,15 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
> >> >> >> >> > }
> >> >> >> >> > }
> >> >> >> >> >
> >> >> >> >> > - graphic_hw_update(con);
> >> >> >> >> > + if (qemu_in_coroutine()) {
> >> >> >> >> > + assert(!con->screendump_co);
> >> >> >> >> > + con->screendump_co = qemu_coroutine_self();
> >> >> >> >> > + aio_bh_schedule_oneshot(qemu_get_aio_context(),
> >> >> >> >> > + graphic_hw_update_bh, con);
> >> >> >> >> > + qemu_coroutine_yield();
> >> >> >> >> > + con->screendump_co = NULL;
> >> >> >> >> > + }
> >> >> >> >>
> >> >> >> >> What if multiple QMP monitors simultaneously screendump? Hmm, it works
> >> >> >> >> because all execute one after another in the same coroutine
> >> >> >> >> qmp_dispatcher_co. Implicit mutual exclusion.
> >> >> >> >>
> >> >> >> >> Executing them one after another is bad, because it lets an ill-behaved
> >> >> >> >> QMP command starve *all* QMP monitors. We do it only out of
> >> >> >> >> (reasonable!) fear of implicit mutual exclusion requirements like the
> >> >> >> >> one you add.
> >> >> >> >>
> >> >> >> >> Let's not add more if we can help it.
> >> >> >> >
> >> >> >> > The situation is not worse than the current blocking handling.
> >> >> >>
> >> >> >> Really?
> >> >> >>
> >> >> >> What makes executing multiple qmp_screendump() concurrently (in separate
> >> >> >> threads) or interleaved (in separate coroutines in the same thread)
> >> >> >> unsafe before this patch?
> >> >> >
> >> >> > QMP command handlers are guaranteed to run in the main thread with the
> >> >> > BQL held, so there is no concurrency. If you want to change this, you
> >> >> > would have much more complicated problems to solve than in this handler.
> >> >> > I'm not sure it's fair to require thread-safety from one handler when
> >> >> > no other handler is thread safe (except accidentally) and nobody seems
> >> >> > to plan actually calling them from multiple threads.
> >> >>
> >> >> "Let's not [...] if we can help it." is hardly a "change this or else no
> >> >> merge" demand. It is a challenge to find a more elegant solution.
> >> >>
> >> >> >> >> Your screendump_co is per QemuConsole instead of per QMP monitor only
> >> >> >> >> because you need to find the coroutine in graphic_hw_update_done(). Can
> >> >> >> >> we somehow pass it via function arguments?
> >> >> >> >
> >> >> >> > I think it could be done later, so I suggest a TODO.
> >> >> >>
> >> >> >> We should avoid making our dependence on implicit mutual exclusion
> >> >> >> worse. When we do it anyway, a big, fat, ugly comment is definitely
> >> >> >> called for.
> >> >> >
> >> >> > Anyway, what I really wanted to add:
> >> >> >
> >> >> > This should be easy to solve by having a CoQueue instead of a single
> >> >>
> >> >> Ah, challenge accepted! Exactly the outcome I was hoping for :)
> >> >>
> >> >> > Coroutine pointer. The coroutine would just call qemu_co_queue_wait(),
> >> >> > which adds itself to the queue before it yields and the update
> >> >> > completion would wake up all coroutines that are currently queued with
> >> >> > qemu_co_queue_restart_all().
> >> >> >
> >> >> > qemu_co_queue_wait() takes a lock as its second parameter. You don't
> >> >> > need it in this context and can just pass NULL. (This is a lock that
> >> >> > would be dropped while the coroutine is sleeping and automatically
> >> >> > reacquired afterwards.)
> >> >> >
> >> >> >> >> In case avoiding the mutual exclusion is impractical: please explain it
> >> >> >> >> in a comment to make it somewhat less implicit.
> >> >> >>
> >> >> >> It is anything but: see appended patch.
> >> >> >
> >> >> > This works, too, but it requires an additional struct. I think the queue
> >> >> > is easier. (Note there is a difference in the mechanism: Your patch
> >> >> > waits for the specific update it triggered, while the CoQueue would wait
> >> >> > for _any_ update to complete. I assume effectively the result is the
> >> >> > same.)
> >> >>
> >> >> Your idea sounds much nicer to me. Thanks!
> >> >
> >> > Similar to the NULL check you asked to remove,
> >> > having a CoQueue there would lead to think that several concurrently
> >> > running screendump are possible.
> >> >
> >> > Is this a direction we are willing to take?
> >>
> >> Let's take a step back.
> >>
> >> The actual problem is to find the coroutine in graphic_hw_update_done(),
> >> so you can wake it.
> >>
> >> Your solution stores the coroutine in the QemuConsole, because that's
> >> readily available in graphic_hw_update_done().
> >>
> >> However, it really, really doesn't belong there, it belongs to the
> >> monitor. Works anyway only because QMP commands execute one after the
> >> other.
> >>
> >> Kevin suggested using a CoQueue to avoid this unspoken dependency. You
> >> object, because it could make readers assume multiple screendump
> >> commands could run concurrently, which is not the case.
> >>
> >> Alright, let's KISS: since there's just one main loop, there's just one
> >> coroutine: @qmp_dispatcher_co. Let's use that, so the dependency on
> >> "one command after the other" is explicit and obvious.
> >
> > Ugh... If you choose that this is the way to go, please add an assertion
> > at least that we are indeed in qmp_dispatcher_co before yielding.
>
> No objection.
>
> To apply the QMP coroutine infrastructure for 5.0, I need a user. We
> have two: block_resize from Kevin, and screendump from Marc-André.
> Neither is quite ready, yet. I'll wait for a respin of either one.
>
Kevin series has conflicts, I will wait for his respin.
--
Marc-André Lureau
next prev parent reply other threads:[~2020-03-03 10:57 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-01-13 14:48 [PATCH] console: make QMP screendump use coroutine Marc-André Lureau
2020-01-13 16:32 ` no-reply
2020-01-13 16:36 ` no-reply
2020-02-12 12:29 ` Gerd Hoffmann
2020-02-13 13:10 ` Markus Armbruster
2020-02-20 7:48 ` Markus Armbruster
2020-02-20 9:43 ` Marc-André Lureau
2020-02-20 16:01 ` Markus Armbruster
2020-02-20 20:11 ` Dr. David Alan Gilbert
2020-02-21 6:31 ` Markus Armbruster
2020-02-21 10:25 ` Dr. David Alan Gilbert
2020-02-21 10:07 ` Kevin Wolf
2020-02-21 16:50 ` Markus Armbruster
2020-02-24 16:20 ` Marc-André Lureau
2020-03-02 14:22 ` Markus Armbruster
2020-03-02 15:36 ` Kevin Wolf
2020-03-03 7:41 ` Markus Armbruster
2020-03-03 10:56 ` Marc-André Lureau [this message]
2020-03-03 14:47 ` Markus Armbruster
2020-03-03 16:03 ` Marc-André Lureau
2020-03-06 8:44 ` Markus Armbruster
2020-03-06 10:03 ` Marc-André Lureau
2020-03-11 12:16 ` Markus Armbruster
2020-03-05 14:41 ` Markus Armbruster
2020-03-05 15:08 ` Marc-André Lureau
2020-03-06 5:56 ` Markus Armbruster
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=CAJ+F1CKpJ6dyX2bNvqFmigcG4eePdTg3_Y4UEQ7_PKSV3Naxbg@mail.gmail.com \
--to=marcandre.lureau@gmail.com \
--cc=armbru@redhat.com \
--cc=dgilbert@redhat.com \
--cc=kraxel@redhat.com \
--cc=kwolf@redhat.com \
--cc=qemu-devel@nongnu.org \
/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).