Re: [PATCH] console: make QMP screendump use coroutine
Marc-André Lureau writes:
> Hi
>
> On Fri, Mar 6, 2020 at 9:44 AM Markus Armbruster wrote:
>>
>> Marc-André Lureau writes:
>>
>> > Hi
>> >
>> > On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster wrote:
>> [...]
>> >> >> 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.
>>
>> As discussed in the "[PATCH v4 1/4] qapi: Add a 'coroutine' flag for
>> commands" sub-thread, HMP commands may execute interleaved. Your code
>> still works, because it only ever abuses QemuConsole with QMP. But it's
>> brittle.
>>
>> Looks like we'll change HMP not to run interleaved. That adds a belt to
>> the suspenders. You might argue that's robust enough.
>>
>> But it's not just the brittleness I dislike. Storing per-monitor-
>> command data in QemuConsole is ugly as sin. Arguing that it works
>> because commands are strictly serialized, and that burying one more
>> dependence on such serialization deep in command code won't make the
>> situation appreciably worse, doesn't change the fact that QemuConsole
>> has no business holding per-monitor-command data.
>
> It is data (the monitor coroutine) associated with an event handler
> (graphic-update).
>
> Someone has to hold the handler/data, and the console seems appropriate.
>
> We could abstract this a bit, for ex, having a GHookList, but as long
> as there is only one handler, it's unnecessary.
The correctness argument is non-local and relies on current limitations
of both QMP and HMP:
* QMP never interleaves commands execution, not even with multiple QMP
monitors. Complete HMP commands can still be interleaved with a QMP
command.
* QMP executes commands marked 'coroutine' in a coroutine. HMP does not
execute commands in coroutines.
* qmp_screendump() carefully avoids the graphic_hw_update() machinery
for HMP. It uses "running in coroutine" as a proxy for "HMP".
* No other user of the graphic_hw_update() machinery wants
graphic_hw_update_done() to wake up a coroutine.
* Therefore, at any time no more than one such update is for a user that
wants a coroutine woken up.
* Therefore, storing the coroutine to be woken up in QemuConsole is
safe.
If you insist that's just fine, please add a comment with the
correctness argument, and get Gerd's blessing for it.
I'd rather remove the need for such a longwinded and brittle argument,
but I'm not the maintainer of ui/ and hw/display/, Gerd is.
>
>>
>> >> >>
>> >> >> 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.
>> >>
>> >
>> > Is this the change you expect?
>> >
>> > diff --git a/ui/console.c b/ui/console.c
>> > index 57df3a5439..d6a8bf0cee 100644
>> > --- a/ui/console.c
>> > +++ b/ui/console.c
>> > @@ -167,7 +167,7 @@ struct QemuConsole {
>> > QEMUFIFO out_fifo;
>> > uint8_t out_fifo_buf[16];
>> > QEMUTimer *kbd_timer;
>> > -Coroutine *screendump_co;
>> > +bool wake_qmp_dispatcher_on_update;
>> >
>> > QTAILQ_ENTRY(QemuConsole) next;
>> > };
>>
>> No, because it still stores per-command data in QemuConsole. You need
>> to, because...
>>
>> > @@ -263,8 +263,8 @@ static void gui_setup_refresh(DisplayState *ds)
>> >
>> > void graphic_hw_update_done(QemuConsole *con)
>> > {
>> > -if (con && con->screendump_co) {
>> > -aio_co_wake(con->screendump_co);
>> > +if (con->wake_qmp_dispatcher_on_update) {
>> > +aio_co_wake(qmp_dispatcher_co);
>>
>> ... you may call aio_co_wake() only while @qmp_dispatcher_co is waiting
>> for it after yielding ...
>>
>> > }
>> > }
>> >
>> > @@ -376,12 +376,15 @@ void qmp_screendump(const char *filename, bool
>> > has_device, const char *device,
>> > }
>> >
>> > if (qemu_in_coroutine()) {
>> > -assert(!con->screendump_co);
>> > -con->screendump_co = qemu_coroutine_self();
>> > +
Re: [PATCH] console: make QMP screendump use coroutine
Hi
On Fri, Mar 6, 2020 at 9:44 AM Markus Armbruster wrote:
>
> Marc-André Lureau writes:
>
> > Hi
> >
> > On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster wrote:
> [...]
> >> >> 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.
>
> As discussed in the "[PATCH v4 1/4] qapi: Add a 'coroutine' flag for
> commands" sub-thread, HMP commands may execute interleaved. Your code
> still works, because it only ever abuses QemuConsole with QMP. But it's
> brittle.
>
> Looks like we'll change HMP not to run interleaved. That adds a belt to
> the suspenders. You might argue that's robust enough.
>
> But it's not just the brittleness I dislike. Storing per-monitor-
> command data in QemuConsole is ugly as sin. Arguing that it works
> because commands are strictly serialized, and that burying one more
> dependence on such serialization deep in command code won't make the
> situation appreciably worse, doesn't change the fact that QemuConsole
> has no business holding per-monitor-command data.
It is data (the monitor coroutine) associated with an event handler
(graphic-update).
Someone has to hold the handler/data, and the console seems appropriate.
We could abstract this a bit, for ex, having a GHookList, but as long
as there is only one handler, it's unnecessary.
>
> >> >>
> >> >> 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.
> >>
> >
> > Is this the change you expect?
> >
> > diff --git a/ui/console.c b/ui/console.c
> > index 57df3a5439..d6a8bf0cee 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -167,7 +167,7 @@ struct QemuConsole {
> > QEMUFIFO out_fifo;
> > uint8_t out_fifo_buf[16];
> > QEMUTimer *kbd_timer;
> > -Coroutine *screendump_co;
> > +bool wake_qmp_dispatcher_on_update;
> >
> > QTAILQ_ENTRY(QemuConsole) next;
> > };
>
> No, because it still stores per-command data in QemuConsole. You need
> to, because...
>
> > @@ -263,8 +263,8 @@ static void gui_setup_refresh(DisplayState *ds)
> >
> > void graphic_hw_update_done(QemuConsole *con)
> > {
> > -if (con && con->screendump_co) {
> > -aio_co_wake(con->screendump_co);
> > +if (con->wake_qmp_dispatcher_on_update) {
> > +aio_co_wake(qmp_dispatcher_co);
>
> ... you may call aio_co_wake() only while @qmp_dispatcher_co is waiting
> for it after yielding ...
>
> > }
> > }
> >
> > @@ -376,12 +376,15 @@ void qmp_screendump(const char *filename, bool
> > has_device, const char *device,
> > }
> >
> > if (qemu_in_coroutine()) {
> > -assert(!con->screendump_co);
> > -con->screendump_co = qemu_coroutine_self();
> > +/*
> > + * The coroutine code is generic, but we are supposed to be on
> > + * the QMP dispatcher coroutine, and we will resume only that now.
> > + */
> > +assert(qemu_coroutine_self() == qmp_dispatcher_co);
> > +con->wake_qmp_dispatcher_on_update = true;
> > aio_bh_schedule_oneshot(qemu_get_aio_context(),
> > graphic_hw_update_bh, con);
> > qemu_coroutine_yield();
>
> ... here. I missed that need when I suggested to use
> @qmp_dispatcher_co. Sorry.
>
> > -con->screendump_co = NULL;
> > +con->wake_qmp_dispatcher_on_update = false;
> > }
>
> Have a look at qxl, the only provider of asynchronous .gfx_update().
> The actual work is done in qxl-render.c. qxl_render_update(),
> qxl_render_update_area_bh(), qxl_render_update_area_unlocked(),
> qxl_render_update_area_done() cooperate carefully to support multiple
> updates in flight.
>
> I guess that's necessary because we also call graphic_hw_update() from
> display code such as ui/vnc.c and ui/spice-display.c.
>
> Before your patch, none of these users waits for an asynchronous update
> to complete,
Re: [PATCH] console: make QMP screendump use coroutine
Marc-André Lureau writes:
> Hi
>
> On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster wrote:
[...]
>> >> 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.
As discussed in the "[PATCH v4 1/4] qapi: Add a 'coroutine' flag for
commands" sub-thread, HMP commands may execute interleaved. Your code
still works, because it only ever abuses QemuConsole with QMP. But it's
brittle.
Looks like we'll change HMP not to run interleaved. That adds a belt to
the suspenders. You might argue that's robust enough.
But it's not just the brittleness I dislike. Storing per-monitor-
command data in QemuConsole is ugly as sin. Arguing that it works
because commands are strictly serialized, and that burying one more
dependence on such serialization deep in command code won't make the
situation appreciably worse, doesn't change the fact that QemuConsole
has no business holding per-monitor-command data.
>> >>
>> >> 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.
>>
>
> Is this the change you expect?
>
> diff --git a/ui/console.c b/ui/console.c
> index 57df3a5439..d6a8bf0cee 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -167,7 +167,7 @@ struct QemuConsole {
> QEMUFIFO out_fifo;
> uint8_t out_fifo_buf[16];
> QEMUTimer *kbd_timer;
> -Coroutine *screendump_co;
> +bool wake_qmp_dispatcher_on_update;
>
> QTAILQ_ENTRY(QemuConsole) next;
> };
No, because it still stores per-command data in QemuConsole. You need
to, because...
> @@ -263,8 +263,8 @@ static void gui_setup_refresh(DisplayState *ds)
>
> void graphic_hw_update_done(QemuConsole *con)
> {
> -if (con && con->screendump_co) {
> -aio_co_wake(con->screendump_co);
> +if (con->wake_qmp_dispatcher_on_update) {
> +aio_co_wake(qmp_dispatcher_co);
... you may call aio_co_wake() only while @qmp_dispatcher_co is waiting
for it after yielding ...
> }
> }
>
> @@ -376,12 +376,15 @@ void qmp_screendump(const char *filename, bool
> has_device, const char *device,
> }
>
> if (qemu_in_coroutine()) {
> -assert(!con->screendump_co);
> -con->screendump_co = qemu_coroutine_self();
> +/*
> + * The coroutine code is generic, but we are supposed to be on
> + * the QMP dispatcher coroutine, and we will resume only that now.
> + */
> +assert(qemu_coroutine_self() == qmp_dispatcher_co);
> +con->wake_qmp_dispatcher_on_update = true;
> aio_bh_schedule_oneshot(qemu_get_aio_context(),
> graphic_hw_update_bh, con);
> qemu_coroutine_yield();
... here. I missed that need when I suggested to use
@qmp_dispatcher_co. Sorry.
> -con->screendump_co = NULL;
> +con->wake_qmp_dispatcher_on_update = false;
> }
Have a look at qxl, the only provider of asynchronous .gfx_update().
The actual work is done in qxl-render.c. qxl_render_update(),
qxl_render_update_area_bh(), qxl_render_update_area_unlocked(),
qxl_render_update_area_done() cooperate carefully to support multiple
updates in flight.
I guess that's necessary because we also call graphic_hw_update() from
display code such as ui/vnc.c and ui/spice-display.c.
Before your patch, none of these users waits for an asynchronous update
to complete, as far as I can tell. Afterwards, QMP screendump does.
Whether more users should I can't tell. Gerd, can you?
Your patch communicates completion to screendump by making
graphic_hw_update() wake a coroutine. It stores the coroutine in
QemuConsole, exploiting that only one call site actually waits for an
asynchronous update to complete, and that caller is never reentered.
This new mechanism is not usable for any other caller, unless it somehow
synchronizes with screendump to avoid reentrance.
Shouldn't we offer a more generally useful way to wait for asynchronous
Re: [PATCH] console: make QMP screendump use coroutine
Marc-André Lureau writes:
> Hi
>
> On Thu, Mar 5, 2020 at 3:46 PM Markus Armbruster wrote:
>>
>> I tried to observe the main loop keeps running while the screendump does
>> its work.
>>
>> The main loop appears to lack trace points. Alright, if there's no
>> hammer handy, I'll use a rock:
>>
>> diff --git a/softmmu/vl.c b/softmmu/vl.c
>> index 5549f4b619..b6561a65d7 100644
>> --- a/softmmu/vl.c
>> +++ b/softmmu/vl.c
>> @@ -1661,6 +1661,7 @@ void qemu_main_loop(void)
>> #ifdef CONFIG_PROFILER
>> ti = profile_getclock();
>> #endif
>> +printf("*** main loop\n");
>> main_loop_wait(false);
>> #ifdef CONFIG_PROFILER
>> dev_time += profile_getclock() - ti;
>>
>>
>> First experiment: does the main loop continue to run when writing out
>> the screendump blocks / would block?
>>
>> Observe qmp_screendump() opens the file without O_EXCL. Great, that
>> lets me block output by making it open a FIFO.
>>
>> Terminal#1:
>>
>> $ mkfifo s
>>
>> Terminal#2:
>>
>> $ upstream-qemu -S -display none -chardev
>> socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
>> *** main loop
>> *** main loop
>> *** main loop
>>
>> Keeps printing at a steady pace.
>>
>> Terminal#3:
>>
>> $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>"
>> UNIX-CONNECT:$HOME/work/images/test-qmp
>> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 4},
>> "package": "v4.2.0-2069-g5e5ae6b644-dirty"}, "capabilities": ["oob"]}}
>> QMP>{"execute": "qmp_capabilities"}
>> {"return": {}}
>> QMP>{"execute": "screendump", "arguments": {"filename": "s"}}
>>
>> The printing in terminal#2 stops. This is expected; qemu_open() calls
>> open(), which blocks, because the FIFO has no reader.
>>
>> Terminal#1:
>>
>> $ exec 4>
>> Now the FIFO has a reader. Terminal#2 remains quiet.
>>
>> We now hang in ppm_save(). Abridged stack backtrace:
>>
>> #0 0x7519d0f5 in writev () at /lib64/libc.so.6
>> #1 0x55e15f61 in qio_channel_file_writev
>> (ioc=0x567bf5f0, iov=0x56a441b0, niov=1, fds=0x0, nfds=0,
>> errp=0x7fffe9d81d10) at /work/armbru/qemu/io/channel-file.c:123
>> #2 0x55e133d3 in qio_channel_writev_full
>> (ioc=0x567bf5f0, iov=0x56a441b0, niov=1, fds=0x0, nfds=0,
>> errp=0x7fffe9d81d10) at /work/armbru/qemu/io/channel.c:86
>> #3 0x55e137a2 in qio_channel_writev
>> (ioc=0x567bf5f0, iov=0x56a441b0, niov=1, errp=0x7fffe9d81d10)
>> at /work/armbru/qemu/io/channel.c:207
>> #4 0x55e13696 in qio_channel_writev_all
>> (ioc=0x567bf5f0, iov=0x7fffe9d81bd0, niov=1, errp=0x7fffe9d81d10)
>> at /work/armbru/qemu/io/channel.c:171
>> #5 0x55e139b1 in qio_channel_write_all
>> (ioc=0x567bf5f0, buf=0x56b05200 "", buflen=1920,
>> errp=0x7fffe9d81d10) at /work/armbru/qemu/io/channel.c:257
>> #6 0x55cd74ff in ppm_save
>> (fd=22, image=0x568ffdd0, errp=0x7fffe9d81d10)
>> at /work/armbru/qemu/ui/console.c:336
>> #7 0x55cd77e6 in qmp_screendump
>> (filename=0x56ea0900 "s", has_device=false, device=0x0,
>> has_head=false, head=0, errp=0x7fffe9d81d10) at
>> /work/armbru/qemu/ui/console.c:401
>>
>> A brief inspection of qio_channel_file_writev() and
>> qio_channel_writev_all() suggests this might work if you make the output
>> file descriptor non-blocking.
>
> Right, the goal was rather originally to fix rhbz#1230527. We got
> coroutine IO by accident, and I was too optimistic about default
> behaviour change ;) I will update the patch.
>
>>
>> $ head -c 1 <&4 | hexdump -C
>> 50|P|
>> 0001
>>
>> Still quiet.
>>
>> $ cat <&4 >/dev/null
>>
>> The printing resumes.
>>
>> $ exec 4<&-
>>
>>
>> Second experiment: does the main loop continue to run while we wait for
>> graphic_hw_update_done()?
>>
>> Left as an exercise for the patch submitter ;)
>>
>>
>
> With your main loop printf, one printf in graphic_hw_update() and one
> in graphic_hw_update_done() ? (rather part of testing commit
> 4d6316218bf7bf3b8c7c7165b072cc314511a7a7, soon 4y old!)
We might also need to artificially delay graphic_hw_update_done().
Re: [PATCH] console: make QMP screendump use coroutine
Hi
On Thu, Mar 5, 2020 at 3:46 PM Markus Armbruster wrote:
>
> I tried to observe the main loop keeps running while the screendump does
> its work.
>
> The main loop appears to lack trace points. Alright, if there's no
> hammer handy, I'll use a rock:
>
> diff --git a/softmmu/vl.c b/softmmu/vl.c
> index 5549f4b619..b6561a65d7 100644
> --- a/softmmu/vl.c
> +++ b/softmmu/vl.c
> @@ -1661,6 +1661,7 @@ void qemu_main_loop(void)
> #ifdef CONFIG_PROFILER
> ti = profile_getclock();
> #endif
> +printf("*** main loop\n");
> main_loop_wait(false);
> #ifdef CONFIG_PROFILER
> dev_time += profile_getclock() - ti;
>
>
> First experiment: does the main loop continue to run when writing out
> the screendump blocks / would block?
>
> Observe qmp_screendump() opens the file without O_EXCL. Great, that
> lets me block output by making it open a FIFO.
>
> Terminal#1:
>
> $ mkfifo s
>
> Terminal#2:
>
> $ upstream-qemu -S -display none -chardev
> socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
> *** main loop
> *** main loop
> *** main loop
>
> Keeps printing at a steady pace.
>
> Terminal#3:
>
> $ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>"
> UNIX-CONNECT:$HOME/work/images/test-qmp
> {"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 4},
> "package": "v4.2.0-2069-g5e5ae6b644-dirty"}, "capabilities": ["oob"]}}
> QMP>{"execute": "qmp_capabilities"}
> {"return": {}}
> QMP>{"execute": "screendump", "arguments": {"filename": "s"}}
>
> The printing in terminal#2 stops. This is expected; qemu_open() calls
> open(), which blocks, because the FIFO has no reader.
>
> Terminal#1:
>
> $ exec 4
> Now the FIFO has a reader. Terminal#2 remains quiet.
>
> We now hang in ppm_save(). Abridged stack backtrace:
>
> #0 0x7519d0f5 in writev () at /lib64/libc.so.6
> #1 0x55e15f61 in qio_channel_file_writev
> (ioc=0x567bf5f0, iov=0x56a441b0, niov=1, fds=0x0, nfds=0,
> errp=0x7fffe9d81d10) at /work/armbru/qemu/io/channel-file.c:123
> #2 0x55e133d3 in qio_channel_writev_full
> (ioc=0x567bf5f0, iov=0x56a441b0, niov=1, fds=0x0, nfds=0,
> errp=0x7fffe9d81d10) at /work/armbru/qemu/io/channel.c:86
> #3 0x55e137a2 in qio_channel_writev
> (ioc=0x567bf5f0, iov=0x56a441b0, niov=1, errp=0x7fffe9d81d10)
> at /work/armbru/qemu/io/channel.c:207
> #4 0x55e13696 in qio_channel_writev_all
> (ioc=0x567bf5f0, iov=0x7fffe9d81bd0, niov=1, errp=0x7fffe9d81d10)
> at /work/armbru/qemu/io/channel.c:171
> #5 0x55e139b1 in qio_channel_write_all
> (ioc=0x567bf5f0, buf=0x56b05200 "", buflen=1920,
> errp=0x7fffe9d81d10) at /work/armbru/qemu/io/channel.c:257
> #6 0x55cd74ff in ppm_save
> (fd=22, image=0x568ffdd0, errp=0x7fffe9d81d10)
> at /work/armbru/qemu/ui/console.c:336
> #7 0x55cd77e6 in qmp_screendump
> (filename=0x56ea0900 "s", has_device=false, device=0x0,
> has_head=false, head=0, errp=0x7fffe9d81d10) at
> /work/armbru/qemu/ui/console.c:401
>
> A brief inspection of qio_channel_file_writev() and
> qio_channel_writev_all() suggests this might work if you make the output
> file descriptor non-blocking.
Right, the goal was rather originally to fix rhbz#1230527. We got
coroutine IO by accident, and I was too optimistic about default
behaviour change ;) I will update the patch.
>
> $ head -c 1 <&4 | hexdump -C
> 50|P|
> 0001
>
> Still quiet.
>
> $ cat <&4 >/dev/null
>
> The printing resumes.
>
> $ exec 4<&-
>
>
> Second experiment: does the main loop continue to run while we wait for
> graphic_hw_update_done()?
>
> Left as an exercise for the patch submitter ;)
>
>
With your main loop printf, one printf in graphic_hw_update() and one
in graphic_hw_update_done() ? (rather part of testing commit
4d6316218bf7bf3b8c7c7165b072cc314511a7a7, soon 4y old!)
--
Marc-André Lureau
Re: [PATCH] console: make QMP screendump use coroutine
I tried to observe the main loop keeps running while the screendump does
its work.
The main loop appears to lack trace points. Alright, if there's no
hammer handy, I'll use a rock:
diff --git a/softmmu/vl.c b/softmmu/vl.c
index 5549f4b619..b6561a65d7 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -1661,6 +1661,7 @@ void qemu_main_loop(void)
#ifdef CONFIG_PROFILER
ti = profile_getclock();
#endif
+printf("*** main loop\n");
main_loop_wait(false);
#ifdef CONFIG_PROFILER
dev_time += profile_getclock() - ti;
First experiment: does the main loop continue to run when writing out
the screendump blocks / would block?
Observe qmp_screendump() opens the file without O_EXCL. Great, that
lets me block output by making it open a FIFO.
Terminal#1:
$ mkfifo s
Terminal#2:
$ upstream-qemu -S -display none -chardev
socket,id=qmp,path=test-qmp,server=on,wait=off -mon mode=control,chardev=qmp
*** main loop
*** main loop
*** main loop
Keeps printing at a steady pace.
Terminal#3:
$ socat "READLINE,history=$HOME/.qmp_history,prompt=QMP>"
UNIX-CONNECT:$HOME/work/images/test-qmp
{"QMP": {"version": {"qemu": {"micro": 50, "minor": 2, "major": 4},
"package": "v4.2.0-2069-g5e5ae6b644-dirty"}, "capabilities": ["oob"]}}
QMP>{"execute": "qmp_capabilities"}
{"return": {}}
QMP>{"execute": "screendump", "arguments": {"filename": "s"}}
The printing in terminal#2 stops. This is expected; qemu_open() calls
open(), which blocks, because the FIFO has no reader.
Terminal#1:
$ exec 4/dev/null
The printing resumes.
$ exec 4<&-
Second experiment: does the main loop continue to run while we wait for
graphic_hw_update_done()?
Left as an exercise for the patch submitter ;)
Re: [PATCH] console: make QMP screendump use coroutine
Hi
On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster wrote:
>
> Kevin Wolf writes:
>
> > Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> >> Marc-André Lureau writes:
> >>
> >> > Hi
> >> >
> >> > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster
> >> > wrote:
> >> >>
> >> >> Kevin Wolf 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
Re: [PATCH] console: make QMP screendump use coroutine
Marc-André Lureau writes: [...] >> 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. The conflict is trivial. I'll prepare a base for you.
Re: [PATCH] console: make QMP screendump use coroutine
Hi
On Tue, Mar 3, 2020 at 8:41 AM Markus Armbruster wrote:
>
> Kevin Wolf writes:
>
> > Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> >> Marc-André Lureau writes:
> >>
> >> > Hi
> >> >
> >> > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster
> >> > wrote:
> >> >>
> >> >> Kevin Wolf 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
Re: [PATCH] console: make QMP screendump use coroutine
Kevin Wolf writes:
> Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
>> Marc-André Lureau writes:
>>
>> > Hi
>> >
>> > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster
>> > wrote:
>> >>
>> >> Kevin Wolf 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.
>> >
>> >
Re: [PATCH] console: make QMP screendump use coroutine
Am 02.03.2020 um 15:22 hat Markus Armbruster geschrieben:
> Marc-André Lureau writes:
>
> > Hi
> >
> > On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster wrote:
> >>
> >> Kevin Wolf 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.
>
Re: [PATCH] console: make QMP screendump use coroutine
Marc-André Lureau writes:
> Hi
>
> On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster wrote:
>>
>> Kevin Wolf 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
Re: [PATCH] console: make QMP screendump use coroutine
Hi
On Fri, Feb 21, 2020 at 5:50 PM Markus Armbruster wrote:
>
> Kevin Wolf 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
Re: [PATCH] console: make QMP screendump use coroutine
Kevin Wolf 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!
Re: [PATCH] console: make QMP screendump use coroutine
* Markus Armbruster (arm...@redhat.com) wrote: > "Dr. David Alan Gilbert" writes: > > > * Markus Armbruster (arm...@redhat.com) wrote: > [...] > >> Collecting several users before building infrastructure makes sense when > >> the design of the infrastructure isn't obvious, or when the need for it > >> is in doubt. > >> > >> Neither is the case for running QMP handlers in a coroutine: QMP > >> commands blocking the main loop is without doubt a problem we need to > >> solve, and the way to solve it was obvious enough for Kevin to do it > >> with one user: block_resize. A second one quickly followed: screendump. > >> > >> The only part that's different for HMP, I think, is "need". > >> > >> Is HMP blocking the main loop a problem? > >> > >> If yes, is it serious enough to justify solving it? > > > > I don't mind if HMP blocks for a small time while doing something, but > > not if it can hang if the guest (or something else like it) misbehaves. > > Not if it's something you might need to issue another command to recover > > from. > > The issue isn't HMP being unavailable while a command executes. The > issue is HMP stopping the main loop while a command executes. > > Stopping the main loop not only stops everything running there, it can > also stop other threads when they synchronize with the main loop via the > Big QEMU Lock. Yep. > The obvious example is a command accessing a remote filesystem. Special > case: NFS with the hard option can hang indefinitely. That I don't worry about too much for HMP; if your host is hosed, fix your host. > screendump does that, and also waits for asynchronous gfx_update() with > qxl devices. Networking again, with a different peer. That I would worry about since that's probably got interactions with the guest and spice, and all the type of things you might be trying to debug or test. > We already decided that QMP commands stopping the main loop is serious. > > To say it's not serious for HMP amounts to "don't do that then, use > QMP". Which may be fair. Not for me to decide, though. It's certainly more important for QMP; you don't want the main lock being held for everyday type of interactions with management layers. Dave -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: [PATCH] console: make QMP screendump use coroutine
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
Re: [PATCH] console: make QMP screendump use coroutine
"Dr. David Alan Gilbert" writes: > * Markus Armbruster (arm...@redhat.com) wrote: [...] >> Collecting several users before building infrastructure makes sense when >> the design of the infrastructure isn't obvious, or when the need for it >> is in doubt. >> >> Neither is the case for running QMP handlers in a coroutine: QMP >> commands blocking the main loop is without doubt a problem we need to >> solve, and the way to solve it was obvious enough for Kevin to do it >> with one user: block_resize. A second one quickly followed: screendump. >> >> The only part that's different for HMP, I think, is "need". >> >> Is HMP blocking the main loop a problem? >> >> If yes, is it serious enough to justify solving it? > > I don't mind if HMP blocks for a small time while doing something, but > not if it can hang if the guest (or something else like it) misbehaves. > Not if it's something you might need to issue another command to recover > from. The issue isn't HMP being unavailable while a command executes. The issue is HMP stopping the main loop while a command executes. Stopping the main loop not only stops everything running there, it can also stop other threads when they synchronize with the main loop via the Big QEMU Lock. The obvious example is a command accessing a remote filesystem. Special case: NFS with the hard option can hang indefinitely. screendump does that, and also waits for asynchronous gfx_update() with qxl devices. Networking again, with a different peer. We already decided that QMP commands stopping the main loop is serious. To say it's not serious for HMP amounts to "don't do that then, use QMP". Which may be fair. Not for me to decide, though.
Re: [PATCH] console: make QMP screendump use coroutine
* Markus Armbruster (arm...@redhat.com) wrote:
> Cc: David for questions regarding the HMP core. David, please look for
> "Is HMP blocking the main loop a problem?"
>
> Marc-André Lureau writes:
>
> > Hi
> >
> > On Thu, Feb 20, 2020 at 8:49 AM Markus Armbruster wrote:
> >>
> >> Marc-André Lureau writes:
> >>
> >> > Thanks to the QMP coroutine support, the screendump handler can
> >> > trigger a graphic_hw_update(), yield and let the main loop run until
> >> > update is done. Then the handler is resumed, and the ppm_save() will
> >> > write the screen image to disk in the coroutine context (thus
> >> > non-blocking).
> >> >
> >> > For now, HMP doesn't have coroutine support, so it remains potentially
> >> > outdated or glitched.
> >> >
> >> > Fixes:
> >> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527
> >> >
> >> > Based-on: <20200109183545.27452-2-kw...@redhat.com>
> >> >
> >> > Cc: Kevin Wolf
> >> > Signed-off-by: Marc-André Lureau
> >> > ---
> >> > qapi/ui.json| 3 ++-
> >> > ui/console.c| 35 +++
> >> > ui/trace-events | 2 +-
> >> > 3 files changed, 30 insertions(+), 10 deletions(-)
> >> >
> >> > diff --git a/qapi/ui.json b/qapi/ui.json
> >> > index e04525d8b4..d941202f34 100644
> >> > --- a/qapi/ui.json
> >> > +++ b/qapi/ui.json
> >> > @@ -96,7 +96,8 @@
> >> > #
> >> > ##
> >> > { 'command': 'screendump',
> >> > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
> >> > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
> >> > + 'coroutine': true }
> >> >
> >> > ##
> >> > # == Spice
> >> > diff --git a/ui/console.c b/ui/console.c
> >> > index ac79d679f5..db184b473f 100644
> >> > --- a/ui/console.c
> >> > +++ b/ui/console.c
> >> > @@ -167,6 +167,7 @@ struct QemuConsole {
> >> > QEMUFIFO out_fifo;
> >> > uint8_t out_fifo_buf[16];
> >> > QEMUTimer *kbd_timer;
> >> > +Coroutine *screendump_co;
> >> >
> >> > QTAILQ_ENTRY(QemuConsole) next;
> >> > };
> >> > @@ -194,7 +195,6 @@ static void dpy_refresh(DisplayState *s);
> >> > static DisplayState *get_alloc_displaystate(void);
> >> > static void text_console_update_cursor_timer(void);
> >> > static void text_console_update_cursor(void *opaque);
> >> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
> >> >
> >> > static void gui_update(void *opaque)
> >> > {
> >> > @@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
> >> >
> >> > void graphic_hw_update_done(QemuConsole *con)
> >> > {
> >> > +if (con && con->screendump_co) {
> >>
> >> How can !con happen?
> >
> > I don't think it can happen anymore (the patch evolved over several
> > years, this is probably a left-over). In any case, it doesn't hurt.
>
> I hate such dead checks, because they make me assume they can actually
> happen. Incorrect assumptions breed bugs.
>
> But I'm willing to defer to the maintainer here. Gerd?
>
> >> > +aio_co_wake(con->screendump_co);
> >> > +}
> >> > }
> >> >
> >> > void graphic_hw_update(QemuConsole *con)
> >> > @@ -310,16 +313,16 @@ void graphic_hw_invalidate(QemuConsole *con)
> >> > }
> >> > }
> >> >
> >> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
> >> > +static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
> >> > {
> >> > -int width = pixman_image_get_width(ds->image);
> >> > -int height = pixman_image_get_height(ds->image);
> >> > +int width = pixman_image_get_width(image);
> >> > +int height = pixman_image_get_height(image);
> >> > g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
> >> > g_autofree char *header = NULL;
> >> > g_autoptr(pixman_image_t) linebuf = NULL;
> >> > int y;
> >> >
> >> > -trace_ppm_save(fd, ds);
> >> > +trace_ppm_save(fd, image);
> >> >
> >> > header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
> >> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
> >> > @@ -329,7 +332,7 @@ static bool ppm_save(int fd, DisplaySurface *ds,
> >> > Error **errp)
> >> >
> >> > linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
> >> > for (y = 0; y < height; y++) {
> >> > -qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
> >> > +qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
> >> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
> >> >(char
> >> > *)pixman_image_get_data(linebuf),
> >> >pixman_image_get_stride(linebuf),
> >> > errp) < 0) {
> >>
> >> Looks like an unrelated optimization / simplification. If I was
> >> maintainer, I'd ask for a separate patch.
> >
> > I can be split, but it's related. We should pass a reference to
> > pixman_image_t, rather than a pointer to DisplaySurface, as the
> > underlying image may change over time, and would result in corrupted
> > coroutine save or worse.
>
> Work that into your commit message,
Re: [PATCH] console: make QMP screendump use coroutine
Cc: David for questions regarding the HMP core. David, please look for
"Is HMP blocking the main loop a problem?"
Marc-André Lureau writes:
> Hi
>
> On Thu, Feb 20, 2020 at 8:49 AM Markus Armbruster wrote:
>>
>> Marc-André Lureau writes:
>>
>> > Thanks to the QMP coroutine support, the screendump handler can
>> > trigger a graphic_hw_update(), yield and let the main loop run until
>> > update is done. Then the handler is resumed, and the ppm_save() will
>> > write the screen image to disk in the coroutine context (thus
>> > non-blocking).
>> >
>> > For now, HMP doesn't have coroutine support, so it remains potentially
>> > outdated or glitched.
>> >
>> > Fixes:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>> >
>> > Based-on: <20200109183545.27452-2-kw...@redhat.com>
>> >
>> > Cc: Kevin Wolf
>> > Signed-off-by: Marc-André Lureau
>> > ---
>> > qapi/ui.json| 3 ++-
>> > ui/console.c| 35 +++
>> > ui/trace-events | 2 +-
>> > 3 files changed, 30 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index e04525d8b4..d941202f34 100644
>> > --- a/qapi/ui.json
>> > +++ b/qapi/ui.json
>> > @@ -96,7 +96,8 @@
>> > #
>> > ##
>> > { 'command': 'screendump',
>> > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
>> > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
>> > + 'coroutine': true }
>> >
>> > ##
>> > # == Spice
>> > diff --git a/ui/console.c b/ui/console.c
>> > index ac79d679f5..db184b473f 100644
>> > --- a/ui/console.c
>> > +++ b/ui/console.c
>> > @@ -167,6 +167,7 @@ struct QemuConsole {
>> > QEMUFIFO out_fifo;
>> > uint8_t out_fifo_buf[16];
>> > QEMUTimer *kbd_timer;
>> > +Coroutine *screendump_co;
>> >
>> > QTAILQ_ENTRY(QemuConsole) next;
>> > };
>> > @@ -194,7 +195,6 @@ static void dpy_refresh(DisplayState *s);
>> > static DisplayState *get_alloc_displaystate(void);
>> > static void text_console_update_cursor_timer(void);
>> > static void text_console_update_cursor(void *opaque);
>> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
>> >
>> > static void gui_update(void *opaque)
>> > {
>> > @@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
>> >
>> > void graphic_hw_update_done(QemuConsole *con)
>> > {
>> > +if (con && con->screendump_co) {
>>
>> How can !con happen?
>
> I don't think it can happen anymore (the patch evolved over several
> years, this is probably a left-over). In any case, it doesn't hurt.
I hate such dead checks, because they make me assume they can actually
happen. Incorrect assumptions breed bugs.
But I'm willing to defer to the maintainer here. Gerd?
>> > +aio_co_wake(con->screendump_co);
>> > +}
>> > }
>> >
>> > void graphic_hw_update(QemuConsole *con)
>> > @@ -310,16 +313,16 @@ void graphic_hw_invalidate(QemuConsole *con)
>> > }
>> > }
>> >
>> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
>> > +static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
>> > {
>> > -int width = pixman_image_get_width(ds->image);
>> > -int height = pixman_image_get_height(ds->image);
>> > +int width = pixman_image_get_width(image);
>> > +int height = pixman_image_get_height(image);
>> > g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
>> > g_autofree char *header = NULL;
>> > g_autoptr(pixman_image_t) linebuf = NULL;
>> > int y;
>> >
>> > -trace_ppm_save(fd, ds);
>> > +trace_ppm_save(fd, image);
>> >
>> > header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
>> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
>> > @@ -329,7 +332,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error
>> > **errp)
>> >
>> > linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
>> > for (y = 0; y < height; y++) {
>> > -qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
>> > +qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
>> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
>> >(char *)pixman_image_get_data(linebuf),
>> >pixman_image_get_stride(linebuf), errp)
>> > < 0) {
>>
>> Looks like an unrelated optimization / simplification. If I was
>> maintainer, I'd ask for a separate patch.
>
> I can be split, but it's related. We should pass a reference to
> pixman_image_t, rather than a pointer to DisplaySurface, as the
> underlying image may change over time, and would result in corrupted
> coroutine save or worse.
Work that into your commit message, please. Might be easier if you
split, but that's between you and the maintainer :)
>> > @@ -340,11 +343,18 @@ static bool ppm_save(int fd, DisplaySurface *ds,
>> > Error **errp)
>> > return true;
>> > }
>> >
>> > +static void graphic_hw_update_bh(void *con)
>> > +{
>> > +graphic_hw_update(con);
>> > +}
>>
Re: [PATCH] console: make QMP screendump use coroutine
Hi
On Thu, Feb 20, 2020 at 8:49 AM Markus Armbruster wrote:
>
> Marc-André Lureau writes:
>
> > Thanks to the QMP coroutine support, the screendump handler can
> > trigger a graphic_hw_update(), yield and let the main loop run until
> > update is done. Then the handler is resumed, and the ppm_save() will
> > write the screen image to disk in the coroutine context (thus
> > non-blocking).
> >
> > For now, HMP doesn't have coroutine support, so it remains potentially
> > outdated or glitched.
> >
> > Fixes:
> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527
> >
> > Based-on: <20200109183545.27452-2-kw...@redhat.com>
> >
> > Cc: Kevin Wolf
> > Signed-off-by: Marc-André Lureau
> > ---
> > qapi/ui.json| 3 ++-
> > ui/console.c| 35 +++
> > ui/trace-events | 2 +-
> > 3 files changed, 30 insertions(+), 10 deletions(-)
> >
> > diff --git a/qapi/ui.json b/qapi/ui.json
> > index e04525d8b4..d941202f34 100644
> > --- a/qapi/ui.json
> > +++ b/qapi/ui.json
> > @@ -96,7 +96,8 @@
> > #
> > ##
> > { 'command': 'screendump',
> > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
> > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
> > + 'coroutine': true }
> >
> > ##
> > # == Spice
> > diff --git a/ui/console.c b/ui/console.c
> > index ac79d679f5..db184b473f 100644
> > --- a/ui/console.c
> > +++ b/ui/console.c
> > @@ -167,6 +167,7 @@ struct QemuConsole {
> > QEMUFIFO out_fifo;
> > uint8_t out_fifo_buf[16];
> > QEMUTimer *kbd_timer;
> > +Coroutine *screendump_co;
> >
> > QTAILQ_ENTRY(QemuConsole) next;
> > };
> > @@ -194,7 +195,6 @@ static void dpy_refresh(DisplayState *s);
> > static DisplayState *get_alloc_displaystate(void);
> > static void text_console_update_cursor_timer(void);
> > static void text_console_update_cursor(void *opaque);
> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
> >
> > static void gui_update(void *opaque)
> > {
> > @@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
> >
> > void graphic_hw_update_done(QemuConsole *con)
> > {
> > +if (con && con->screendump_co) {
>
> How can !con happen?
I don't think it can happen anymore (the patch evolved over several
years, this is probably a left-over). In any case, it doesn't hurt.
>
> > +aio_co_wake(con->screendump_co);
> > +}
> > }
> >
> > void graphic_hw_update(QemuConsole *con)
> > @@ -310,16 +313,16 @@ void graphic_hw_invalidate(QemuConsole *con)
> > }
> > }
> >
> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
> > +static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
> > {
> > -int width = pixman_image_get_width(ds->image);
> > -int height = pixman_image_get_height(ds->image);
> > +int width = pixman_image_get_width(image);
> > +int height = pixman_image_get_height(image);
> > g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
> > g_autofree char *header = NULL;
> > g_autoptr(pixman_image_t) linebuf = NULL;
> > int y;
> >
> > -trace_ppm_save(fd, ds);
> > +trace_ppm_save(fd, image);
> >
> > header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
> > @@ -329,7 +332,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error
> > **errp)
> >
> > linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
> > for (y = 0; y < height; y++) {
> > -qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
> > +qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
> >(char *)pixman_image_get_data(linebuf),
> >pixman_image_get_stride(linebuf), errp)
> > < 0) {
>
> Looks like an unrelated optimization / simplification. If I was
> maintainer, I'd ask for a separate patch.
I can be split, but it's related. We should pass a reference to
pixman_image_t, rather than a pointer to DisplaySurface, as the
underlying image may change over time, and would result in corrupted
coroutine save or worse.
> > @@ -340,11 +343,18 @@ static bool ppm_save(int fd, DisplaySurface *ds,
> > Error **errp)
> > return true;
> > }
> >
> > +static void graphic_hw_update_bh(void *con)
> > +{
> > +graphic_hw_update(con);
> > +}
> > +
> > +/* may be called in coroutine context or not */
>
> Hmm.
>
> Even though the QMP core always calls in coroutine context, the comment
> is correct: hmp_screendump() calls it outside coroutine context.
> Because of that...
>
> > 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
Re: [PATCH] console: make QMP screendump use coroutine
Marc-André Lureau writes:
> Thanks to the QMP coroutine support, the screendump handler can
> trigger a graphic_hw_update(), yield and let the main loop run until
> update is done. Then the handler is resumed, and the ppm_save() will
> write the screen image to disk in the coroutine context (thus
> non-blocking).
>
> For now, HMP doesn't have coroutine support, so it remains potentially
> outdated or glitched.
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>
> Based-on: <20200109183545.27452-2-kw...@redhat.com>
>
> Cc: Kevin Wolf
> Signed-off-by: Marc-André Lureau
> ---
> qapi/ui.json| 3 ++-
> ui/console.c| 35 +++
> ui/trace-events | 2 +-
> 3 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/qapi/ui.json b/qapi/ui.json
> index e04525d8b4..d941202f34 100644
> --- a/qapi/ui.json
> +++ b/qapi/ui.json
> @@ -96,7 +96,8 @@
> #
> ##
> { 'command': 'screendump',
> - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
> + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
> + 'coroutine': true }
>
> ##
> # == Spice
> diff --git a/ui/console.c b/ui/console.c
> index ac79d679f5..db184b473f 100644
> --- a/ui/console.c
> +++ b/ui/console.c
> @@ -167,6 +167,7 @@ struct QemuConsole {
> QEMUFIFO out_fifo;
> uint8_t out_fifo_buf[16];
> QEMUTimer *kbd_timer;
> +Coroutine *screendump_co;
>
> QTAILQ_ENTRY(QemuConsole) next;
> };
> @@ -194,7 +195,6 @@ static void dpy_refresh(DisplayState *s);
> static DisplayState *get_alloc_displaystate(void);
> static void text_console_update_cursor_timer(void);
> static void text_console_update_cursor(void *opaque);
> -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
>
> static void gui_update(void *opaque)
> {
> @@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
>
> void graphic_hw_update_done(QemuConsole *con)
> {
> +if (con && con->screendump_co) {
How can !con happen?
> +aio_co_wake(con->screendump_co);
> +}
> }
>
> void graphic_hw_update(QemuConsole *con)
> @@ -310,16 +313,16 @@ void graphic_hw_invalidate(QemuConsole *con)
> }
> }
>
> -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
> +static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
> {
> -int width = pixman_image_get_width(ds->image);
> -int height = pixman_image_get_height(ds->image);
> +int width = pixman_image_get_width(image);
> +int height = pixman_image_get_height(image);
> g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
> g_autofree char *header = NULL;
> g_autoptr(pixman_image_t) linebuf = NULL;
> int y;
>
> -trace_ppm_save(fd, ds);
> +trace_ppm_save(fd, image);
>
> header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
> if (qio_channel_write_all(QIO_CHANNEL(ioc),
> @@ -329,7 +332,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error
> **errp)
>
> linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
> for (y = 0; y < height; y++) {
> -qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
> +qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
> if (qio_channel_write_all(QIO_CHANNEL(ioc),
>(char *)pixman_image_get_data(linebuf),
>pixman_image_get_stride(linebuf), errp) <
> 0) {
Looks like an unrelated optimization / simplification. If I was
maintainer, I'd ask for a separate patch.
> @@ -340,11 +343,18 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error
> **errp)
> return true;
> }
>
> +static void graphic_hw_update_bh(void *con)
> +{
> +graphic_hw_update(con);
> +}
> +
> +/* may be called in coroutine context or not */
Hmm.
Even though the QMP core always calls in coroutine context, the comment
is correct: hmp_screendump() calls it outside coroutine context.
Because of that...
> 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);
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.
Your
Re: [PATCH] console: make QMP screendump use coroutine
Gerd Hoffmann writes: > Hi, > >> Thanks to the QMP coroutine support, the screendump handler can >> trigger a graphic_hw_update(), yield and let the main loop run until >> update is done. Then the handler is resumed, and the ppm_save() will >> write the screen image to disk in the coroutine context (thus >> non-blocking). >> >> For now, HMP doesn't have coroutine support, so it remains potentially >> outdated or glitched. >> >> Fixes: >> https://bugzilla.redhat.com/show_bug.cgi?id=1230527 >> >> Based-on: <20200109183545.27452-2-kw...@redhat.com> > > What is the status here? Tried to apply (worked) and build (failed), > seems Kevins patches are not merged yet? I reviewed v3, Kevin worked in improvements promptly, and I failed to review v4 promptly. Sorry about that.
Re: [PATCH] console: make QMP screendump use coroutine
Hi, > Thanks to the QMP coroutine support, the screendump handler can > trigger a graphic_hw_update(), yield and let the main loop run until > update is done. Then the handler is resumed, and the ppm_save() will > write the screen image to disk in the coroutine context (thus > non-blocking). > > For now, HMP doesn't have coroutine support, so it remains potentially > outdated or glitched. > > Fixes: > https://bugzilla.redhat.com/show_bug.cgi?id=1230527 > > Based-on: <20200109183545.27452-2-kw...@redhat.com> What is the status here? Tried to apply (worked) and build (failed), seems Kevins patches are not merged yet? thanks, Gerd
Re: [PATCH] console: make QMP screendump use coroutine
Patchew URL: https://patchew.org/QEMU/20200113144848.2168018-1-marcandre.lur...@redhat.com/ Hi, This series failed the docker-mingw@fedora build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #! /bin/bash export ARCH=x86_64 make docker-image-fedora V=1 NETWORK=1 time make docker-test-mingw@fedora J=14 NETWORK=1 === TEST SCRIPT END === /tmp/qemu-test/src/qapi/ui.json:98: command has unknown key 'coroutine' Valid keys are 'allow-oob', 'allow-preconfig', 'boxed', 'command', 'data', 'features', 'gen', 'if', 'returns', 'success-response'. CC /tmp/qemu-test/build/slirp/src/ncsi.o make: *** [Makefile:618: qapi-gen-timestamp] Error 1 make: *** Waiting for unfinished jobs CC /tmp/qemu-test/build/slirp/src/version.o CC /tmp/qemu-test/build/slirp/src/tcp_output.o --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=5bdb51421baf44d4a4d0ef4e0dc04458', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-y95ekbja/src/docker-src.2020-01-13-11.34.01.22073:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=5bdb51421baf44d4a4d0ef4e0dc04458 make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-y95ekbja/src' make: *** [docker-run-test-mingw@fedora] Error 2 real2m1.045s user0m8.011s The full log is available at http://patchew.org/logs/20200113144848.2168018-1-marcandre.lur...@redhat.com/testing.docker-mingw@fedora/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
Re: [PATCH] console: make QMP screendump use coroutine
Patchew URL: https://patchew.org/QEMU/20200113144848.2168018-1-marcandre.lur...@redhat.com/ Hi, This series failed the docker-quick@centos7 build test. Please find the testing commands and their output below. If you have Docker installed, you can probably reproduce it locally. === TEST SCRIPT BEGIN === #!/bin/bash make docker-image-centos7 V=1 NETWORK=1 time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1 === TEST SCRIPT END === Valid keys are 'allow-oob', 'allow-preconfig', 'boxed', 'command', 'data', 'features', 'gen', 'if', 'returns', 'success-response'. GEN hw/i386/trace.h GEN hw/i386/xen/trace.h make: *** [qapi-gen-timestamp] Error 1 make: *** Waiting for unfinished jobs Traceback (most recent call last): File "./tests/docker/docker.py", line 662, in --- raise CalledProcessError(retcode, cmd) subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=d3669b4e376a44e4a4d59206be32335f', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-_89wvl29/src/docker-src.2020-01-13-11.30.29.16733:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2. filter=--filter=label=com.qemu.instance.uuid=d3669b4e376a44e4a4d59206be32335f make[1]: *** [docker-run] Error 1 make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-_89wvl29/src' make: *** [docker-run-test-quick@centos7] Error 2 real2m20.446s user0m8.494s The full log is available at http://patchew.org/logs/20200113144848.2168018-1-marcandre.lur...@redhat.com/testing.docker-quick@centos7/?type=message. --- Email generated automatically by Patchew [https://patchew.org/]. Please send your feedback to patchew-de...@redhat.com
[PATCH] console: make QMP screendump use coroutine
Thanks to the QMP coroutine support, the screendump handler can
trigger a graphic_hw_update(), yield and let the main loop run until
update is done. Then the handler is resumed, and the ppm_save() will
write the screen image to disk in the coroutine context (thus
non-blocking).
For now, HMP doesn't have coroutine support, so it remains potentially
outdated or glitched.
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=1230527
Based-on: <20200109183545.27452-2-kw...@redhat.com>
Cc: Kevin Wolf
Signed-off-by: Marc-André Lureau
---
qapi/ui.json| 3 ++-
ui/console.c| 35 +++
ui/trace-events | 2 +-
3 files changed, 30 insertions(+), 10 deletions(-)
diff --git a/qapi/ui.json b/qapi/ui.json
index e04525d8b4..d941202f34 100644
--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -96,7 +96,8 @@
#
##
{ 'command': 'screendump',
- 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
+ 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
+ 'coroutine': true }
##
# == Spice
diff --git a/ui/console.c b/ui/console.c
index ac79d679f5..db184b473f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -167,6 +167,7 @@ struct QemuConsole {
QEMUFIFO out_fifo;
uint8_t out_fifo_buf[16];
QEMUTimer *kbd_timer;
+Coroutine *screendump_co;
QTAILQ_ENTRY(QemuConsole) next;
};
@@ -194,7 +195,6 @@ static void dpy_refresh(DisplayState *s);
static DisplayState *get_alloc_displaystate(void);
static void text_console_update_cursor_timer(void);
static void text_console_update_cursor(void *opaque);
-static bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
static void gui_update(void *opaque)
{
@@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
void graphic_hw_update_done(QemuConsole *con)
{
+if (con && con->screendump_co) {
+aio_co_wake(con->screendump_co);
+}
}
void graphic_hw_update(QemuConsole *con)
@@ -310,16 +313,16 @@ void graphic_hw_invalidate(QemuConsole *con)
}
}
-static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
+static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
{
-int width = pixman_image_get_width(ds->image);
-int height = pixman_image_get_height(ds->image);
+int width = pixman_image_get_width(image);
+int height = pixman_image_get_height(image);
g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
g_autofree char *header = NULL;
g_autoptr(pixman_image_t) linebuf = NULL;
int y;
-trace_ppm_save(fd, ds);
+trace_ppm_save(fd, image);
header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
if (qio_channel_write_all(QIO_CHANNEL(ioc),
@@ -329,7 +332,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error
**errp)
linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
for (y = 0; y < height; y++) {
-qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
+qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
if (qio_channel_write_all(QIO_CHANNEL(ioc),
(char *)pixman_image_get_data(linebuf),
pixman_image_get_stride(linebuf), errp) < 0)
{
@@ -340,11 +343,18 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error
**errp)
return true;
}
+static void graphic_hw_update_bh(void *con)
+{
+graphic_hw_update(con);
+}
+
+/* may be called in coroutine context or not */
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;
+}
+
surface = qemu_console_surface(con);
if (!surface) {
error_setg(errp, "no surface");
@@ -379,7 +397,8 @@ void qmp_screendump(const char *filename, bool has_device,
const char *device,
return;
}
-if (!ppm_save(fd, surface, errp)) {
+image = pixman_image_ref(surface->image);
+if (!ppm_save(fd, image, errp)) {
qemu_unlink(filename);
}
}
diff --git a/ui/trace-events b/ui/trace-events
index 0dcda393c1..e8726fc969 100644
--- a/ui/trace-events
+++ b/ui/trace-events
@@ -15,7 +15,7 @@ displaysurface_create_pixman(void *display_surface)
"surface=%p"
displaysurface_free(void *display_surface) "surface=%p"
displaychangelistener_register(void *dcl, const char *name) "%p [ %s ]"
