Re: [PATCH] console: make QMP screendump use coroutine

[Kevin Wolf]

Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> Marc-André Lureau <address@hidden> writes:
> 
> > Hi
> >
> > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster <address@hidden> wrote:
> >>
> >> Kevin Wolf <address@hidden> 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.

Kevin