[Spice-devel] [spice-gtk PATCH v6 2/2] channel-usbredir: drop isoc packets on low bandwidth
Fabiano Fidêncio
fabiano at fidencio.org
Fri Oct 23 01:59:23 PDT 2015
On Fri, Oct 23, 2015 at 9:53 AM, Victor Toso <victortoso at redhat.com> wrote:
> When channel wants to send much more data then the wire can handle, the
> queue grows fast. This patch does not limit the queue growth but
> introduces an internal API to check if queue size is too big.
>
> This internal API is used in usbredir_buffered_output_size_callback
> which is called before any isoc pacaket is queued in usbredir. The
> usbredir implements the logic to:
> - only drop isoc packets
> - while dropping packtes does still give us video frames from and above
> 10fps streams
>
> An easy way to test locally is sharing and webcam and with tc:
> tc qdisc add dev lo root netem delay 100ms
> tc qdisc change dev lo root netem delay 1000ms
> tc qdisc del dev lo root netem
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1264156
> ---
> src/channel-usbredir.c | 9 +++++++++
> src/spice-channel-priv.h | 2 ++
> src/spice-channel.c | 19 ++++++++++++++++++-
> 3 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 89f5c9d..dbcf0af 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -91,6 +91,7 @@ static void usbredir_log(void *user_data, int level, const char *msg);
> static int usbredir_read_callback(void *user_data, uint8_t *data, int count);
> static int usbredir_write_callback(void *user_data, uint8_t *data, int count);
> static void usbredir_write_flush_callback(void *user_data);
> +static uint64_t usbredir_buffered_output_size_callback(void *user_data);
>
> static void *usbredir_alloc_lock(void);
> static void usbredir_lock_lock(void *user_data);
> @@ -224,6 +225,8 @@ void spice_usbredir_channel_set_context(SpiceUsbredirChannel *channel,
> usbredirhost_fl_write_cb_owns_buffer);
> if (!priv->host)
> g_error("Out of memory allocating usbredirhost");
> +
> + usbredirhost_set_buffered_output_size_cb(priv->host, usbredir_buffered_output_size_callback);
> }
>
> static gboolean spice_usbredir_channel_open_device(
> @@ -461,6 +464,12 @@ void spice_usbredir_channel_get_guest_filter(
> /* ------------------------------------------------------------------ */
> /* callbacks (any context) */
>
> +static uint64_t usbredir_buffered_output_size_callback(void *user_data)
> +{
> + g_return_val_if_fail(SPICE_IS_USBREDIR_CHANNEL(user_data), 0);
> + return spice_channel_get_queue_size(SPICE_CHANNEL(user_data));
> +}
> +
> /* Note that this function must be re-entrant safe, as it can get called
> from both the main thread as well as from the usb event handling thread */
> static void usbredir_write_flush_callback(void *user_data)
> diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> index 436a521..4b2d1e6 100644
> --- a/src/spice-channel-priv.h
> +++ b/src/spice-channel-priv.h
> @@ -111,6 +111,7 @@ struct _SpiceChannelPrivate {
> gboolean xmit_queue_blocked;
> STATIC_MUTEX xmit_queue_lock;
> guint xmit_queue_wakeup_id;
> + guint64 xmit_queue_size;
>
> char name[16];
> enum spice_channel_state state;
> @@ -171,6 +172,7 @@ void spice_channel_wakeup(SpiceChannel *channel, gboolean cancel);
>
> SpiceSession* spice_channel_get_session(SpiceChannel *channel);
> enum spice_channel_state spice_channel_get_state(SpiceChannel *channel);
> +guint64 spice_channel_get_queue_size (SpiceChannel *channel);
>
> /* coroutine context */
> typedef void (*handler_msg_in)(SpiceChannel *channel, SpiceMsgIn *msg, gpointer data);
> diff --git a/src/spice-channel.c b/src/spice-channel.c
> index 2ce52c7..8b6125f 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -697,10 +697,12 @@ void spice_msg_out_send(SpiceMsgOut *out)
> {
> SpiceChannelPrivate *c;
> gboolean was_empty;
> + guint32 size;
>
> g_return_if_fail(out != NULL);
> g_return_if_fail(out->channel != NULL);
> c = out->channel->priv;
> + size = spice_marshaller_get_total_size(out->marshaller);
>
> STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> if (c->xmit_queue_blocked) {
> @@ -710,6 +712,7 @@ void spice_msg_out_send(SpiceMsgOut *out)
>
> was_empty = g_queue_is_empty(&c->xmit_queue);
> g_queue_push_tail(&c->xmit_queue, out);
> + c->xmit_queue_size = (was_empty) ? size : c->xmit_queue_size + size;
>
> /* One wakeup is enough to empty the entire queue -> only do a wakeup
> if the queue was empty, and there isn't one pending already. */
> @@ -2104,8 +2107,11 @@ static void spice_channel_iterate_write(SpiceChannel *channel)
> STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> out = g_queue_pop_head(&c->xmit_queue);
> STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
> - if (out)
> + if (out) {
> + guint32 size = spice_marshaller_get_total_size(out->marshaller);
> + c->xmit_queue_size = (c->xmit_queue_size < size) ? 0 : c->xmit_queue_size - size;
> spice_channel_write_msg(channel, out);
> + }
> } while (out);
>
> spice_channel_flushed(channel, TRUE);
> @@ -2814,6 +2820,17 @@ enum spice_channel_state spice_channel_get_state(SpiceChannel *channel)
> }
>
> G_GNUC_INTERNAL
> +guint64 spice_channel_get_queue_size (SpiceChannel *channel)
> +{
> + guint64 size;
> + SpiceChannelPrivate *c = channel->priv;
> + STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> + size = c->xmit_queue_size;
> + STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
> + return size;
> +}
> +
> +G_GNUC_INTERNAL
> void spice_channel_swap(SpiceChannel *channel, SpiceChannel *swap, gboolean swap_msgs)
> {
> SpiceChannelPrivate *c = channel->priv;
> --
> 2.5.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
I didn't go through the code and I will leave it for Hans.
But as you're depending on a new added API on usbredir would be nice
if you could also bump the dep version on configure.ac and then use it
conditionally on spice-gtk.
One of the ways for doing it is basically what Jonathon did for
virt-viewer here:
https://www.redhat.com/archives/virt-tools-list/2015-October/msg00039.html
Best Regards,
--
Fabiano Fidêncio
More information about the Spice-devel
mailing list