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

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

2011-07-08 Thread Gerd Hoffmann


+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.


-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.


  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);


+}
+}
+
  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).



+
  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).



-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.



  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?



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);

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

2011-07-08 Thread Gerd Hoffmann


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
}


I would do

if (async) {
#if ...
  worker-foo_async()
#else
  abort() /* should hot happen */
#endif
} else {
  worker-foo
}


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


cookie should be big enougth to store the info directly.  malloc works 
too though.



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)


Yes, thats why the minor version of the shared library needs to be raised.

cheers,
  Gerd

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

2011-07-08 Thread Gerd Hoffmann


btw, I'm looking at #if.*MINOR code like

  #if SPICE_INTERFACE_CORE_MINOR= 3

(ui/spice-core.c)

Shouldn't that be checking the MAJOR as well?


major changing means a incompatible change.  I doubt we ever will do 
that.  But if you feel better checking that it probably should just be a


#if SPICE_INTERFACE_CORE_MAJOR != 1
#error incompatible spice core interface
#endif

at the top of the file.

cheers,
  Gerd

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

2011-07-08 Thread 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.

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

2011-07-08 Thread 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.

btw, I'm looking at #if.*MINOR code like

 #if SPICE_INTERFACE_CORE_MINOR = 3

(ui/spice-core.c)

Shouldn't that be checking the MAJOR as well?

[snip]

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

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

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

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

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

5 matches

Site Navigation

Mail list logo

Footer information