Re: [PATCH spice] chardev: remove write polling

[Uri Lublin <uril@xxxxxxxxxx>]

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

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