[Spice-devel] [spice-gtk PATCH v2] spice-channel: check message queue length

[Victor Toso]

CC hans about this one as well.

Btw, this was tested on windows client (related to rhbz#1201387) and it
seems to improve well.

https://bugzilla.redhat.com/show_bug.cgi?id=1201387

Thanks for any input,

On Fri, Oct 09, 2015 at 02:59:05PM +0200, Victor Toso 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.
> 
> In the case of usbredir, video devices on high latency can easily
> trigger this situation.
> 
> An easy way to test locally is sharing and webcam and simulating high
> latency 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   |  7 +++++++
>  src/spice-channel-priv.h |  2 ++
>  src/spice-channel.c      | 27 ++++++++++++++++++++++++++-
>  3 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/src/channel-usbredir.c b/src/channel-usbredir.c
> index 89f5c9d..fb7bd07 100644
> --- a/src/channel-usbredir.c
> +++ b/src/channel-usbredir.c
> @@ -475,6 +475,13 @@ static void usbredir_write_flush_callback(void *user_data)
>      if (!priv->host)
>          return;
>  
> +    if (spice_channel_has_full_queue (SPICE_CHANNEL(channel))) {
> +        g_warning ("device %04x:%04x is loosing data due high traffic",
> +                   spice_usb_device_get_vid(priv->spice_device),
> +                   spice_usb_device_get_pid(priv->spice_device));
> +        return;
> +    }
> +
>      usbredirhost_write_guest_data(priv->host);
>  }
>  
> diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> index 436a521..ae8d01e 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                     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);
> +gboolean spice_channel_has_full_queue (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..d574b90 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->queue_size = (was_empty) ? size : c->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->queue_size = (c->queue_size < size) ? 0 : c->queue_size - size;
>              spice_channel_write_msg(channel, out);
> +        }
>      } while (out);
>  
>      spice_channel_flushed(channel, TRUE);
> @@ -2813,6 +2819,25 @@ enum spice_channel_state spice_channel_get_state(SpiceChannel *channel)
>      return channel->priv->state;
>  }
>  
> +#define QUEUE_MEMORY_LIMIT_MB   10
> +
> +G_GNUC_INTERNAL
> +gboolean spice_channel_has_full_queue (SpiceChannel *channel)
> +{
> +    SpiceChannelPrivate *c = channel->priv;
> +    gdouble ret_mb;
> +    guint mb_limit;
> +    const gchar *env = g_getenv("SPICE_CHANNEL_QUEUE_SIZE");
> +    mb_limit = (env == NULL) ? QUEUE_MEMORY_LIMIT_MB : strtoul(env, NULL, 10);
> +
> +    STATIC_MUTEX_LOCK(c->xmit_queue_lock);
> +    ret_mb = c->queue_size/1024.0/1024.0;
> +    STATIC_MUTEX_UNLOCK(c->xmit_queue_lock);
> +    CHANNEL_DEBUG(channel, "queue length: %u with total size: %4.4f MB (max = %u MB)",
> +                  g_queue_get_length(&c->xmit_queue), ret_mb, mb_limit);
> +    return (ret_mb > mb_limit);
> +}
> +
>  G_GNUC_INTERNAL
>  void spice_channel_swap(SpiceChannel *channel, SpiceChannel *swap, gboolean swap_msgs)
>  {
> -- 
> 2.5.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel