[Spice-devel] [PATCH spice] chardev: remove write polling
Christophe Fergeau
cfergeau at redhat.com
Tue Nov 25 05:11:19 PST 2014
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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/spice-devel/attachments/20141125/61d2af76/attachment-0001.sig>
More information about the Spice-devel
mailing list