[Spice-devel] [PATCH spice] chardev: remove write polling
Marc-André Lureau
marcandre.lureau at gmail.com
Tue Nov 25 05:49:07 PST 2014
On Tue, Nov 25, 2014 at 2:11 PM, Christophe Fergeau <cfergeau at 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;
>> - 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...
I understand we can imagine bad things would happen, so we could
introduce a new function, but I don't think that's necessary either.
People should not make too much assumption about what can happen when
calling a library function.
>
>>
>> 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
--
Marc-André Lureau
More information about the Spice-devel
mailing list