[Spice-devel] [spice-gtk PATCH v2] spice-channel: check message queue length
Fabiano Fidêncio
fidencio at redhat.com
Fri Oct 16 07:02:11 PDT 2015
Victor,
On Mon, Oct 12, 2015 at 10:51 AM, Victor Toso <lists at victortoso.com> 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> 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
More information about the Spice-devel
mailing list