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 <cfergeau@xxxxxxxxxx> 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); > >> - 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. But they also have to make assumptions that the function they call will do 'something' or else there is no point calling it. Then they should expect that the function will change in minor ways during the library life time. What is a minor change/a major change is then open for debate ;) ACK with the major version part fixed. Christophe
Attachment:
pgpZzaa4Cfoba.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel