Re: [spice-gtk PATCH v6 2/2] channel-usbredir: drop isoc packets on low bandwidth

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Oct 23, 2015 at 9:53 AM, Victor Toso <victortoso@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> 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
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]