qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Markus Armbruster <armbru@redhat.com>
To: Kevin Wolf <kwolf@redhat.com>
Cc: "Gerd Hoffmann" <kraxel@redhat.com>,
	"Marc-André Lureau" <marcandre.lureau@gmail.com>,
	"Markus Armbruster" <armbru@redhat.com>,
	"David Alan Gilbert" <dgilbert@redhat.com>,
	QEMU <qemu-devel@nongnu.org>
Subject: Re: [PATCH] console: make QMP screendump use coroutine
Date: Fri, 21 Feb 2020 17:50:46 +0100	[thread overview]
Message-ID: <87pne751g9.fsf@dusky.pond.sub.org> (raw)
In-Reply-To: <20200221100700.GA5254@linux.fritz.box> (Kevin Wolf's message of "Fri, 21 Feb 2020 11:07:00 +0100")

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!



  reply	other threads:[~2020-02-21 16:53 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 [this message]
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
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=87pne751g9.fsf@dusky.pond.sub.org \
    --to=armbru@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=kraxel@redhat.com \
    --cc=kwolf@redhat.com \
    --cc=marcandre.lureau@gmail.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).