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