Re: [Qemu-devel] [PATCH v2] qxl: async I/O

[Alon Levy]

On Fri, Jul 08, 2011 at 09:17:50AM +0200, Gerd Hoffmann wrote:
> >+void qxl_spice_update_area_async(PCIQXLDevice *qxl, uint32_t surface_id,
> >+                           struct QXLRect *area, struct QXLRect 
> >*dirty_rects,
> >+                           uint32_t num_dirty_rects, uint32_t 
> >clear_dirty_region,
> >+                           int async)
> >+{
> >+    if (async) {
> >+        qxl->ssd.worker->update_area_async(qxl->ssd.worker, surface_id, 
> >area, dirty_rects,
> >+                                 num_dirty_rects, clear_dirty_region, 0);
> 
> Fails to build with older libspice.

> 
> >+    } else {
> >+        qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, 
> >dirty_rects,
> >+                                 num_dirty_rects, clear_dirty_region);
> >+    }
> >+}
> >
> >  void qxl_spice_update_area(PCIQXLDevice *qxl, uint32_t surface_id,
> >                             struct QXLRect *area, struct QXLRect 
> > *dirty_rects,
> >                             uint32_t num_dirty_rects, uint32_t 
> > clear_dirty_region)
> >  {
> >-    qxl->ssd.worker->update_area(qxl->ssd.worker, surface_id, area, 
> >dirty_rects,
> >-                             num_dirty_rects, clear_dirty_region);
> >+    qxl_spice_update_area_async(qxl, surface_id, area, dirty_rects,
> >+                                num_dirty_rects, clear_dirty_region, 0);
> >  }
> 
> Pretty pointless wrapper IMHO.

The above two lines change was a mistake. What about:

qxl_spice_update_area_async(...)
{
#ifdef ..
 if (async) {
    qxl->ssd.worker->update_area_async(...)
 } else {
    qxl_spice_update_area(...)
 }
#else
 qxl_spice_update_area(...)
#endif
}

> 
> >-void qxl_spice_destroy_surface_wait(PCIQXLDevice *qxl, uint32_t id)
> >+static void qxl_spice_destroy_surface_wait_complete(PCIQXLDevice *qxl)
> >  {
> >      qemu_mutex_lock(&qxl->track_lock);
> >-    PANIC_ON(id>= NUM_SURFACES);
> >-    qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
> >-    qxl->guest_surfaces.cmds[id] = 0;
> >+    qxl->guest_surfaces.cmds[qxl->io_data.surface_id] = 0;
> 
> I'd suggest to pass in the surface id as argument instead.

I can use the cookie if that's what you mean (which I guess means it will make
more sense to define it as a void pointer).
> 
> >      qxl->guest_surfaces.count--;
> >      qemu_mutex_unlock(&qxl->track_lock);
> >  }
> >
> >+static void qxl_spice_destroy_surface_wait_async(PCIQXLDevice *qxl, 
> >uint32_t id, int async)
> >+{
> >+    qxl->io_data.surface_id = id;
> >+    if (async) {
> >+        qxl->ssd.worker->destroy_surface_wait_async(qxl->ssd.worker, id, 0);
> >+    } else {
> >+        qxl->ssd.worker->destroy_surface_wait(qxl->ssd.worker, id);
> >+        qxl_spice_destroy_surface_wait_complete(qxl);
> 
> qxl_spice_destroy_surface_wait_complete(qxl, id);
and use the cookie on the async_complete with appropriate casting and free, got 
it.

> 
> >+    }
> >+}
> >+
> >  void qxl_spice_loadvm_commands(PCIQXLDevice *qxl, struct QXLCommandExt 
> > *ext,
> >                                 uint32_t count)
> >  {
> >@@ -171,15 +193,29 @@ void qxl_spice_reset_memslots(PCIQXLDevice *qxl)
> >      qxl->ssd.worker->reset_memslots(qxl->ssd.worker);
> >  }
> >
> >-void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
> >+static void qxl_spice_destroy_surfaces_complete(PCIQXLDevice *qxl)
> >  {
> >      qemu_mutex_lock(&qxl->track_lock);
> >-    qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
> >      memset(&qxl->guest_surfaces.cmds, 0, sizeof(qxl->guest_surfaces.cmds));
> >      qxl->guest_surfaces.count = 0;
> >      qemu_mutex_unlock(&qxl->track_lock);
> >  }
> >
> >+static void qxl_spice_destroy_surfaces(PCIQXLDevice *qxl)
> >+{
> >+    qxl->ssd.worker->destroy_surfaces(qxl->ssd.worker);
> >+    qxl_spice_destroy_surfaces_complete(qxl);
> >+}
> >+
> >+static void qxl_spice_destroy_surfaces_async(PCIQXLDevice *qxl, int async)
> >+{
> >+    if (async) {
> >+        qxl->ssd.worker->destroy_surfaces_async(qxl->ssd.worker, 0);
> >+    } else {
> >+        qxl_spice_destroy_surfaces(qxl);
> >+    }
> >+}
> 
> I'd combine those into one function simliar to
> qxl_spice_destroy_surface_wait_async (and we don't need the _async
> suffix if we have a single version only which gets passed in async
> as argument).
ok, I'll ditch the suffix.

