[Spice-devel,spice-gtk,v1] spice-channel: check message queue length

Submitted by Victor Toso on Oct. 5, 2015, 3:10 p.m.

Details

Message ID 1444057812-25749-2-git-send-email-victortoso@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Victor Toso Oct. 5, 2015, 3:10 p.m.
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 length 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 150ms
    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 | 1 +
 src/spice-channel.c      | 9 +++++++++
 3 files changed, 17 insertions(+)

Patch hide | download patch | download mbox

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..709e032 100644
--- a/src/spice-channel-priv.h
+++ b/src/spice-channel-priv.h
@@ -171,6 +171,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..0195bda 100644
--- a/src/spice-channel.c
+++ b/src/spice-channel.c
@@ -56,6 +56,8 @@  static void spice_channel_reset_capabilities(SpiceChannel *channel);
 static void spice_channel_send_migration_handshake(SpiceChannel *channel);
 static gboolean channel_connect(SpiceChannel *channel, gboolean tls);
 
+#define QUEUE_MAX_LENGTH    500
+
 /**
  * SECTION:spice-channel
  * @short_description: the base channel class
@@ -2814,6 +2816,13 @@  enum spice_channel_state spice_channel_get_state(SpiceChannel *channel)
 }
 
 G_GNUC_INTERNAL
+gboolean spice_channel_has_full_queue (SpiceChannel *channel)
+{
+    SpiceChannelPrivate *c = channel->priv;
+    return (g_queue_get_length (&c->xmit_queue) > QUEUE_MAX_LENGTH);
+}
+
+G_GNUC_INTERNAL
 void spice_channel_swap(SpiceChannel *channel, SpiceChannel *swap, gboolean swap_msgs)
 {
     SpiceChannelPrivate *c = channel->priv;

Comments

(adding Hans in CC)

----- Original Message -----
> 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 length 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 150ms
>     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 | 1 +
>  src/spice-channel.c      | 9 +++++++++
>  3 files changed, 17 insertions(+)
> 
> 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..709e032 100644
> --- a/src/spice-channel-priv.h
> +++ b/src/spice-channel-priv.h
> @@ -171,6 +171,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..0195bda 100644
> --- a/src/spice-channel.c
> +++ b/src/spice-channel.c
> @@ -56,6 +56,8 @@ static void spice_channel_reset_capabilities(SpiceChannel
> *channel);
>  static void spice_channel_send_migration_handshake(SpiceChannel *channel);
>  static gboolean channel_connect(SpiceChannel *channel, gboolean tls);
>  
> +#define QUEUE_MAX_LENGTH    500
> +
>  /**
>   * SECTION:spice-channel
>   * @short_description: the base channel class
> @@ -2814,6 +2816,13 @@ enum spice_channel_state
> spice_channel_get_state(SpiceChannel *channel)
>  }
>  
>  G_GNUC_INTERNAL
> +gboolean spice_channel_has_full_queue (SpiceChannel *channel)
> +{
> +    SpiceChannelPrivate *c = channel->priv;
> +    return (g_queue_get_length (&c->xmit_queue) > QUEUE_MAX_LENGTH);
> +}
> +
> +G_GNUC_INTERNAL
>  void spice_channel_swap(SpiceChannel *channel, SpiceChannel *swap, gboolean
>  swap_msgs)
>  {
>      SpiceChannelPrivate *c = channel->priv;
> --
> 2.4.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>