Re: [PATCH 3/3] console: make QMP/HMP screendump run in coroutine

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Tue, Oct 27, 2020 at 12:41 PM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> marcandre.lureau@redhat.com writes:
>>
>> > From: Marc-André Lureau <marcandre.lureau@redhat.com>
>> >
>> > Thanks to the monitors coroutine support, the screendump handler can
>>
>> monitors'
>>
>> Suggest to add (merge commit b7092cda1b3) right before the comma.
>>
>> > trigger a graphic_hw_update(), yield and let the main loop run until
>> > update is done. Then the handler is resumed, and ppm_save() will write
>> > the screen image to disk in the coroutine context (thus non-blocking).
>> >
>> > Potentially, during non-blocking write, some new graphic update could
>> > happen, and thus the image may have some glitches. Whether that
>> > behaviour is acceptable is discutable. Allocating new memory may not be
>>
>> s/discutable/debatable/
>>
>> > a good idea, as framebuffers can be quite large. Even then, QEMU may
>> > become less responsive as it requires paging in etc.
>>
>> Tradeoff.  I'm okay with "simple & efficient, but might glitch".  It
>> should be documented, though.  Followup patch is fine.
>>
>> > Related to:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>> >
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
[...]
>> Let's revisit the experiment I did for v1: "observe the main loop keeps
>> running while the screendump does its work".
>>
>> Message-ID: <87a74ueudt.fsf@dusky.pond.sub.org>
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-03/msg01235.html
>>
>> I repeated the first experiment "does the main loop continue to run when
>> writing out the screendump blocks / would block?"  Same result: main
>> loop remains blocked.
>>
>> Back then, you replied
>>
>>     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.
>>
>> I'm unsure what exactly the update is.  Is it the change from
>>
>>     Fixes:
>>     https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>>
>> to
>>
>>     Related to:
>>     https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>>
>> ?
>
> Right, qmp_screendump() calls qemu_open_old(filename, O_WRONLY |
> O_CREAT | O_TRUNC | O_BINARY, 0666), so opened in blocking mode.
>
> So simply opening a FS file with O_NONBLOCK or calling
> qemu_try_set_nonblock(fd) with the resulting fd doesn't really help to
> check it's async (unless I am missing a trick to slow down disk IO
> somehow...?)
>
> It's annoying that it also does O_TRUNC: even if you pass a
> socket/pipe to add-fd, it will fail to ftruncate() (via the
> "/dev/fdset" path). Furthermore, access mode check seems kinda
> incomplete. Afaict, in monitor_fdset_dup_fd_add(), F_GETFL may return
> O_RDWR which is different than O_RDONLY or O_WRONLY, yet should be
> considered compatible for the caller I think..
>
> With some little hacks though, I could check passing a pipe does
> indeed make PPM save async, and the main loop is reentered. I don't
> know if it's enough to convince you it's not really the problem of
> this code change though. We get coroutine IO by accident here, I think
> we already said that.

I'm okay with this patch "only" getting us closer to the goal of having
screendump not block the main loop.  I just want the commit message to
be clear on how close.

>> The commit message says "ppm_save() will write the screen image to disk
>> in the coroutine context (thus non-blocking)."  Sure reads like a claim
>> the main loop is no longer blocked.  It is blocked, at least for me.
>> Please clarify.
>
> Let's clarify it by saying it's still blocking then, and tackle that
> in a future change.

Works for me!

>> Back then, I proposed a second experiment: "does the main loop continue
>> to run while we wait for graphic_hw_update_done()?"  I don't know the
>> result.  Do you?
>>
>> The commit message claims "the screendump handler can trigger a
>> graphic_hw_update(), yield and let the main loop run until update is
>> done."
>
> Isn't it clearly the case with the BH being triggered after main loop?

Yes, but have you *tested* the main loop keeps running?