> 
> >+
> >  void qxl_spice_reset_image_cache(PCIQXLDevice *qxl)
> >  {
> >      qxl->ssd.worker->reset_image_cache(qxl->ssd.worker);
> >@@ -706,6 +742,38 @@ static int interface_flush_resources(QXLInstance *sin)
> >      return ret;
> >  }
> >
> >+static void qxl_add_memslot_complete(PCIQXLDevice *d);
> >+static void qxl_create_guest_primary_complete(PCIQXLDevice *d);
> >+
> >+/* called from spice server thread context only */
> >+static void interface_async_complete(QXLInstance *sin, uint64_t cookie)
> >+{
> >+    PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
> >+    uint32_t current_async;
> >+
> >+    qemu_mutex_lock(&qxl->async_lock);
> >+    current_async = qxl->current_async;
> >+    qxl->current_async = QXL_UNDEFINED_IO;
> >+    qemu_mutex_unlock(&qxl->async_lock);
> 
> I'd tend to use the cookie to pass that information (also the stuff
> in io_data).

yeah, I'll throw that, malloc something, cast to cookie, pass it, cast back, 
free.

> 
> >-static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t 
> >delta)
> >+static void qxl_add_memslot_complete(PCIQXLDevice *d)
> 
> I think it isn't needed to move that to the completion callback.
> Memory slots can be created and destroyed with I/O commands only, so
> there is no need to care about the ordering like we have to with
> surfaces.
ok.

> 
> >      qemu_mutex_init(&qxl->track_lock);
> >+    qemu_mutex_init(&qxl->async_lock);
> 
> Do we really need two locks?
> When passing info via cookie, doesn't the need for the async lock go
> away completely?
the current_async still gets changed from two threads (on [vcpu]ioport_write
and [worker]async_complete)

> 
> >index af10ae8..b7bc0de 100644
> >--- a/ui/spice-display.c
> >+++ b/ui/spice-display.c
> >@@ -62,6 +62,20 @@ void qemu_spice_rect_union(QXLRect *dest, const QXLRect 
> >*r)
> >      dest->right = MAX(dest->right, r->right);
> >  }
> >
> >+int qemu_spice_supports_async(SimpleSpiceDisplay *ssd)
> >+{
> >+    return (ssd->worker->major_version>  3 ||
> >+            (ssd->worker->major_version == 3&&  
> >ssd->worker->minor_version>= 1));
> >+}
> 
> Doing a runtime check here is pointless, just use
> #if SPICE_INTERFACE_QXL_MINOR >= 1
> ...
> #endif
this is a runtime check - what's preventing someone from compiling with 3.1 and 
running with 3.0?
that we will require a newer library version? (which I am yet to send a patch 
for)

> 
> >  void qemu_spice_create_primary_surface(SimpleSpiceDisplay *ssd, uint32_t 
> > id,
> >                                         QXLDevSurfaceCreate *surface)
> >  {
> >      ssd->worker->create_primary_surface(ssd->worker, id, surface);
> >  }
> >
> >+void qemu_spice_destroy_primary_surface_async(SimpleSpiceDisplay *ssd, 
> >uint32_t id, int async)
> >+{
> >+    if (async) {
> >+        ssd->worker->destroy_primary_surface_async(ssd->worker, id, 0);
> >+    } else {
> >+        qemu_spice_destroy_primary_surface(ssd, id);
> >+    }
> >+}
> 
> Like for all others: one only please.
ok

> 
> cheers,
>   Gerd