Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async

[Anthony Liguori <anthony@codemonkey.ws>]

On 03/06/2012 09:56 AM, Alon Levy wrote:
> On Tue, Mar 06, 2012 at 07:51:29AM -0600, Anthony Liguori wrote:
>> On 03/06/2012 07:16 AM, Alon Levy wrote:
>>> On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote:
>>>> On Tue, 06 Mar 2012 08:36:34 +0100
>>>> Gerd Hoffmann<kraxel@redhat.com>   wrote:
>>>>
>>>>>    Hi,
>>>>>
>>>>>>> How would the parallel execution facility be opaque to the implementer?
>>>>>>> screendump returns, screendump_async needs to pass a closure. You can
>>>>>>> automatically generate any amount of code, but you can only have a
>>>>>>> single function implementation with longjmp/coroutine, or having a
>>>>>>> saparate thread per command but that would mean taking locks for
>>>>>>> anything not trivial, which avoids the no-change-to-implementation. Is
>>>>>>> this what you have in mind?
>>>>>>
>>>>>> It would not be opaque to the implementer.  But it would avoid
>>>>>> introducing new commands and events, instead we have a unified mechanism
>>>>>> to signal completion.
>>>>>
>>>>> Ok.  We have a async mechanism today: .mhandler.cmd_async = ...
>>>>>
>>>>> I know it has its problems like no cancelation and is deprecated and
>>>>> all.  But still: how about using it as interim until QAPI-based async
>>>>> monitor support is ready?  We could unbreak qxl screendumps without
>>>>> having to introduce a new (but temporary!) screendump_async command +
>>>>> completion event.
>>>>
>>>> There are a few problems here, but the blocking one is that a command
>>>> can't go from sync to async. This is an incompatible change.
>>>>
>>>> If you mind adding the temporary command and if this issue is so rare
>>>> that none can reproduce it, then I'd suggest to wait for 1.2.
>>>>
>>>
>>> There are two options really:
>>>   1. revert the patches that changed qxl screendump to save the ppm
>>>   before (possibly) updating the framebuffer.
>>>   2. introduce a new command that is really async
>>>
>>>   The third option, what Gerd proposes, doesn't break the blocking chain
>>>   going from the A, the dual purpose spice client and libvirt client,
>>>   through libvirt, qemu, spice and back to A.
>>>
>>> If no one can reproduce the block then it would seem 1 makes sense.
>>
>> So let's start with a reproducible test case that demonstrates the
>> problem before we start introducing new commands then if there's
>> doubt about the nature of the problem.
>
> s/the problem/different problem/:
>
>   NB: To get this hang I had to disable update_area's from the guest, just
>       because otherwise there is a very small window between suspending the
>       client and qemu waiting on the same stack trace below issued from the
>       guest update_area io out, instead of from the screendump monitor command.
>
>   spice client, qemu, libvirt, virsh screenshot.
>
>   libvirt controlled qemu with qxl device and spice connection.
>   qemu ... -vga qxl -spice disable-ticketing,port=5900...
>   spicec -h localhost -p 5900
>   [boot until qxl driver is loaded, and guest is doing something (Running
>   toms 2d benchmark works), suspend spicec]
>   virsh screenshot<vm-name>  /tmp/dump.ppm
>
> virsh will hang at:
>   #1 virCondWait
>   #2 qemuMonitorSend
>   #3 qemuMonitorJSONCommandWithFd
>   #4 qemuMonitorJSONCommand
>   #5 qemuMonitorJSONScreendump
>
> qemu is hung at:
>   main thread:
>    #0 read

Is qxl doing a blocking read?  If so, that's a bug in qxl.  You cannot do a 
blocking read while holding qemu_mutex.

Regards,

Anthony Liguori