qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Alon Levy
Subject: Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
Date: Tue, 6 Mar 2012 18:02:13 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

On Tue, Mar 06, 2012 at 05:56:49PM +0200, 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<address@hidden>  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.

Explanation to the note: 'disable update_area' - I meant disable the
implementation, i.e. change the ioport_write switch to return without
actually rendering anything. It causes artifacts of course, but if you
know how the vm looks by heart it's good enough to reproduce. Nothing
changed in the guest.

> 
>  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
>   #1 read_safe
>   #2 dispatcher_send_message
>   #3 red_dispatcher_update_area
>   #4 qxl_worker_update_area
>   #5 qxl_spice_update_area
>   #6 qxl_render_update
>   #7 qxl_hw_screen_dump
>   #8 vga_hw_screen_dump
> 
>  spice worker thread:
>   #0 nanosleep
>   #1 usleep
>   #2 flush_display_commands
>   #3 handle_dev_update
>   #4 dispatcher_handle_single_read
>   #5 dispatcher_handle_recv_read
>   #6 handle_dev_input
>   #7 red_worker_main
>   #8 start_thread
>   #9 clone
> 
> So the two problems here are:
>  virsh screenshot is hung if the client is hung / not responsive
>  qemu guest thread will hang the first time it does a vmexit if the
>  client is hung / not responsive.
> 
> Like Gerd mentioned previously, this issue should be addressed in spice
> but is hard to correct there, hence the async io patches that landed
> long ago, and hence the async update_area done for screendump as well
> that we are discussing reverting.
> 
> So if I have you convinced it should not be reverted, we are back to
> needing a asynchronous command for screendump. And Luiz said changing
> the sync command to async is not an option (though I didn't understand
> why), so we are left with a new, temporary, command.
> 
> Any objections?
> > 
> > Regards,
> > 
> > Anthony Liguori
> > 
> > >
> > >But 1 serves a second purpose, which is to allow the guest to do io
> > >exits while the server thread is processing the area update request
> > >issued for the screendump. So it makes sense regardless.
> > >
> > >=>  Option 2.
> > 
> > 
> 



reply via email to

[Prev in Thread] Current Thread [Next in Thread]