Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
Hi, >> qemu is hung at: >> main thread: >>#0 read > > Is qxl doing a blocking read? If so, that's a bug in qxl. It used to do that, with the latest spice pull it is gone[1]. And this fix is exactly what broke screendump. Spice does lazy rendering on the server side to avoid burning cpu for nothing, because in the common case there is nothing to render. Thats why there is no up-to-date displaysurface we can just write out when the screendump command comes in. What qxl screendump used to do is this: (1) ask spice server to render the screen (2) blocking read, waiting for spice spice server finish<- BUG (3) write out screendump What qxl screendump does now is: (1) ask spice server to render the screen (2) write out screendump from outdated displaysurface <- BUG (3) spice server finished, callback comes in, arm BH (4) bottom half handler updates displaysurface What we like to do instead is this: (1) ask spice server to render the screen (2) spice server finished, callback comes in, arm BH (3) bottom half handler updates displaysurface and writes the screendump (4) signal screendump monitor command is finished. See our problem now? cheers, Gerd [1] With the exception of guest with old qxl drivers which is pretty much unfixable due to way the old guest <-> qxl interface is designed.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/06/2012 10:26 AM, Alon Levy wrote: On Tue, Mar 06, 2012 at 10:16:42AM -0600, Anthony Liguori wrote: 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 Hoffmannwrote: 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 /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. What are you saying, that that read should be fixed? then I guess it's good that patches fixing it have landed. That stack was prior to "qxl: make qxl_render_update async", 81fb6f1504fb9ef71f2382f44af34756668296e8 I'm sorry, I'm thoroughly confused by this thread. Can you start a new thread with a clear explanation of the problem you're trying to solve or at least point me to an existing note? Regards, Anthony Liguori . Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, Mar 06, 2012 at 10:16:42AM -0600, Anthony Liguori wrote: > 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 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 /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. What are you saying, that that read should be fixed? then I guess it's good that patches fixing it have landed. That stack was prior to "qxl: make qxl_render_update async", 81fb6f1504fb9ef71f2382f44af34756668296e8 . > > Regards, > > Anthony Liguori >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
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 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 /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
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
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 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 /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 se
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
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 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 /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. > >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/06/2012 08:23 AM, Alon Levy wrote: On Tue, Mar 06, 2012 at 10:53:42AM -0300, Luiz Capitulino wrote: So cutting off a part of the email is a good way to win arguments? cool trick. It doesn't work as well if you acknowledge that was the motivation ;-) (j/k) I agree a reproducer is a good idea, but as I mentioned in the cut part, doing the update area without keeping the monitor waiting improves performance of the guest by letting it do io exits. Why is that a bad thing? You say, "improves performance", but can you quantify that? And why is screendump on the performance critical path in the first place? Having a small test case that demonstrates the problem (be it functional or performance) will help ground this discussion in concrete terms. I think we need to step back and reexamine what the problem we're trying to solve is. Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, Mar 06, 2012 at 10:53:42AM -0300, Luiz Capitulino wrote: > On Tue, 06 Mar 2012 07:51:29 -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 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. > > Completely agree, I was going to suggest that too. > So cutting off a part of the email is a good way to win arguments? cool trick. I agree a reproducer is a good idea, but as I mentioned in the cut part, doing the update area without keeping the monitor waiting improves performance of the guest by letting it do io exits. Why is that a bad thing?
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, 06 Mar 2012 07:51:29 -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 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. Completely agree, I was going to suggest that too.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
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 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. 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.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, Mar 06, 2012 at 09:24:27AM -0300, Luiz Capitulino wrote: > On Tue, 06 Mar 2012 08:36:34 +0100 > Gerd Hoffmann 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. 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.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, 06 Mar 2012 08:36:34 +0100 Gerd Hoffmann 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.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, Mar 06, 2012 at 09:10:00AM +0100, Gerd Hoffmann wrote: > On 03/06/12 08:56, Alon Levy wrote: > > On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann 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. > > > > Actually, I'm not sure this doesn't reintroduce the original problem > > (which I haven't been able to reproduce): > > > > client: screenshot <-> client libvirt <-> host libvirt > > > > host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client > > Hmm? spice client can ask for a screendump via libvirt? > > /me looks completely puzzled. No, ok, bad attempt. "client" is a libvirt and spice client. client as a libvirt client asks for screenshot. libvirt (client side) asks from libvirt (host side) libvirt (host side) issues a screendump monitor command (the new internal async version) qxl_screen_dump asks spice server to render spice server waits for pipe to spice client to empty (lower then 50) but spice client, which is just "client", is blocking on completion of the screendump command. I have two problems with my own explanation: 1. I didn't manage to reproduce it myself, and nor has Marc Andre (who first reported this problem) yesterday. 2. I remember that the async monitor command I sent in October did fix the problem, so something with my description is wrong. > > cheers, > Gerd > >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/06/12 08:56, Alon Levy wrote: > On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann 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. > > Actually, I'm not sure this doesn't reintroduce the original problem > (which I haven't been able to reproduce): > > client: screenshot <-> client libvirt <-> host libvirt > > host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client Hmm? spice client can ask for a screendump via libvirt? /me looks completely puzzled. cheers, Gerd
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann 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. Actually, I'm not sure this doesn't reintroduce the original problem (which I haven't been able to reproduce): client: screenshot <-> client libvirt <-> host libvirt host libvirt (screendump) <-> qemu monitor -> <- spice server <-> client > > cheers, > Gerd >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Tue, Mar 06, 2012 at 08:36:34AM +0100, Gerd Hoffmann 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. With the added benefit of no libvirt changes for the temporary solution. > > cheers, > Gerd >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
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. cheers, Gerd
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Mon, 5 Mar 2012 20:58:08 +0200 Alon Levy wrote: > On Mon, Mar 05, 2012 at 08:17:52PM +0200, Avi Kivity wrote: > > On 03/05/2012 08:09 PM, Alon Levy wrote: > > > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > > > > On Mon, 05 Mar 2012 11:27:07 -0600 > > > > Anthony Liguori wrote: > > > > > > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > > > >> > > > > > >> > > > > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > > > > >> It should mean execute this closure when you finish (function > > > > > >> pointer > > > > > >> + opaque). > > > > > >> > > > > > >> The QMP event should be dispatched from the closure such that the > > > > > >> screendump code doesn't have to have a direct dependency on QMP. > > > > > >> > > > > > > > > > > > > What about using the parallel execution facility of qmp? It's > > > > > > silly to > > > > > > duplicate every command X with X-async and X-COMPLETED. > > > > > > 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. > > Yeah, that sounds good. Let's have that for 6.4. That's too far away, qemu will have be re-written a dozen of times when we get there. But if you meant 1.2, yes, I hope to have async support for 1.2. But I must admit that I'm having some trouble delivering things on time...
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 12:22 PM, Avi Kivity wrote: On 03/05/2012 08:16 PM, Anthony Liguori wrote: We're pretty close to being there. Luiz, about how long do you think before we get there? It's a pity to add new commands along the way. It's more complicated than this unfortunately. A client needs to be able to determine whether the 'screendump' command works as expected. Unfortunately, when QXL was introduced, 'screendump' became broken across multiple releases. screendump is the right interface, but since it was broken in multiple releases, we need another command for a client to determine that it's not broken. In the short term, screendump_async is that. After QAPI, it will probably be a screendump2. I don't mind introducing short term commands and then deprecating it particularly when they are marked as such. Zooming out from screendump, there are several ways to deal with broken commands: 1. introduce new commands This is our only short term options for this specific circumstance. 2. introduce capabilities that say "screendump is fixed wrt bug so-and-so" We don't have such a mechanism and I generally would prefer to avoid it. There's a certain shame in introducing new commands that hopefully will cause us to be more careful in the future. 3. fix it and backport the fix to a stable release This is only not practical because it requires such an infrastructure change. 4. ignore the broken command and hope 5. just tell clients to rely on version information and ignore the existence of downstreams This is really mostly a downstream problem, not an upstream problem. Version numbers can be reliable here for upstream. This is the approach that would be typically taken for a C library. The flip side is that this is also while we end up with autotools and a bunch of compile time tests to determine what broken functions exist. Regards, Anthony Liguori My preference is 3->2->1->4, or we'll end up with screendump65535 soon.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Mon, Mar 05, 2012 at 08:17:52PM +0200, Avi Kivity wrote: > On 03/05/2012 08:09 PM, Alon Levy wrote: > > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > > > On Mon, 05 Mar 2012 11:27:07 -0600 > > > Anthony Liguori wrote: > > > > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > > >> > > > > >> > > > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > > > >> It should mean execute this closure when you finish (function pointer > > > > >> + opaque). > > > > >> > > > > >> The QMP event should be dispatched from the closure such that the > > > > >> screendump code doesn't have to have a direct dependency on QMP. > > > > >> > > > > > > > > > > What about using the parallel execution facility of qmp? It's silly > > > > > to > > > > > duplicate every command X with X-async and X-COMPLETED. > > > > 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. Yeah, that sounds good. Let's have that for 6.4. > > -- > error compiling committee.c: too many arguments to function > >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 08:16 PM, Anthony Liguori wrote: > >> >>>We're pretty close to being there. Luiz, about how long do you >>> think before we get there? >> >> It's a pity to add new commands along the way. > > It's more complicated than this unfortunately. > > A client needs to be able to determine whether the 'screendump' > command works as expected. Unfortunately, when QXL was introduced, > 'screendump' became broken across multiple releases. > > screendump is the right interface, but since it was broken in multiple > releases, we need another command for a client to determine that it's > not broken. In the short term, screendump_async is that. After QAPI, > it will probably be a screendump2. > > I don't mind introducing short term commands and then deprecating it > particularly when they are marked as such. Zooming out from screendump, there are several ways to deal with broken commands: 1. introduce new commands 2. introduce capabilities that say "screendump is fixed wrt bug so-and-so" 3. fix it and backport the fix to a stable release 4. ignore the broken command and hope My preference is 3->2->1->4, or we'll end up with screendump65535 soon. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 08:09 PM, Alon Levy wrote: > On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > > On Mon, 05 Mar 2012 11:27:07 -0600 > > Anthony Liguori wrote: > > > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > >> > > > >> > > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > > >> It should mean execute this closure when you finish (function pointer > > > >> + opaque). > > > >> > > > >> The QMP event should be dispatched from the closure such that the > > > >> screendump code doesn't have to have a direct dependency on QMP. > > > >> > > > > > > > > What about using the parallel execution facility of qmp? It's silly to > > > > duplicate every command X with X-async and X-COMPLETED. > > 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. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 11:29 AM, Avi Kivity wrote: On 03/05/2012 07:27 PM, Anthony Liguori wrote: On 03/05/2012 11:20 AM, Avi Kivity wrote: On 03/05/2012 04:33 PM, Anthony Liguori wrote: async in QEMU doesn't mean "generate a QMP event when you're done". It should mean execute this closure when you finish (function pointer + opaque). The QMP event should be dispatched from the closure such that the screendump code doesn't have to have a direct dependency on QMP. What about using the parallel execution facility of qmp? It's silly to duplicate every command X with X-async and X-COMPLETED. We need to switch over to QAPI to get there. Just an implementation detail, yes? No spec/protocol changes? Correct. We're pretty close to being there. Luiz, about how long do you think before we get there? It's a pity to add new commands along the way. It's more complicated than this unfortunately. A client needs to be able to determine whether the 'screendump' command works as expected. Unfortunately, when QXL was introduced, 'screendump' became broken across multiple releases. screendump is the right interface, but since it was broken in multiple releases, we need another command for a client to determine that it's not broken. In the short term, screendump_async is that. After QAPI, it will probably be a screendump2. I don't mind introducing short term commands and then deprecating it particularly when they are marked as such. Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Mon, Mar 05, 2012 at 02:31:42PM -0300, Luiz Capitulino wrote: > On Mon, 05 Mar 2012 11:27:07 -0600 > Anthony Liguori wrote: > > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > >> > > >> > > >> async in QEMU doesn't mean "generate a QMP event when you're done". > > >> It should mean execute this closure when you finish (function pointer > > >> + opaque). > > >> > > >> The QMP event should be dispatched from the closure such that the > > >> screendump code doesn't have to have a direct dependency on QMP. > > >> > > > > > > What about using the parallel execution facility of qmp? It's silly to > > > duplicate every command X with X-async and X-COMPLETED. 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? > > > > We need to switch over to QAPI to get there. We're pretty close to being > > there. > > Luiz, about how long do you think before we get there? > > It's a bit hard to tell, because it depends on upstream review plus me not > being bogged down with other stuff. But maybe in around two weeks when I > resume my work on this. >
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Mon, 05 Mar 2012 19:29:14 +0200 Avi Kivity wrote: > On 03/05/2012 07:27 PM, Anthony Liguori wrote: > > On 03/05/2012 11:20 AM, Avi Kivity wrote: > >> On 03/05/2012 04:33 PM, Anthony Liguori wrote: > >>> > >>> > >>> async in QEMU doesn't mean "generate a QMP event when you're done". > >>> It should mean execute this closure when you finish (function pointer > >>> + opaque). > >>> > >>> The QMP event should be dispatched from the closure such that the > >>> screendump code doesn't have to have a direct dependency on QMP. > >>> > >> > >> What about using the parallel execution facility of qmp? It's silly to > >> duplicate every command X with X-async and X-COMPLETED. > > > > We need to switch over to QAPI to get there. > > Just an implementation detail, yes? No spec/protocol changes? We haven't discussed it yet how to do async commands in detail, so it may or may not have protocol changes. I have a simple proposal in mind, but haven't submitted it yet. > > We're pretty close to being there. Luiz, about how long do you > > think before we get there? > > It's a pity to add new commands along the way. We decided not to block useful features because of the long time that's taking to do async support properly.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Mon, 05 Mar 2012 11:27:07 -0600 Anthony Liguori wrote: > On 03/05/2012 11:20 AM, Avi Kivity wrote: > > On 03/05/2012 04:33 PM, Anthony Liguori wrote: > >> > >> > >> async in QEMU doesn't mean "generate a QMP event when you're done". > >> It should mean execute this closure when you finish (function pointer > >> + opaque). > >> > >> The QMP event should be dispatched from the closure such that the > >> screendump code doesn't have to have a direct dependency on QMP. > >> > > > > What about using the parallel execution facility of qmp? It's silly to > > duplicate every command X with X-async and X-COMPLETED. > > We need to switch over to QAPI to get there. We're pretty close to being > there. > Luiz, about how long do you think before we get there? It's a bit hard to tell, because it depends on upstream review plus me not being bogged down with other stuff. But maybe in around two weeks when I resume my work on this.
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 07:27 PM, Anthony Liguori wrote: > On 03/05/2012 11:20 AM, Avi Kivity wrote: >> On 03/05/2012 04:33 PM, Anthony Liguori wrote: >>> >>> >>> async in QEMU doesn't mean "generate a QMP event when you're done". >>> It should mean execute this closure when you finish (function pointer >>> + opaque). >>> >>> The QMP event should be dispatched from the closure such that the >>> screendump code doesn't have to have a direct dependency on QMP. >>> >> >> What about using the parallel execution facility of qmp? It's silly to >> duplicate every command X with X-async and X-COMPLETED. > > We need to switch over to QAPI to get there. Just an implementation detail, yes? No spec/protocol changes? > We're pretty close to being there. Luiz, about how long do you > think before we get there? It's a pity to add new commands along the way. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 11:20 AM, Avi Kivity wrote: On 03/05/2012 04:33 PM, Anthony Liguori wrote: async in QEMU doesn't mean "generate a QMP event when you're done". It should mean execute this closure when you finish (function pointer + opaque). The QMP event should be dispatched from the closure such that the screendump code doesn't have to have a direct dependency on QMP. What about using the parallel execution facility of qmp? It's silly to duplicate every command X with X-async and X-COMPLETED. We need to switch over to QAPI to get there. We're pretty close to being there. Luiz, about how long do you think before we get there? Regards, Anthony Liguori
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 04:33 PM, Anthony Liguori wrote: > > > async in QEMU doesn't mean "generate a QMP event when you're done". > It should mean execute this closure when you finish (function pointer > + opaque). > > The QMP event should be dispatched from the closure such that the > screendump code doesn't have to have a direct dependency on QMP. > What about using the parallel execution facility of qmp? It's silly to duplicate every command X with X-async and X-COMPLETED. -- error compiling committee.c: too many arguments to function
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On Mon, Mar 05, 2012 at 08:33:06AM -0600, Anthony Liguori wrote:
> On 03/05/2012 08:16 AM, Alon Levy wrote:
> >adds a handler for the following qmp screendump-async command.
> >
> >graphics_console_init signature change required touching every user, but
> >no implementation of the new vga_hw_screen_dump_async_ptr is provided
> >in this patch.
> >
> >Signed-off-by: Alon Levy
> >---
> > console.c|4
> > console.h|5 +
> > hw/blizzard.c|2 +-
> > hw/cirrus_vga.c |4 ++--
> > hw/exynos4210_fimd.c |3 ++-
> > hw/g364fb.c |2 +-
> > hw/jazz_led.c|3 ++-
> > hw/milkymist-vgafb.c |2 +-
> > hw/musicpal.c|2 +-
> > hw/omap_dss.c|4 +++-
> > hw/omap_lcdc.c |2 +-
> > hw/pl110.c |2 +-
> > hw/pxa2xx_lcd.c |2 +-
> > hw/qxl.c |3 ++-
> > hw/sm501.c |4 ++--
> > hw/ssd0303.c |2 +-
> > hw/ssd0323.c |2 +-
> > hw/tc6393xb.c|1 +
> > hw/tcx.c |4 ++--
> > hw/vga-isa-mm.c |3 ++-
> > hw/vga-isa.c |3 ++-
> > hw/vga-pci.c |3 ++-
> > hw/vga_int.h |1 +
> > hw/vmware_vga.c |3 ++-
> > hw/xenfb.c |2 ++
> > 25 files changed, 45 insertions(+), 23 deletions(-)
> >
> >diff --git a/console.c b/console.c
> >index 25d8acb..9a49e93 100644
> >--- a/console.c
> >+++ b/console.c
> >@@ -124,6 +124,7 @@ struct TextConsole {
> > vga_hw_update_ptr hw_update;
> > vga_hw_invalidate_ptr hw_invalidate;
> > vga_hw_screen_dump_ptr hw_screen_dump;
> >+vga_hw_screen_dump_async_ptr hw_screen_dump_async;
> > vga_hw_text_update_ptr hw_text_update;
> > void *hw;
> >
> >@@ -1403,6 +1404,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr
> >update,
> > vga_hw_invalidate_ptr invalidate,
> > vga_hw_screen_dump_ptr screen_dump,
> > vga_hw_text_update_ptr text_update,
> >+ vga_hw_screen_dump_async_ptr
> >+screen_dump_async,
> > void *opaque)
> > {
> > TextConsole *s;
> >@@ -1421,6 +1424,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr
> >update,
> > s->hw_update = update;
> > s->hw_invalidate = invalidate;
> > s->hw_screen_dump = screen_dump;
> >+s->hw_screen_dump_async = screen_dump_async;
> > s->hw_text_update = text_update;
> > s->hw = opaque;
> >
> >diff --git a/console.h b/console.h
> >index c22803c..3cf28c0 100644
> >--- a/console.h
> >+++ b/console.h
> >@@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t
> >*dest, uint32_t ch)
> > typedef void (*vga_hw_update_ptr)(void *);
> > typedef void (*vga_hw_invalidate_ptr)(void *);
> > typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
> >+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
> >+ bool cswitch);
> > typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
> >
> > DisplayState *graphic_console_init(vga_hw_update_ptr update,
> > vga_hw_invalidate_ptr invalidate,
> > vga_hw_screen_dump_ptr screen_dump,
> > vga_hw_text_update_ptr text_update,
> >+ vga_hw_screen_dump_async_ptr
> >+screen_dump_async,
> > void *opaque);
>
>
> async in QEMU doesn't mean "generate a QMP event when you're done".
> It should mean execute this closure when you finish (function
> pointer + opaque).
>
> The QMP event should be dispatched from the closure such that the
> screendump code doesn't have to have a direct dependency on QMP.
Yes, I can add a closure (I could put the filename there, that's the
only thing we need to have there right now).
Note that there is already a function wrapping the specific QEVENT used.
>
> Regards,
>
> Anthony Liguori
Re: [Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
On 03/05/2012 08:16 AM, Alon Levy wrote:
adds a handler for the following qmp screendump-async command.
graphics_console_init signature change required touching every user, but
no implementation of the new vga_hw_screen_dump_async_ptr is provided
in this patch.
Signed-off-by: Alon Levy
---
console.c|4
console.h|5 +
hw/blizzard.c|2 +-
hw/cirrus_vga.c |4 ++--
hw/exynos4210_fimd.c |3 ++-
hw/g364fb.c |2 +-
hw/jazz_led.c|3 ++-
hw/milkymist-vgafb.c |2 +-
hw/musicpal.c|2 +-
hw/omap_dss.c|4 +++-
hw/omap_lcdc.c |2 +-
hw/pl110.c |2 +-
hw/pxa2xx_lcd.c |2 +-
hw/qxl.c |3 ++-
hw/sm501.c |4 ++--
hw/ssd0303.c |2 +-
hw/ssd0323.c |2 +-
hw/tc6393xb.c|1 +
hw/tcx.c |4 ++--
hw/vga-isa-mm.c |3 ++-
hw/vga-isa.c |3 ++-
hw/vga-pci.c |3 ++-
hw/vga_int.h |1 +
hw/vmware_vga.c |3 ++-
hw/xenfb.c |2 ++
25 files changed, 45 insertions(+), 23 deletions(-)
diff --git a/console.c b/console.c
index 25d8acb..9a49e93 100644
--- a/console.c
+++ b/console.c
@@ -124,6 +124,7 @@ struct TextConsole {
vga_hw_update_ptr hw_update;
vga_hw_invalidate_ptr hw_invalidate;
vga_hw_screen_dump_ptr hw_screen_dump;
+vga_hw_screen_dump_async_ptr hw_screen_dump_async;
vga_hw_text_update_ptr hw_text_update;
void *hw;
@@ -1403,6 +1404,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr
update,
vga_hw_invalidate_ptr invalidate,
vga_hw_screen_dump_ptr screen_dump,
vga_hw_text_update_ptr text_update,
+ vga_hw_screen_dump_async_ptr
+screen_dump_async,
void *opaque)
{
TextConsole *s;
@@ -1421,6 +1424,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr
update,
s->hw_update = update;
s->hw_invalidate = invalidate;
s->hw_screen_dump = screen_dump;
+s->hw_screen_dump_async = screen_dump_async;
s->hw_text_update = text_update;
s->hw = opaque;
diff --git a/console.h b/console.h
index c22803c..3cf28c0 100644
--- a/console.h
+++ b/console.h
@@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest,
uint32_t ch)
typedef void (*vga_hw_update_ptr)(void *);
typedef void (*vga_hw_invalidate_ptr)(void *);
typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
+ bool cswitch);
typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
DisplayState *graphic_console_init(vga_hw_update_ptr update,
vga_hw_invalidate_ptr invalidate,
vga_hw_screen_dump_ptr screen_dump,
vga_hw_text_update_ptr text_update,
+ vga_hw_screen_dump_async_ptr
+screen_dump_async,
void *opaque);
async in QEMU doesn't mean "generate a QMP event when you're done". It should
mean execute this closure when you finish (function pointer + opaque).
The QMP event should be dispatched from the closure such that the screendump
code doesn't have to have a direct dependency on QMP.
Regards,
Anthony Liguori
[Qemu-devel] [PATCH v2 1/2] console: add hw_screen_dump_async
adds a handler for the following qmp screendump-async command.
graphics_console_init signature change required touching every user, but
no implementation of the new vga_hw_screen_dump_async_ptr is provided
in this patch.
Signed-off-by: Alon Levy
---
console.c|4
console.h|5 +
hw/blizzard.c|2 +-
hw/cirrus_vga.c |4 ++--
hw/exynos4210_fimd.c |3 ++-
hw/g364fb.c |2 +-
hw/jazz_led.c|3 ++-
hw/milkymist-vgafb.c |2 +-
hw/musicpal.c|2 +-
hw/omap_dss.c|4 +++-
hw/omap_lcdc.c |2 +-
hw/pl110.c |2 +-
hw/pxa2xx_lcd.c |2 +-
hw/qxl.c |3 ++-
hw/sm501.c |4 ++--
hw/ssd0303.c |2 +-
hw/ssd0323.c |2 +-
hw/tc6393xb.c|1 +
hw/tcx.c |4 ++--
hw/vga-isa-mm.c |3 ++-
hw/vga-isa.c |3 ++-
hw/vga-pci.c |3 ++-
hw/vga_int.h |1 +
hw/vmware_vga.c |3 ++-
hw/xenfb.c |2 ++
25 files changed, 45 insertions(+), 23 deletions(-)
diff --git a/console.c b/console.c
index 25d8acb..9a49e93 100644
--- a/console.c
+++ b/console.c
@@ -124,6 +124,7 @@ struct TextConsole {
vga_hw_update_ptr hw_update;
vga_hw_invalidate_ptr hw_invalidate;
vga_hw_screen_dump_ptr hw_screen_dump;
+vga_hw_screen_dump_async_ptr hw_screen_dump_async;
vga_hw_text_update_ptr hw_text_update;
void *hw;
@@ -1403,6 +1404,8 @@ DisplayState *graphic_console_init(vga_hw_update_ptr
update,
vga_hw_invalidate_ptr invalidate,
vga_hw_screen_dump_ptr screen_dump,
vga_hw_text_update_ptr text_update,
+ vga_hw_screen_dump_async_ptr
+screen_dump_async,
void *opaque)
{
TextConsole *s;
@@ -1421,6 +1424,7 @@ DisplayState *graphic_console_init(vga_hw_update_ptr
update,
s->hw_update = update;
s->hw_invalidate = invalidate;
s->hw_screen_dump = screen_dump;
+s->hw_screen_dump_async = screen_dump_async;
s->hw_text_update = text_update;
s->hw = opaque;
diff --git a/console.h b/console.h
index c22803c..3cf28c0 100644
--- a/console.h
+++ b/console.h
@@ -341,17 +341,22 @@ static inline void console_write_ch(console_ch_t *dest,
uint32_t ch)
typedef void (*vga_hw_update_ptr)(void *);
typedef void (*vga_hw_invalidate_ptr)(void *);
typedef void (*vga_hw_screen_dump_ptr)(void *, const char *, bool cswitch);
+typedef void (*vga_hw_screen_dump_async_ptr)(void *, const char *filename,
+ bool cswitch);
typedef void (*vga_hw_text_update_ptr)(void *, console_ch_t *);
DisplayState *graphic_console_init(vga_hw_update_ptr update,
vga_hw_invalidate_ptr invalidate,
vga_hw_screen_dump_ptr screen_dump,
vga_hw_text_update_ptr text_update,
+ vga_hw_screen_dump_async_ptr
+screen_dump_async,
void *opaque);
void vga_hw_update(void);
void vga_hw_invalidate(void);
void vga_hw_screen_dump(const char *filename);
+void vga_hw_screen_dump_async(const char *filename);
void vga_hw_text_update(console_ch_t *chardata);
void monitor_protocol_screen_dump_complete_event(const char *filename);
diff --git a/hw/blizzard.c b/hw/blizzard.c
index c7d844d..cc51045 100644
--- a/hw/blizzard.c
+++ b/hw/blizzard.c
@@ -963,7 +963,7 @@ void *s1d13745_init(qemu_irq gpio_int)
s->state = graphic_console_init(blizzard_update_display,
blizzard_invalidate_display,
- blizzard_screen_dump, NULL, s);
+ blizzard_screen_dump, NULL, NULL, s);
switch (ds_get_bits_per_pixel(s->state)) {
case 0:
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 4edcb94..d2a9c27 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2900,7 +2900,7 @@ static int vga_initfn(ISADevice *dev)
isa_address_space(dev));
s->ds = graphic_console_init(s->update, s->invalidate,
s->screen_dump, s->text_update,
- s);
+ s->screen_dump_async, s);
rom_add_vga(VGABIOS_CIRRUS_FILENAME);
/* XXX ISA-LFB support */
/* FIXME not qdev yet */
@@ -2941,7 +2941,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
cirrus_init_common(s, device_id, 1, pci_address_space(dev));
s->vga.ds = graphic_console_init(s->vga.update, s->vga.invalidate,
s->vga.screen_dump, s->vga.text_update,
-
