[Spice-devel] [spice-gtk PATCH v2] spice-channel: check message queue length
Fabiano Fidêncio
fidencio at redhat.com
Fri Oct 16 07:04:12 PDT 2015
On Fri, Oct 16, 2015 at 4:02 PM, Fabiano Fidêncio <fidencio at redhat.com> wrote:
> 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
And ...
16:01 <hansg> About adding the callback to the usbredirhost code. If
you look at usbredirhost.h you will see that struct usbredirhost
is not defined there, all uses of it deals with pointers
to it only, so you can simply add an extra callback pointer to
the struct without breaking ABI
16:02 <hansg> And then add a new helper function to set the new
callback on a struct usbredirhost * returned by
usbredirhost_open[_full]()
Best Regards,
--
Fabiano Fidêncio
More information about the Spice-devel
mailing list