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

[Marc-André Lureau]

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?

fwiw, my earlier async series did allow that, and was using a queue
for concurrent screendumps (but without coroutine & CoQueue, since it
was all traditional callback/events-based)



-- 
Marc-André Lureau