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 read-only function to be read and write, hopefully code does not rely on the read-only behaviour... > > 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 { > -- > 2.1.0 > > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
pgpCcfidCVZiY.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel