Re: [Spice-devel] [PATCH v2] server: add async io support
On Thu, Jul 07, 2011 at 09:29:47AM +0200, Gerd Hoffmann wrote: > >Hi, > >I find it problematic to decouple the qxl operations upon QXL_IO and the > >worker operations. For example: upon destroy_surfaces, the qxl resets > >the tracked guest commands. However, since destroy_surfaces is asynced, > >the worker might read commands from the ring before handling destroy > >surfaces, and then the qxl guest commands will be modified. > > That should be fixed on the qemu side I think. qxl should update > its status when it receives the notification that the operation is > complete, not after submitting the request. > > (Ha! first use case for the magic cookie ;) > I could use the cookies, but since I'm already doing a guest_bug when guest does two asyncs overlapping, I think I can just use the last remembered one I store for the express purpose of discovering this bug (until now). > >I suggest a partial decoupling: on async io, the dispatcher will still > >wait for RED_WORKER_MESSAGE_READY, but the server will set it > >immediately when it receives the async message. > > No. I don't want async messages take a round trip to the worker > thread. Even if the worker sends an answer instantly alone the > thread scheduling adds noticeable latencies. > > cheers, > Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2] server: add async io support
On Thu, Jul 07, 2011 at 09:38:01AM +0300, Yonit Halperin wrote:
> On 07/06/2011 08:37 PM, Alon Levy wrote:
> >The new _ASYNC io's in qxl_dev listed at the end get six new api
> >functions, and an additional callback function "async_complete". When
> >the async version of a specific io is used, completion is notified by
> >calling async_complete, and no READY message is written or expected by
> >the dispatcher.
> >
> >update_area has been changed to push QXLRects to the worker thread, where
> >the conversion to SpiceRect takes place.
> >
> >Added api:
> >
> >QXLWorker:
> > update_area_async
> > add_memslot_async
> > destroy_surfaces_async
> > destroy_primary_surface_async
> > create_primary_surface_async
> > destroy_surface_wait_async
> > oom_async
> >
> >
> >
> >-static void qxl_worker_oom(QXLWorker *qxl_worker)
> >+static void qxl_worker_oom_helper(QXLWorker *qxl_worker, int async)
> > {
> > RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
> >+RedWorkerMessage message;
> >+
> > if (!test_bit(RED_WORKER_PENDING_OOM, dispatcher->pending)) {
> >-RedWorkerMessage message = RED_WORKER_MESSAGE_OOM;
> >+if (async) {
> >+message = RED_WORKER_MESSAGE_OOM_ASYNC;
> >+} else {
> >+message = RED_WORKER_MESSAGE_OOM;
> >+}
> > set_bit(RED_WORKER_PENDING_OOM,&dispatcher->pending);
> > write_message(dispatcher->channel,&message);
> > }
> > }
> >
>
> OOM has always been async - do we need two different RED_WORKER_MESSAGEs?
No, I guess we can throw away the IO as well then.
>
> >+static void qxl_worker_oom(QXLWorker *qxl_worker)
> >+{
> >+qxl_worker_oom_helper(qxl_worker, 0);
> >+}
> >+
> >+static void qxl_worker_oom_async(QXLWorker *qxl_worker)
> >+{
> >+qxl_worker_oom_helper(qxl_worker, 1);
> >+}
> >+
>
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2] server: add async io support
On Thu, Jul 07, 2011 at 09:16:33AM +0300, Yonit Halperin wrote:
> On 07/06/2011 08:37 PM, Alon Levy wrote:
> >The new _ASYNC io's in qxl_dev listed at the end get six new api
> >functions, and an additional callback function "async_complete". When
> >the async version of a specific io is used, completion is notified by
> >calling async_complete, and no READY message is written or expected by
> >the dispatcher.
> >
> >update_area has been changed to push QXLRects to the worker thread, where
> >the conversion to SpiceRect takes place.
> >
> >Added api:
> >
> >QXLWorker:
> > update_area_async
> > add_memslot_async
> > destroy_surfaces_async
> > destroy_primary_surface_async
> > create_primary_surface_async
> > destroy_surface_wait_async
> > oom_async
> >
> >QXLInterface:
> > async_complete
> >---
> > server/red_dispatcher.c | 179
> > ++-
> > server/red_parse_qxl.c |2 +-
> > server/red_parse_qxl.h |2 +-
> > server/red_worker.c | 80 ++---
> > server/red_worker.h |8 ++
> > server/spice.h | 12 +++
> > 6 files changed, 221 insertions(+), 62 deletions(-)
> >
> >diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
> >index 56446ab..74c7af2 100644
> >--- a/server/red_dispatcher.c
> >+++ b/server/red_dispatcher.c
> >@@ -209,38 +209,49 @@ static void update_client_mouse_allowed(void)
> > }
> > }
> >
> >-static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t
> >surface_id,
> >+static void qxl_worker_update_area_helper(QXLWorker *qxl_worker, uint32_t
> >surface_id,
> > QXLRect *qxl_area, QXLRect
> > *qxl_dirty_rects,
> >- uint32_t num_dirty_rects, uint32_t
> >clear_dirty_region)
> >+ uint32_t num_dirty_rects, uint32_t
> >clear_dirty_region,
> >+ int async)
> > {
> > RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
> >-RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
> >-SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
> >-SpiceRect *area = spice_new0(SpiceRect, 1);
> >-int i;
> >-
> >-red_get_rect_ptr(area, qxl_area);
> >+RedWorkerMessage message;
> >
> >+if (async) {
> >+ message = RED_WORKER_MESSAGE_UPDATE_ASYNC;
> >+} else {
> >+ message = RED_WORKER_MESSAGE_UPDATE;
> >+}
> > write_message(dispatcher->channel,&message);
> > send_data(dispatcher->channel,&surface_id, sizeof(uint32_t));
> >-send_data(dispatcher->channel,&area, sizeof(SpiceRect *));
> >-send_data(dispatcher->channel,&dirty_rects, sizeof(SpiceRect *));
> >+send_data(dispatcher->channel,&qxl_area, sizeof(QXLRect *));
> >+send_data(dispatcher->channel,&qxl_dirty_rects, sizeof(QXLRect *));
> > send_data(dispatcher->channel,&num_dirty_rects, sizeof(uint32_t));
> > send_data(dispatcher->channel,&clear_dirty_region, sizeof(uint32_t));
> >+if (async) {
> >+return;
> >+}
>
> Hi,
> I find it problematic to decouple the qxl operations upon QXL_IO and
> the worker operations. For example: upon destroy_surfaces, the qxl
> resets the tracked guest commands. However, since destroy_surfaces
> is asynced, the worker might read commands from the ring before
> handling destroy surfaces, and then the qxl guest commands will be
> modified.
> I suggest a partial decoupling: on async io, the dispatcher will
> still wait for RED_WORKER_MESSAGE_READY, but the server will set it
> immediately when it receives the async message.
>
ok, I guess I see two solutions:
1. doing a stop on async, and a start on async_complete (a little heavy handed)
2. use the remembered io and do the "after completion" part in it, not on the
io
call, so from async_complete.
I'll do 2.
> > read_message(dispatcher->channel,&message);
> > ASSERT(message == RED_WORKER_MESSAGE_READY);
> >+}
> >
> >-for (i = 0; i< num_dirty_rects; i++) {
> >-qxl_dirty_rects[i].top= dirty_rects[i].top;
> >-qxl_dirty_rects[i].left = dirty_rects[i].left;
> >-qxl_dirty_rects[i].bottom = dirty_rects[i].bottom;
> >-qxl_dirty_rects[i].right = dirty_rects[i].right;
> >-}
> >+static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t
> >surface_id,
> >+ QXLRect *qxl_area, QXLRect
> >*qxl_dirty_rects,
> >+ uint32_t num_dirty_rects, uint32_t
> >clear_dirty_region)
> >+{
> >+qxl_worker_update_area_helper(qxl_worker, surface_id, qxl_area,
> >qxl_dirty_rects, num_dirty_rects,
> >+clear_dirty_region, 0);
> >+}
> >
> >-free(dirty_rects);
> >-free(area);
> >+static void qxl_worker_update_area_async(QXLWorker *qxl_worker, uint32_t
> >surface_id,
> >+ QXLRect *qxl_area, QXLRect
> >*qxl_dirty_rects,
> >+
Re: [Spice-devel] [PATCH v2] server: add async io support
Hi, I find it problematic to decouple the qxl operations upon QXL_IO and the worker operations. For example: upon destroy_surfaces, the qxl resets the tracked guest commands. However, since destroy_surfaces is asynced, the worker might read commands from the ring before handling destroy surfaces, and then the qxl guest commands will be modified. That should be fixed on the qemu side I think. qxl should update its status when it receives the notification that the operation is complete, not after submitting the request. (Ha! first use case for the magic cookie ;) I suggest a partial decoupling: on async io, the dispatcher will still wait for RED_WORKER_MESSAGE_READY, but the server will set it immediately when it receives the async message. No. I don't want async messages take a round trip to the worker thread. Even if the worker sends an answer instantly alone the thread scheduling adds noticeable latencies. cheers, Gerd ___ Spice-devel mailing list Spice-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2] server: add async io support
On 07/06/2011 08:37 PM, Alon Levy wrote:
The new _ASYNC io's in qxl_dev listed at the end get six new api
functions, and an additional callback function "async_complete". When
the async version of a specific io is used, completion is notified by
calling async_complete, and no READY message is written or expected by
the dispatcher.
update_area has been changed to push QXLRects to the worker thread, where
the conversion to SpiceRect takes place.
Added api:
QXLWorker:
update_area_async
add_memslot_async
destroy_surfaces_async
destroy_primary_surface_async
create_primary_surface_async
destroy_surface_wait_async
oom_async
-static void qxl_worker_oom(QXLWorker *qxl_worker)
+static void qxl_worker_oom_helper(QXLWorker *qxl_worker, int async)
{
RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
+RedWorkerMessage message;
+
if (!test_bit(RED_WORKER_PENDING_OOM, dispatcher->pending)) {
-RedWorkerMessage message = RED_WORKER_MESSAGE_OOM;
+if (async) {
+message = RED_WORKER_MESSAGE_OOM_ASYNC;
+} else {
+message = RED_WORKER_MESSAGE_OOM;
+}
set_bit(RED_WORKER_PENDING_OOM,&dispatcher->pending);
write_message(dispatcher->channel,&message);
}
}
OOM has always been async - do we need two different RED_WORKER_MESSAGEs?
+static void qxl_worker_oom(QXLWorker *qxl_worker)
+{
+qxl_worker_oom_helper(qxl_worker, 0);
+}
+
+static void qxl_worker_oom_async(QXLWorker *qxl_worker)
+{
+qxl_worker_oom_helper(qxl_worker, 1);
+}
+
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
Re: [Spice-devel] [PATCH v2] server: add async io support
On 07/06/2011 08:37 PM, Alon Levy wrote:
The new _ASYNC io's in qxl_dev listed at the end get six new api
functions, and an additional callback function "async_complete". When
the async version of a specific io is used, completion is notified by
calling async_complete, and no READY message is written or expected by
the dispatcher.
update_area has been changed to push QXLRects to the worker thread, where
the conversion to SpiceRect takes place.
Added api:
QXLWorker:
update_area_async
add_memslot_async
destroy_surfaces_async
destroy_primary_surface_async
create_primary_surface_async
destroy_surface_wait_async
oom_async
QXLInterface:
async_complete
---
server/red_dispatcher.c | 179 ++-
server/red_parse_qxl.c |2 +-
server/red_parse_qxl.h |2 +-
server/red_worker.c | 80 ++---
server/red_worker.h |8 ++
server/spice.h | 12 +++
6 files changed, 221 insertions(+), 62 deletions(-)
diff --git a/server/red_dispatcher.c b/server/red_dispatcher.c
index 56446ab..74c7af2 100644
--- a/server/red_dispatcher.c
+++ b/server/red_dispatcher.c
@@ -209,38 +209,49 @@ static void update_client_mouse_allowed(void)
}
}
-static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
+static void qxl_worker_update_area_helper(QXLWorker *qxl_worker, uint32_t
surface_id,
QXLRect *qxl_area, QXLRect
*qxl_dirty_rects,
- uint32_t num_dirty_rects, uint32_t
clear_dirty_region)
+ uint32_t num_dirty_rects, uint32_t
clear_dirty_region,
+ int async)
{
RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
-RedWorkerMessage message = RED_WORKER_MESSAGE_UPDATE;
-SpiceRect *dirty_rects = spice_new0(SpiceRect, num_dirty_rects);
-SpiceRect *area = spice_new0(SpiceRect, 1);
-int i;
-
-red_get_rect_ptr(area, qxl_area);
+RedWorkerMessage message;
+if (async) {
+ message = RED_WORKER_MESSAGE_UPDATE_ASYNC;
+} else {
+ message = RED_WORKER_MESSAGE_UPDATE;
+}
write_message(dispatcher->channel,&message);
send_data(dispatcher->channel,&surface_id, sizeof(uint32_t));
-send_data(dispatcher->channel,&area, sizeof(SpiceRect *));
-send_data(dispatcher->channel,&dirty_rects, sizeof(SpiceRect *));
+send_data(dispatcher->channel,&qxl_area, sizeof(QXLRect *));
+send_data(dispatcher->channel,&qxl_dirty_rects, sizeof(QXLRect *));
send_data(dispatcher->channel,&num_dirty_rects, sizeof(uint32_t));
send_data(dispatcher->channel,&clear_dirty_region, sizeof(uint32_t));
+if (async) {
+return;
+}
Hi,
I find it problematic to decouple the qxl operations upon QXL_IO and the
worker operations. For example: upon destroy_surfaces, the qxl resets
the tracked guest commands. However, since destroy_surfaces is asynced,
the worker might read commands from the ring before handling destroy
surfaces, and then the qxl guest commands will be modified.
I suggest a partial decoupling: on async io, the dispatcher will still
wait for RED_WORKER_MESSAGE_READY, but the server will set it
immediately when it receives the async message.
read_message(dispatcher->channel,&message);
ASSERT(message == RED_WORKER_MESSAGE_READY);
+}
-for (i = 0; i< num_dirty_rects; i++) {
-qxl_dirty_rects[i].top= dirty_rects[i].top;
-qxl_dirty_rects[i].left = dirty_rects[i].left;
-qxl_dirty_rects[i].bottom = dirty_rects[i].bottom;
-qxl_dirty_rects[i].right = dirty_rects[i].right;
-}
+static void qxl_worker_update_area(QXLWorker *qxl_worker, uint32_t surface_id,
+ QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
+ uint32_t num_dirty_rects, uint32_t
clear_dirty_region)
+{
+qxl_worker_update_area_helper(qxl_worker, surface_id, qxl_area,
qxl_dirty_rects, num_dirty_rects,
+clear_dirty_region, 0);
+}
-free(dirty_rects);
-free(area);
+static void qxl_worker_update_area_async(QXLWorker *qxl_worker, uint32_t
surface_id,
+ QXLRect *qxl_area, QXLRect *qxl_dirty_rects,
+ uint32_t num_dirty_rects, uint32_t
clear_dirty_region)
+{
+qxl_worker_update_area_helper(qxl_worker, surface_id, qxl_area,
qxl_dirty_rects, num_dirty_rects,
+clear_dirty_region, 1);
}
+
static void qxl_worker_add_memslot(QXLWorker *qxl_worker, QXLDevMemSlot
*mem_slot)
{
RedDispatcher *dispatcher = (RedDispatcher *)qxl_worker;
@@ -252,6 +263,15 @@ static void qxl_worker_add_memslot(QXLWorker *qxl_worker,
QXLDevMemSlot *mem_slo
ASSERT(message == RED_WORKER_MESSAGE_READY);
}
+static void qxl_worker_add_memslot_async(QXLWorker *qxl_worker, QXLDevMemSlot
*mem_slot)
+{
+Red
