Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-11-25 Thread Christophe Fergeau 
Hey,

On Mon, Nov 24, 2014 at 04:56:26PM +0100, Marc-André Lureau wrote:
 In an effort to reduce the wakeups per second, get rid of the
 write_to_dev timer when the implementation supports
 SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.
 
 When this flag is set, the frontend instance is responsible for calling
 spice_char_device_wakeup() when the device is ready to perform IO.
 
 Related to:
 https://bugzilla.redhat.com/show_bug.cgi?id=912763
 ---
 
 v3: fix interface version check, spotted by Uri
 
 server/char_device.c | 36 +++-
  server/spice.h   |  9 +++--
  2 files changed, 34 insertions(+), 11 deletions(-)
 
 diff --git a/server/char_device.c b/server/char_device.c
 index 6d2339e..5f1b32d 100644
 --- a/server/char_device.c
 +++ b/server/char_device.c
 @@ -438,7 +438,10 @@ static int 
 spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  }
  
  spice_char_device_state_ref(dev);
 -core-timer_cancel(dev-write_to_dev_timer);
 +
 +if (dev-write_to_dev_timer) {
 +core-timer_cancel(dev-write_to_dev_timer);
 +}
  
  sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
 base);
  while (dev-running) {
 @@ -473,8 +476,10 @@ static int 
 spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  /* retry writing as long as the write queue is not empty */
  if (dev-running) {
  if (dev-cur_write_buf) {
 -core-timer_start(dev-write_to_dev_timer,
 -  CHAR_DEVICE_WRITE_TO_TIMEOUT);
 +if (dev-write_to_dev_timer) {
 +core-timer_start(dev-write_to_dev_timer,
 +  CHAR_DEVICE_WRITE_TO_TIMEOUT);
 +}

The previous code was making sure to handle partial writes, the
SPICE_CHAR_DEVICE_NOTIFY_WRITABLE does not seem to handle that, I don't
know how common this is.

  } else {
  spice_assert(ring_is_empty(dev-write_queue));
  }
 @@ -488,7 +493,9 @@ static void spice_char_dev_write_retry(void *opaque)
  {
  SpiceCharDeviceState *dev = opaque;
  
 -core-timer_cancel(dev-write_to_dev_timer);
 +if (dev-write_to_dev_timer) {
 +core-timer_cancel(dev-write_to_dev_timer);
 +}
  spice_char_device_write_to_device(dev);
  }
  
 @@ -635,6 +642,7 @@ SpiceCharDeviceState 
 *spice_char_device_state_create(SpiceCharDeviceInstance *si
   void *opaque)
  {
  SpiceCharDeviceState *char_dev;
 +SpiceCharDeviceInterface *sif;
  
  spice_assert(sin);
  spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
 @@ -652,10 +660,15 @@ SpiceCharDeviceState 
 *spice_char_device_state_create(SpiceCharDeviceInstance *si
  ring_init(char_dev-write_bufs_pool);
  ring_init(char_dev-clients);
  
 -char_dev-write_to_dev_timer = 
 core-timer_add(spice_char_dev_write_retry, char_dev);
 -if (!char_dev-write_to_dev_timer) {
 -spice_error(failed creating char dev write timer);
 +sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, 
 SpiceCharDeviceInterface, base);
 +if ((sif-base.minor_version = 1  sif-base.minor_version = 2) ||

First minor should be major

For the record, this check is wrong in general, it would fail for major
== 0 and minor == 3. Major version has always been 1, so in this
specific case it's good enough.


 +!(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
 +char_dev-write_to_dev_timer = 
 core-timer_add(spice_char_dev_write_retry, char_dev);
 +if (!char_dev-write_to_dev_timer) {
 +spice_error(failed creating char dev write timer);
 +}
  }
 +
  char_dev-refs = 1;
  sin-st = char_dev;
  spice_debug(sin %p dev_state %p, sin, char_dev);
 @@ -697,7 +710,9 @@ static void 
 spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
  void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
  {
  reds_on_char_device_state_destroy(char_dev);
 -core-timer_remove(char_dev-write_to_dev_timer);
 +if (char_dev-write_to_dev_timer) {
 +core-timer_remove(char_dev-write_to_dev_timer);
 +}
  write_buffers_queue_free(char_dev-write_queue);
  write_buffers_queue_free(char_dev-write_bufs_pool);
  if (char_dev-cur_write_buf) {
 @@ -805,7 +820,9 @@ void spice_char_device_stop(SpiceCharDeviceState *dev)
  spice_debug(dev_state %p, dev);
  dev-running = FALSE;
  dev-active = FALSE;
 -core-timer_cancel(dev-write_to_dev_timer);
 +if (dev-write_to_dev_timer) {
 +core-timer_cancel(dev-write_to_dev_timer);
 +}
  }
  
  void spice_char_device_reset(SpiceCharDeviceState *dev)
 @@ -842,6 +859,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
  
  void spice_char_device_wakeup(SpiceCharDeviceState *dev)
  {
 +spice_char_device_write_to_device(dev);
  spice_char_device_read_from_device(dev);
  }

I still don't feel really comfortable changing a

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-11-25 Thread Marc-André Lureau 
On Tue, Nov 25, 2014 at 2:11 PM, Christophe Fergeau cferg...@redhat.com wrote:
 Hey,

 On Mon, Nov 24, 2014 at 04:56:26PM +0100, Marc-André Lureau wrote:
 In an effort to reduce the wakeups per second, get rid of the
 write_to_dev timer when the implementation supports
 SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.

 When this flag is set, the frontend instance is responsible for calling
 spice_char_device_wakeup() when the device is ready to perform IO.

 Related to:
 https://bugzilla.redhat.com/show_bug.cgi?id=912763
 ---

 v3: fix interface version check, spotted by Uri

 server/char_device.c | 36 +++-
  server/spice.h   |  9 +++--
  2 files changed, 34 insertions(+), 11 deletions(-)

 diff --git a/server/char_device.c b/server/char_device.c
 index 6d2339e..5f1b32d 100644
 --- a/server/char_device.c
 +++ b/server/char_device.c
 @@ -438,7 +438,10 @@ static int 
 spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  }

  spice_char_device_state_ref(dev);
 -core-timer_cancel(dev-write_to_dev_timer);
 +
 +if (dev-write_to_dev_timer) {
 +core-timer_cancel(dev-write_to_dev_timer);
 +}

  sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
 base);
  while (dev-running) {
 @@ -473,8 +476,10 @@ static int 
 spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  /* retry writing as long as the write queue is not empty */
  if (dev-running) {
  if (dev-cur_write_buf) {
 -core-timer_start(dev-write_to_dev_timer,
 -  CHAR_DEVICE_WRITE_TO_TIMEOUT);
 +if (dev-write_to_dev_timer) {
 +core-timer_start(dev-write_to_dev_timer,
 +  CHAR_DEVICE_WRITE_TO_TIMEOUT);
 +}

 The previous code was making sure to handle partial writes, the
 SPICE_CHAR_DEVICE_NOTIFY_WRITABLE does not seem to handle that, I don't
 know how common this is.

I don't get your question, the point is to remove the polling, when
the device can handle more write it will notify and the write can
continue. So it still handles partial write.


  } else {
  spice_assert(ring_is_empty(dev-write_queue));
  }
 @@ -488,7 +493,9 @@ static void spice_char_dev_write_retry(void *opaque)
  {
  SpiceCharDeviceState *dev = opaque;

 -core-timer_cancel(dev-write_to_dev_timer);
 +if (dev-write_to_dev_timer) {
 +core-timer_cancel(dev-write_to_dev_timer);
 +}
  spice_char_device_write_to_device(dev);
  }

 @@ -635,6 +642,7 @@ SpiceCharDeviceState 
 *spice_char_device_state_create(SpiceCharDeviceInstance *si
   void *opaque)
  {
  SpiceCharDeviceState *char_dev;
 +SpiceCharDeviceInterface *sif;

  spice_assert(sin);
  spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
 @@ -652,10 +660,15 @@ SpiceCharDeviceState 
 *spice_char_device_state_create(SpiceCharDeviceInstance *si
  ring_init(char_dev-write_bufs_pool);
  ring_init(char_dev-clients);

 -char_dev-write_to_dev_timer = 
 core-timer_add(spice_char_dev_write_retry, char_dev);
 -if (!char_dev-write_to_dev_timer) {
 -spice_error(failed creating char dev write timer);
 +sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, 
 SpiceCharDeviceInterface, base);
 +if ((sif-base.minor_version = 1  sif-base.minor_version = 2) ||

 First minor should be major

right, see next comment


 For the record, this check is wrong in general, it would fail for major
 == 0 and minor == 3. Major version has always been 1, so in this
 specific case it's good enough.

well, in fact the major check is useless, as we error out in
add_interface() if the major don't match (you can see similar code in
guest_set_client_capabilities)

So I'll just remove the first major check



 +!(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
 +char_dev-write_to_dev_timer = 
 core-timer_add(spice_char_dev_write_retry, char_dev);
 +if (!char_dev-write_to_dev_timer) {
 +spice_error(failed creating char dev write timer);
 +}
  }
 +
  char_dev-refs = 1;
  sin-st = char_dev;
  spice_debug(sin %p dev_state %p, sin, char_dev);
 @@ -697,7 +710,9 @@ static void 
 spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
  void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
  {
  reds_on_char_device_state_destroy(char_dev);
 -core-timer_remove(char_dev-write_to_dev_timer);
 +if (char_dev-write_to_dev_timer) {
 +core-timer_remove(char_dev-write_to_dev_timer);
 +}
  write_buffers_queue_free(char_dev-write_queue);
  write_buffers_queue_free(char_dev-write_bufs_pool);
  if (char_dev-cur_write_buf) {
 @@ -805,7 +820,9 @@ void spice_char_device_stop(SpiceCharDeviceState *dev)
  spice_debug(dev_state %p, dev);
  dev-running = FALSE;
  dev-active = FALSE;
 -

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-11-25 Thread Christophe Fergeau 
On Tue, Nov 25, 2014 at 02:49:07PM +0100, Marc-André Lureau wrote:
 On Tue, Nov 25, 2014 at 2:11 PM, Christophe Fergeau cferg...@redhat.com 
 wrote:
  Hey,
 
  On Mon, Nov 24, 2014 at 04:56:26PM +0100, Marc-André Lureau wrote:
  In an effort to reduce the wakeups per second, get rid of the
  write_to_dev timer when the implementation supports
  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.
 
  When this flag is set, the frontend instance is responsible for calling
  spice_char_device_wakeup() when the device is ready to perform IO.
 
  Related to:
  https://bugzilla.redhat.com/show_bug.cgi?id=912763
  ---
 
  v3: fix interface version check, spotted by Uri
 
  server/char_device.c | 36 +++-
   server/spice.h   |  9 +++--
   2 files changed, 34 insertions(+), 11 deletions(-)
 
  diff --git a/server/char_device.c b/server/char_device.c
  index 6d2339e..5f1b32d 100644
  --- a/server/char_device.c
  +++ b/server/char_device.c
  @@ -438,7 +438,10 @@ static int 
  spice_char_device_write_to_device(SpiceCharDeviceState *dev)
   }
 
   spice_char_device_state_ref(dev);
  -core-timer_cancel(dev-write_to_dev_timer);
  +
  +if (dev-write_to_dev_timer) {
  +core-timer_cancel(dev-write_to_dev_timer);
  +}
 
   sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
  base);
   while (dev-running) {
  @@ -473,8 +476,10 @@ static int 
  spice_char_device_write_to_device(SpiceCharDeviceState *dev)
   /* retry writing as long as the write queue is not empty */
   if (dev-running) {
   if (dev-cur_write_buf) {
  -core-timer_start(dev-write_to_dev_timer,
  -  CHAR_DEVICE_WRITE_TO_TIMEOUT);
  +if (dev-write_to_dev_timer) {
  +core-timer_start(dev-write_to_dev_timer,
  +  CHAR_DEVICE_WRITE_TO_TIMEOUT);
  +}
 
  The previous code was making sure to handle partial writes, the
  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE does not seem to handle that, I don't
  know how common this is.
 
 I don't get your question, the point is to remove the polling, when
 the device can handle more write it will notify and the write can
 continue. So it still handles partial write.

This assumes that the only time when sif-write can return a negative
value is when the device cannot handle more writes, this was not obvious
from a quick look at the code, hence the question. With that assumption,
the change makes sense.

 
 
   } else {
   spice_assert(ring_is_empty(dev-write_queue));
   }
  @@ -488,7 +493,9 @@ static void spice_char_dev_write_retry(void *opaque)
   {
   SpiceCharDeviceState *dev = opaque;
 
  -core-timer_cancel(dev-write_to_dev_timer);
  +if (dev-write_to_dev_timer) {
  +core-timer_cancel(dev-write_to_dev_timer);
  +}
   spice_char_device_write_to_device(dev);
   }
 
  @@ -635,6 +642,7 @@ SpiceCharDeviceState 
  *spice_char_device_state_create(SpiceCharDeviceInstance *si
void *opaque)
   {
   SpiceCharDeviceState *char_dev;
  +SpiceCharDeviceInterface *sif;
 
   spice_assert(sin);
   spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
  
  @@ -652,10 +660,15 @@ SpiceCharDeviceState 
  *spice_char_device_state_create(SpiceCharDeviceInstance *si
   ring_init(char_dev-write_bufs_pool);
   ring_init(char_dev-clients);
 
  -char_dev-write_to_dev_timer = 
  core-timer_add(spice_char_dev_write_retry, char_dev);
  -if (!char_dev-write_to_dev_timer) {
  -spice_error(failed creating char dev write timer);
  +sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, 
  SpiceCharDeviceInterface, base);
  +if ((sif-base.minor_version = 1  sif-base.minor_version = 2) ||
 
  First minor should be major
 
 right, see next comment
 
 
  For the record, this check is wrong in general, it would fail for major
  == 0 and minor == 3. Major version has always been 1, so in this
  specific case it's good enough.
 
 well, in fact the major check is useless, as we error out in
 add_interface() if the major don't match (you can see similar code in
 guest_set_client_capabilities)
 
 So I'll just remove the first major check
 
 
 
  +!(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
  +char_dev-write_to_dev_timer = 
  core-timer_add(spice_char_dev_write_retry, char_dev);
  +if (!char_dev-write_to_dev_timer) {
  +spice_error(failed creating char dev write timer);
  +}
   }
  +
   char_dev-refs = 1;
   sin-st = char_dev;
   spice_debug(sin %p dev_state %p, sin, char_dev);
  @@ -697,7 +710,9 @@ static void 
  spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
   void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
   {
   reds_on_char_device_state_destroy(char_dev);
  -

[Spice-devel] [PATCH spice] chardev: remove write polling

2014-11-24 Thread Marc-André Lureau 
In an effort to reduce the wakeups per second, get rid of the
write_to_dev timer when the implementation supports
SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.

When this flag is set, the frontend instance is responsible for calling
spice_char_device_wakeup() when the device is ready to perform IO.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=912763
---

v3: fix interface version check, spotted by Uri

server/char_device.c | 36 +++-
 server/spice.h   |  9 +++--
 2 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/server/char_device.c b/server/char_device.c
index 6d2339e..5f1b32d 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -438,7 +438,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
 }
 
 spice_char_device_state_ref(dev);
-core-timer_cancel(dev-write_to_dev_timer);
+
+if (dev-write_to_dev_timer) {
+core-timer_cancel(dev-write_to_dev_timer);
+}
 
 sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
 while (dev-running) {
@@ -473,8 +476,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
 /* retry writing as long as the write queue is not empty */
 if (dev-running) {
 if (dev-cur_write_buf) {
-core-timer_start(dev-write_to_dev_timer,
-  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+if (dev-write_to_dev_timer) {
+core-timer_start(dev-write_to_dev_timer,
+  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+}
 } else {
 spice_assert(ring_is_empty(dev-write_queue));
 }
@@ -488,7 +493,9 @@ static void spice_char_dev_write_retry(void *opaque)
 {
 SpiceCharDeviceState *dev = opaque;
 
-core-timer_cancel(dev-write_to_dev_timer);
+if (dev-write_to_dev_timer) {
+core-timer_cancel(dev-write_to_dev_timer);
+}
 spice_char_device_write_to_device(dev);
 }
 
@@ -635,6 +642,7 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
  void *opaque)
 {
 SpiceCharDeviceState *char_dev;
+SpiceCharDeviceInterface *sif;
 
 spice_assert(sin);
 spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
@@ -652,10 +660,15 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
 ring_init(char_dev-write_bufs_pool);
 ring_init(char_dev-clients);
 
-char_dev-write_to_dev_timer = core-timer_add(spice_char_dev_write_retry, 
char_dev);
-if (!char_dev-write_to_dev_timer) {
-spice_error(failed creating char dev write timer);
+sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
+if ((sif-base.minor_version = 1  sif-base.minor_version = 2) ||
+!(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
+char_dev-write_to_dev_timer = 
core-timer_add(spice_char_dev_write_retry, char_dev);
+if (!char_dev-write_to_dev_timer) {
+spice_error(failed creating char dev write timer);
+}
 }
+
 char_dev-refs = 1;
 sin-st = char_dev;
 spice_debug(sin %p dev_state %p, sin, char_dev);
@@ -697,7 +710,9 @@ static void 
spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
 void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
 {
 reds_on_char_device_state_destroy(char_dev);
-core-timer_remove(char_dev-write_to_dev_timer);
+if (char_dev-write_to_dev_timer) {
+core-timer_remove(char_dev-write_to_dev_timer);
+}
 write_buffers_queue_free(char_dev-write_queue);
 write_buffers_queue_free(char_dev-write_bufs_pool);
 if (char_dev-cur_write_buf) {
@@ -805,7 +820,9 @@ void spice_char_device_stop(SpiceCharDeviceState *dev)
 spice_debug(dev_state %p, dev);
 dev-running = FALSE;
 dev-active = FALSE;
-core-timer_cancel(dev-write_to_dev_timer);
+if (dev-write_to_dev_timer) {
+core-timer_cancel(dev-write_to_dev_timer);
+}
 }
 
 void spice_char_device_reset(SpiceCharDeviceState *dev)
@@ -842,6 +859,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
 
 void spice_char_device_wakeup(SpiceCharDeviceState *dev)
 {
+spice_char_device_write_to_device(dev);
 spice_char_device_read_from_device(dev);
 }
 
diff --git a/server/spice.h b/server/spice.h
index 58700d1..bd5bba8 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -24,7 +24,7 @@
 #include spice/vd_agent.h
 #include spice/macros.h
 
-#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */
+#define SPICE_SERVER_VERSION 0x000c06 /* release 0.12.6 */
 
 #ifdef SPICE_SERVER_INTERNAL
 #undef SPICE_GNUC_DEPRECATED
@@ -402,11 +402,15 @@ void spice_server_set_record_rate(SpiceRecordInstance 
*sin, uint32_t frequen
 
 #define SPICE_INTERFACE_CHAR_DEVICE char_device
 #define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
-#define

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-11-24 Thread Christophe Fergeau 
Hey,

On Mon, Nov 24, 2014 at 04:56:26PM +0100, Marc-André Lureau wrote:
 In an effort to reduce the wakeups per second, get rid of the
 write_to_dev timer when the implementation supports
 SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.

Do you know what is the status of getting support for this in QEMU? Did
the char device changes this feature depends on get committed to master?

Christophe


pgpnj_W1ZrDAY.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-11-24 Thread Marc-André Lureau 
Hi

- Original Message -
 Hey,
 
 On Mon, Nov 24, 2014 at 04:56:26PM +0100, Marc-André Lureau wrote:
  In an effort to reduce the wakeups per second, get rid of the
  write_to_dev timer when the implementation supports
  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.
 
 Do you know what is the status of getting support for this in QEMU? Did
 the char device changes this feature depends on get committed to master?

It is in Gerd spice-next branch, I assume it will be merge soon, so it would be 
better to make sure it is indeed in 0.12.6.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-27 Thread Christophe Fergeau 
For reference, the corresponding QEMU patch is
https://lists.nongnu.org/archive/html/qemu-devel/2014-10/msg02939.html
(which depends on 1/2 which in turn depends on another patch)

Christophe

On Fri, Oct 24, 2014 at 12:17:08PM +0200, Marc-André Lureau wrote:
 In an effort to reduce the wakeups per second, get rid of the
 write_to_dev timer when the implementation supports
 SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.
 
 When this flag is set, the frontend instance is responsible for calling
 spice_char_device_wakeup() when the device is ready to perform IO.
 
 Related to:
 https://bugzilla.redhat.com/show_bug.cgi?id=912763
 ---
 
 in v2:
 - Check char device version before accessing the flags field.
 
  server/char_device.c | 28 +---
  server/spice.h   |  9 +++--
  2 files changed, 28 insertions(+), 9 deletions(-)
 
 diff --git a/server/char_device.c b/server/char_device.c
 index 6d2339e..487a5c5 100644
 --- a/server/char_device.c
 +++ b/server/char_device.c
 @@ -438,7 +438,10 @@ static int 
 spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  }
  
  spice_char_device_state_ref(dev);
 -core-timer_cancel(dev-write_to_dev_timer);
 +
 +if (dev-write_to_dev_timer) {
 +core-timer_cancel(dev-write_to_dev_timer);
 +}
  
  sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
 base);
  while (dev-running) {
 @@ -473,8 +476,10 @@ static int 
 spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  /* retry writing as long as the write queue is not empty */
  if (dev-running) {
  if (dev-cur_write_buf) {
 -core-timer_start(dev-write_to_dev_timer,
 -  CHAR_DEVICE_WRITE_TO_TIMEOUT);
 +if (dev-write_to_dev_timer) {
 +core-timer_start(dev-write_to_dev_timer,
 +  CHAR_DEVICE_WRITE_TO_TIMEOUT);
 +}
  } else {
  spice_assert(ring_is_empty(dev-write_queue));
  }
 @@ -635,6 +640,7 @@ SpiceCharDeviceState 
 *spice_char_device_state_create(SpiceCharDeviceInstance *si
   void *opaque)
  {
  SpiceCharDeviceState *char_dev;
 +SpiceCharDeviceInterface *sif;
  
  spice_assert(sin);
  spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
 @@ -652,10 +658,15 @@ SpiceCharDeviceState 
 *spice_char_device_state_create(SpiceCharDeviceInstance *si
  ring_init(char_dev-write_bufs_pool);
  ring_init(char_dev-clients);
  
 -char_dev-write_to_dev_timer = 
 core-timer_add(spice_char_dev_write_retry, char_dev);
 -if (!char_dev-write_to_dev_timer) {
 -spice_error(failed creating char dev write timer);
 +sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, 
 SpiceCharDeviceInterface, base);
 +if (sif-base.minor_version = 1  sif-base.minor_version = 3
 + !(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
 +char_dev-write_to_dev_timer = 
 core-timer_add(spice_char_dev_write_retry, char_dev);
 +if (!char_dev-write_to_dev_timer) {
 +spice_error(failed creating char dev write timer);
 +}
  }
 +
  char_dev-refs = 1;
  sin-st = char_dev;
  spice_debug(sin %p dev_state %p, sin, char_dev);
 @@ -697,7 +708,9 @@ static void 
 spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
  void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
  {
  reds_on_char_device_state_destroy(char_dev);
 -core-timer_remove(char_dev-write_to_dev_timer);
 +if (char_dev-write_to_dev_timer) {
 +core-timer_remove(char_dev-write_to_dev_timer);
 +}
  write_buffers_queue_free(char_dev-write_queue);
  write_buffers_queue_free(char_dev-write_bufs_pool);
  if (char_dev-cur_write_buf) {
 @@ -842,6 +855,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
  
  void spice_char_device_wakeup(SpiceCharDeviceState *dev)
  {
 +spice_char_device_write_to_device(dev);
  spice_char_device_read_from_device(dev);
  }
  
 diff --git a/server/spice.h b/server/spice.h
 index 58700d1..bd5bba8 100644
 --- a/server/spice.h
 +++ b/server/spice.h
 @@ -24,7 +24,7 @@
  #include spice/vd_agent.h
  #include spice/macros.h
  
 -#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */
 +#define SPICE_SERVER_VERSION 0x000c06 /* release 0.12.6 */
  
  #ifdef SPICE_SERVER_INTERNAL
  #undef SPICE_GNUC_DEPRECATED
 @@ -402,11 +402,15 @@ void 
 spice_server_set_record_rate(SpiceRecordInstance *sin, uint32_t frequen
  
  #define SPICE_INTERFACE_CHAR_DEVICE char_device
  #define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
 -#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 2
 +#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 3
  typedef struct SpiceCharDeviceInterface SpiceCharDeviceInterface;
  typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
  typedef struct SpiceCharDeviceState SpiceCharDeviceState;
  
 +typedef enum {
 +

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-27 Thread Christophe Fergeau 
On Fri, Oct 24, 2014 at 12:17:08PM +0200, Marc-André Lureau wrote:
  void spice_char_device_wakeup(SpiceCharDeviceState *dev)
  {
 +spice_char_device_write_to_device(dev);
  spice_char_device_read_from_device(dev);

What impact does this change have on existing users of this function?

Christophe


pgpdnaIcbv_lH.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-27 Thread Marc-André Lureau 
On Mon, Oct 27, 2014 at 11:26 AM, Christophe Fergeau cferg...@redhat.com
wrote:

 What impact does this change have on existing users of this function?


None, if there is nothing to write. If there is something to write, it will
write now instead of waiting for the timer afew ms later.


-- 
Marc-André Lureau
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-27 Thread Uri Lublin 

Hi Marc-Andre,

On 10/24/2014 01:17 PM, Marc-André Lureau wrote:

In an effort to reduce the wakeups per second, get rid of the
write_to_dev timer when the implementation supports
SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.

When this flag is set, the frontend instance is responsible for calling
spice_char_device_wakeup() when the device is ready to perform IO.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=912763
---

in v2:
- Check char device version before accessing the flags field.

  server/char_device.c | 28 +---
  server/spice.h   |  9 +++--
  2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/server/char_device.c b/server/char_device.c
index 6d2339e..487a5c5 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -438,7 +438,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  }
  
  spice_char_device_state_ref(dev);

-core-timer_cancel(dev-write_to_dev_timer);
+
+if (dev-write_to_dev_timer) {
+core-timer_cancel(dev-write_to_dev_timer);
+}
  
  sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, base);

  while (dev-running) {
@@ -473,8 +476,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
  /* retry writing as long as the write queue is not empty */
  if (dev-running) {
  if (dev-cur_write_buf) {
-core-timer_start(dev-write_to_dev_timer,
-  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+if (dev-write_to_dev_timer) {
+core-timer_start(dev-write_to_dev_timer,
+  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+}
  } else {
  spice_assert(ring_is_empty(dev-write_queue));
  }
@@ -635,6 +640,7 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
   void *opaque)
  {
  SpiceCharDeviceState *char_dev;
+SpiceCharDeviceInterface *sif;
  
  spice_assert(sin);

  spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
@@ -652,10 +658,15 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
  ring_init(char_dev-write_bufs_pool);
  ring_init(char_dev-clients);
  
-char_dev-write_to_dev_timer = core-timer_add(spice_char_dev_write_retry, char_dev);

-if (!char_dev-write_to_dev_timer) {
-spice_error(failed creating char dev write timer);
+sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
+if (sif-base.minor_version = 1  sif-base.minor_version = 3
+ !(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {


The condition here seems wrong:
a) base.MAJOR_version =1
b) if e.g. sif.base.minor_version == 2, you want to use the timer, right ?

Uri


+char_dev-write_to_dev_timer = 
core-timer_add(spice_char_dev_write_retry, char_dev);
+if (!char_dev-write_to_dev_timer) {
+spice_error(failed creating char dev write timer);
+}
  }
+
  char_dev-refs = 1;
  sin-st = char_dev;
  spice_debug(sin %p dev_state %p, sin, char_dev);
@@ -697,7 +708,9 @@ static void 
spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
  void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
  {
  reds_on_char_device_state_destroy(char_dev);
-core-timer_remove(char_dev-write_to_dev_timer);
+if (char_dev-write_to_dev_timer) {
+core-timer_remove(char_dev-write_to_dev_timer);
+}
  write_buffers_queue_free(char_dev-write_queue);
  write_buffers_queue_free(char_dev-write_bufs_pool);
  if (char_dev-cur_write_buf) {
@@ -842,6 +855,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
  
  void spice_char_device_wakeup(SpiceCharDeviceState *dev)

  {
+spice_char_device_write_to_device(dev);
  spice_char_device_read_from_device(dev);
  }
  
diff --git a/server/spice.h b/server/spice.h

index 58700d1..bd5bba8 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -24,7 +24,7 @@
  #include spice/vd_agent.h
  #include spice/macros.h
  
-#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */

+#define SPICE_SERVER_VERSION 0x000c06 /* release 0.12.6 */
  
  #ifdef SPICE_SERVER_INTERNAL

  #undef SPICE_GNUC_DEPRECATED
@@ -402,11 +402,15 @@ void spice_server_set_record_rate(SpiceRecordInstance 
*sin, uint32_t frequen
  
  #define SPICE_INTERFACE_CHAR_DEVICE char_device

  #define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
-#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 2
+#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 3
  typedef struct SpiceCharDeviceInterface SpiceCharDeviceInterface;
  typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
  typedef struct SpiceCharDeviceState SpiceCharDeviceState;
  
+typedef enum {

+SPICE_CHAR_DEVICE_NOTIFY_WRITABLE = 1  0,
+} spice_char_device_flags;
+
  struct SpiceCharDeviceInterface {
  SpiceBaseInterface

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-27 Thread Marc-André Lureau 


- Mail original -
 Hi Marc-Andre,
 
 On 10/24/2014 01:17 PM, Marc-André Lureau wrote:
  In an effort to reduce the wakeups per second, get rid of the
  write_to_dev timer when the implementation supports
  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.
 
  When this flag is set, the frontend instance is responsible for calling
  spice_char_device_wakeup() when the device is ready to perform IO.
 
  Related to:
  https://bugzilla.redhat.com/show_bug.cgi?id=912763
  ---
 
  in v2:
  - Check char device version before accessing the flags field.
 
server/char_device.c | 28 +---
server/spice.h   |  9 +++--
2 files changed, 28 insertions(+), 9 deletions(-)
 
  diff --git a/server/char_device.c b/server/char_device.c
  index 6d2339e..487a5c5 100644
  --- a/server/char_device.c
  +++ b/server/char_device.c
  @@ -438,7 +438,10 @@ static int
  spice_char_device_write_to_device(SpiceCharDeviceState *dev)
}

spice_char_device_state_ref(dev);
  -core-timer_cancel(dev-write_to_dev_timer);
  +
  +if (dev-write_to_dev_timer) {
  +core-timer_cancel(dev-write_to_dev_timer);
  +}

sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface,
base);
while (dev-running) {
  @@ -473,8 +476,10 @@ static int
  spice_char_device_write_to_device(SpiceCharDeviceState *dev)
/* retry writing as long as the write queue is not empty */
if (dev-running) {
if (dev-cur_write_buf) {
  -core-timer_start(dev-write_to_dev_timer,
  -  CHAR_DEVICE_WRITE_TO_TIMEOUT);
  +if (dev-write_to_dev_timer) {
  +core-timer_start(dev-write_to_dev_timer,
  +  CHAR_DEVICE_WRITE_TO_TIMEOUT);
  +}
} else {
spice_assert(ring_is_empty(dev-write_queue));
}
  @@ -635,6 +640,7 @@ SpiceCharDeviceState
  *spice_char_device_state_create(SpiceCharDeviceInstance *si
 void *opaque)
{
SpiceCharDeviceState *char_dev;
  +SpiceCharDeviceInterface *sif;

spice_assert(sin);
spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client

  @@ -652,10 +658,15 @@ SpiceCharDeviceState
  *spice_char_device_state_create(SpiceCharDeviceInstance *si
ring_init(char_dev-write_bufs_pool);
ring_init(char_dev-clients);

  -char_dev-write_to_dev_timer =
  core-timer_add(spice_char_dev_write_retry, char_dev);
  -if (!char_dev-write_to_dev_timer) {
  -spice_error(failed creating char dev write timer);
  +sif = SPICE_CONTAINEROF(char_dev-sin-base.sif,
  SpiceCharDeviceInterface, base);
  +if (sif-base.minor_version = 1  sif-base.minor_version = 3
  + !(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
 
 The condition here seems wrong:
 a) base.MAJOR_version =1
 b) if e.g. sif.base.minor_version == 2, you want to use the timer, right ?

oops, you are correct, fixing that

 
 Uri
 
  +char_dev-write_to_dev_timer =
  core-timer_add(spice_char_dev_write_retry, char_dev);
  +if (!char_dev-write_to_dev_timer) {
  +spice_error(failed creating char dev write timer);
  +}
}
  +
char_dev-refs = 1;
sin-st = char_dev;
spice_debug(sin %p dev_state %p, sin, char_dev);
  @@ -697,7 +708,9 @@ static void
  spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
{
reds_on_char_device_state_destroy(char_dev);
  -core-timer_remove(char_dev-write_to_dev_timer);
  +if (char_dev-write_to_dev_timer) {
  +core-timer_remove(char_dev-write_to_dev_timer);
  +}
write_buffers_queue_free(char_dev-write_queue);
write_buffers_queue_free(char_dev-write_bufs_pool);
if (char_dev-cur_write_buf) {
  @@ -842,6 +855,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)

void spice_char_device_wakeup(SpiceCharDeviceState *dev)
{
  +spice_char_device_write_to_device(dev);
spice_char_device_read_from_device(dev);
}

  diff --git a/server/spice.h b/server/spice.h
  index 58700d1..bd5bba8 100644
  --- a/server/spice.h
  +++ b/server/spice.h
  @@ -24,7 +24,7 @@
#include spice/vd_agent.h
#include spice/macros.h

  -#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */
  +#define SPICE_SERVER_VERSION 0x000c06 /* release 0.12.6 */

#ifdef SPICE_SERVER_INTERNAL
#undef SPICE_GNUC_DEPRECATED
  @@ -402,11 +402,15 @@ void
  spice_server_set_record_rate(SpiceRecordInstance *sin, uint32_t
  frequen

#define SPICE_INTERFACE_CHAR_DEVICE char_device
#define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
  -#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 2
  +#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 3
typedef struct SpiceCharDeviceInterface

[Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-24 Thread Marc-André Lureau 
In an effort to reduce the wakeups per second, get rid of the
write_to_dev timer when the implementation supports
SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.

When this flag is set, the frontend instance is responsible for calling
spice_char_device_wakeup() when the device is ready to perform IO.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=912763
---
 server/char_device.c | 27 ---
 server/spice.h   |  7 ++-
 2 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/server/char_device.c b/server/char_device.c
index 6d2339e..5bb2a3c 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -438,7 +438,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
 }
 
 spice_char_device_state_ref(dev);
-core-timer_cancel(dev-write_to_dev_timer);
+
+if (dev-write_to_dev_timer) {
+core-timer_cancel(dev-write_to_dev_timer);
+}
 
 sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
 while (dev-running) {
@@ -473,8 +476,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
 /* retry writing as long as the write queue is not empty */
 if (dev-running) {
 if (dev-cur_write_buf) {
-core-timer_start(dev-write_to_dev_timer,
-  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+if (dev-write_to_dev_timer) {
+core-timer_start(dev-write_to_dev_timer,
+  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+}
 } else {
 spice_assert(ring_is_empty(dev-write_queue));
 }
@@ -635,6 +640,7 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
  void *opaque)
 {
 SpiceCharDeviceState *char_dev;
+SpiceCharDeviceInterface *sif;
 
 spice_assert(sin);
 spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
@@ -652,10 +658,14 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
 ring_init(char_dev-write_bufs_pool);
 ring_init(char_dev-clients);
 
-char_dev-write_to_dev_timer = core-timer_add(spice_char_dev_write_retry, 
char_dev);
-if (!char_dev-write_to_dev_timer) {
-spice_error(failed creating char dev write timer);
+sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
+if (!(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
+char_dev-write_to_dev_timer = 
core-timer_add(spice_char_dev_write_retry, char_dev);
+if (!char_dev-write_to_dev_timer) {
+spice_error(failed creating char dev write timer);
+}
 }
+
 char_dev-refs = 1;
 sin-st = char_dev;
 spice_debug(sin %p dev_state %p, sin, char_dev);
@@ -697,7 +707,9 @@ static void 
spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
 void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
 {
 reds_on_char_device_state_destroy(char_dev);
-core-timer_remove(char_dev-write_to_dev_timer);
+if (char_dev-write_to_dev_timer) {
+core-timer_remove(char_dev-write_to_dev_timer);
+}
 write_buffers_queue_free(char_dev-write_queue);
 write_buffers_queue_free(char_dev-write_bufs_pool);
 if (char_dev-cur_write_buf) {
@@ -842,6 +854,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
 
 void spice_char_device_wakeup(SpiceCharDeviceState *dev)
 {
+spice_char_device_write_to_device(dev);
 spice_char_device_read_from_device(dev);
 }
 
diff --git a/server/spice.h b/server/spice.h
index 58700d1..6449f42 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -24,7 +24,7 @@
 #include spice/vd_agent.h
 #include spice/macros.h
 
-#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */
+#define SPICE_SERVER_VERSION 0x000c06 /* release 0.12.6 */
 
 #ifdef SPICE_SERVER_INTERNAL
 #undef SPICE_GNUC_DEPRECATED
@@ -407,6 +407,10 @@ typedef struct SpiceCharDeviceInterface 
SpiceCharDeviceInterface;
 typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
 typedef struct SpiceCharDeviceState SpiceCharDeviceState;
 
+typedef enum {
+SPICE_CHAR_DEVICE_NOTIFY_WRITABLE = 1  0,
+} spice_char_device_flags;
+
 struct SpiceCharDeviceInterface {
 SpiceBaseInterface base;
 
@@ -414,6 +418,7 @@ struct SpiceCharDeviceInterface {
 int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len);
 int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
 void (*event)(SpiceCharDeviceInstance *sin, uint8_t event);
+spice_char_device_flags flags;
 };
 
 struct SpiceCharDeviceInstance {
-- 
1.9.3

___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-24 Thread Christophe Fergeau 
On Fri, Oct 24, 2014 at 10:54:24AM +0200, Marc-André Lureau wrote:
 @@ -407,6 +407,10 @@ typedef struct SpiceCharDeviceInterface 
 SpiceCharDeviceInterface;
  typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
  typedef struct SpiceCharDeviceState SpiceCharDeviceState;
  
 +typedef enum {
 +SPICE_CHAR_DEVICE_NOTIFY_WRITABLE = 1  0,
 +} spice_char_device_flags;
 +
  struct SpiceCharDeviceInterface {
  SpiceBaseInterface base;
  
 @@ -414,6 +418,7 @@ struct SpiceCharDeviceInterface {
  int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len);
  int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
  void (*event)(SpiceCharDeviceInstance *sin, uint8_t event);
 +spice_char_device_flags flags;
  };

QEMU uses a static SpiceCharDeviceInterface:

static SpiceCharDeviceInterface vmc_interface = {
.base.type  = SPICE_INTERFACE_CHAR_DEVICE,
.base.description   = spice virtual channel char device,
.base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
.base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
.state  = vmc_state,
.write  = vmc_write,
.read   = vmc_read,
#if SPICE_SERVER_VERSION = 0x000c02
.event  = vmc_event,
#endif
};

If we are using a QEMU version compiled against an older spice-server, and then
upgrade spice-server but not QEMU, I don't think accessing that new 'flags'
filed is going to work (in other words, this change seems to be an ABI break).

Christophe


pgprssEkmTd2R.pgp
Description: PGP signature
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

Re: [Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-24 Thread Marc-André Lureau 


- Original Message -
 On Fri, Oct 24, 2014 at 10:54:24AM +0200, Marc-André Lureau wrote:
  @@ -407,6 +407,10 @@ typedef struct SpiceCharDeviceInterface
  SpiceCharDeviceInterface;
   typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
   typedef struct SpiceCharDeviceState SpiceCharDeviceState;
   
  +typedef enum {
  +SPICE_CHAR_DEVICE_NOTIFY_WRITABLE = 1  0,
  +} spice_char_device_flags;
  +
   struct SpiceCharDeviceInterface {
   SpiceBaseInterface base;
   
  @@ -414,6 +418,7 @@ struct SpiceCharDeviceInterface {
   int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int
   len);
   int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
   void (*event)(SpiceCharDeviceInstance *sin, uint8_t event);
  +spice_char_device_flags flags;
   };
 
 QEMU uses a static SpiceCharDeviceInterface:
 
 static SpiceCharDeviceInterface vmc_interface = {
 .base.type  = SPICE_INTERFACE_CHAR_DEVICE,
 .base.description   = spice virtual channel char device,
 .base.major_version = SPICE_INTERFACE_CHAR_DEVICE_MAJOR,
 .base.minor_version = SPICE_INTERFACE_CHAR_DEVICE_MINOR,
 .state  = vmc_state,
 .write  = vmc_write,
 .read   = vmc_read,
 #if SPICE_SERVER_VERSION = 0x000c02
 .event  = vmc_event,
 #endif
 };
 
 If we are using a QEMU version compiled against an older spice-server, and
 then
 upgrade spice-server but not QEMU, I don't think accessing that new 'flags'
 filed is going to work (in other words, this change seems to be an ABI
 break).
 

right, I had already made a change locally to use the interface version 
(although
I think we shouldn't be using that, but instead use regular library versioning)
Sending now.
___
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel

[Spice-devel] [PATCH spice] chardev: remove write polling

2014-10-24 Thread Marc-André Lureau 
In an effort to reduce the wakeups per second, get rid of the
write_to_dev timer when the implementation supports
SPICE_CHAR_DEVICE_NOTIFY_WRITABLE.

When this flag is set, the frontend instance is responsible for calling
spice_char_device_wakeup() when the device is ready to perform IO.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=912763
---

in v2:
- Check char device version before accessing the flags field.

 server/char_device.c | 28 +---
 server/spice.h   |  9 +++--
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/server/char_device.c b/server/char_device.c
index 6d2339e..487a5c5 100644
--- a/server/char_device.c
+++ b/server/char_device.c
@@ -438,7 +438,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
 }
 
 spice_char_device_state_ref(dev);
-core-timer_cancel(dev-write_to_dev_timer);
+
+if (dev-write_to_dev_timer) {
+core-timer_cancel(dev-write_to_dev_timer);
+}
 
 sif = SPICE_CONTAINEROF(dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
 while (dev-running) {
@@ -473,8 +476,10 @@ static int 
spice_char_device_write_to_device(SpiceCharDeviceState *dev)
 /* retry writing as long as the write queue is not empty */
 if (dev-running) {
 if (dev-cur_write_buf) {
-core-timer_start(dev-write_to_dev_timer,
-  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+if (dev-write_to_dev_timer) {
+core-timer_start(dev-write_to_dev_timer,
+  CHAR_DEVICE_WRITE_TO_TIMEOUT);
+}
 } else {
 spice_assert(ring_is_empty(dev-write_queue));
 }
@@ -635,6 +640,7 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
  void *opaque)
 {
 SpiceCharDeviceState *char_dev;
+SpiceCharDeviceInterface *sif;
 
 spice_assert(sin);
 spice_assert(cbs-read_one_msg_from_device  cbs-ref_msg_to_client 
@@ -652,10 +658,15 @@ SpiceCharDeviceState 
*spice_char_device_state_create(SpiceCharDeviceInstance *si
 ring_init(char_dev-write_bufs_pool);
 ring_init(char_dev-clients);
 
-char_dev-write_to_dev_timer = core-timer_add(spice_char_dev_write_retry, 
char_dev);
-if (!char_dev-write_to_dev_timer) {
-spice_error(failed creating char dev write timer);
+sif = SPICE_CONTAINEROF(char_dev-sin-base.sif, SpiceCharDeviceInterface, 
base);
+if (sif-base.minor_version = 1  sif-base.minor_version = 3
+ !(sif-flags  SPICE_CHAR_DEVICE_NOTIFY_WRITABLE)) {
+char_dev-write_to_dev_timer = 
core-timer_add(spice_char_dev_write_retry, char_dev);
+if (!char_dev-write_to_dev_timer) {
+spice_error(failed creating char dev write timer);
+}
 }
+
 char_dev-refs = 1;
 sin-st = char_dev;
 spice_debug(sin %p dev_state %p, sin, char_dev);
@@ -697,7 +708,9 @@ static void 
spice_char_device_state_unref(SpiceCharDeviceState *char_dev)
 void spice_char_device_state_destroy(SpiceCharDeviceState *char_dev)
 {
 reds_on_char_device_state_destroy(char_dev);
-core-timer_remove(char_dev-write_to_dev_timer);
+if (char_dev-write_to_dev_timer) {
+core-timer_remove(char_dev-write_to_dev_timer);
+}
 write_buffers_queue_free(char_dev-write_queue);
 write_buffers_queue_free(char_dev-write_bufs_pool);
 if (char_dev-cur_write_buf) {
@@ -842,6 +855,7 @@ void spice_char_device_reset(SpiceCharDeviceState *dev)
 
 void spice_char_device_wakeup(SpiceCharDeviceState *dev)
 {
+spice_char_device_write_to_device(dev);
 spice_char_device_read_from_device(dev);
 }
 
diff --git a/server/spice.h b/server/spice.h
index 58700d1..bd5bba8 100644
--- a/server/spice.h
+++ b/server/spice.h
@@ -24,7 +24,7 @@
 #include spice/vd_agent.h
 #include spice/macros.h
 
-#define SPICE_SERVER_VERSION 0x000c05 /* release 0.12.5 */
+#define SPICE_SERVER_VERSION 0x000c06 /* release 0.12.6 */
 
 #ifdef SPICE_SERVER_INTERNAL
 #undef SPICE_GNUC_DEPRECATED
@@ -402,11 +402,15 @@ void spice_server_set_record_rate(SpiceRecordInstance 
*sin, uint32_t frequen
 
 #define SPICE_INTERFACE_CHAR_DEVICE char_device
 #define SPICE_INTERFACE_CHAR_DEVICE_MAJOR 1
-#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 2
+#define SPICE_INTERFACE_CHAR_DEVICE_MINOR 3
 typedef struct SpiceCharDeviceInterface SpiceCharDeviceInterface;
 typedef struct SpiceCharDeviceInstance SpiceCharDeviceInstance;
 typedef struct SpiceCharDeviceState SpiceCharDeviceState;
 
+typedef enum {
+SPICE_CHAR_DEVICE_NOTIFY_WRITABLE = 1  0,
+} spice_char_device_flags;
+
 struct SpiceCharDeviceInterface {
 SpiceBaseInterface base;
 
@@ -414,6 +418,7 @@ struct SpiceCharDeviceInterface {
 int (*write)(SpiceCharDeviceInstance *sin, const uint8_t *buf, int len);
 int (*read)(SpiceCharDeviceInstance *sin, uint8_t *buf, int len);
 void (*event)(SpiceCharDeviceInstance *sin, uint8_t