Victor, On Mon, Oct 12, 2015 at 10:51 AM, Victor Toso <lists@xxxxxxxxxxxxxx> wrote: > 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@xxxxxxxxxxxxxxxxxxxxx >> http://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ > Spice-devel mailing list > Spice-devel@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel I had a private conversation with Hans (which he agreed to share in this email) and here is his suggestion: 15:51 <hansg> Hi, ah yes that one, I had it on my radar at one point... 15:51 <hansg> Overall I think the idea is good, but the dropping of usbredir data may only be done for isoc packets, not for any other packets, and data must be dropped a whole packet at a time 15:52 <hansg> So you likely will need to have some code in usbredirparser or usbredirhost to take care of this for you 15:53 <hansg> Also with video data (what this is about) you really do not want to drop every other packet, as then you will just end up generating only unusable frames. 15:54 <hansg> What you want to do is buffer upto a threshold and then stop accepting packets until the queue has been drained to a second much lower threshold 15:55 <hansg> Thinking more about this, I think that what you need to do is add a new callback which usbredirhost can call which is called "is_space_available" or some such, and have that return a boolean 15:55 <hansg> Then in usbredirhost when receiving an isoc packet from an usb device call this callback (if defined) and if it returns falls drop the packet 15:56 <hansg> Then in spice-gtk defined the callback and have it return true until the queue grows to above a certain threshold 15:56 <hansg> Once the queue is above the threshold have the callback return falls till the queue is drained till the 15:56 <hansg> second much lower threshold 15:57 <hansg> Does that make sense ? 15:58 <fidencio> yes, it does. 15:58 <fidencio> do you mind if I c&p this conversation to that thread? 15:58 <hansg> No I do not mind, please do copy and paste it, thanks 15:58 <fidencio> thanks a lot! Best Regards, -- Fabiano Fidêncio _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel