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

[Kevin Wolf]

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.

> >> 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
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.)

Kevin