[Spice-devel] [spice-gtk PATCH v6 2/2] channel-usbredir: drop isoc packets on low bandwidth
Hans de Goede
hdegoede at redhat.com
Fri Oct 23 06:37:47 PDT 2015
Hi,
On 10/23/2015 10:59 AM, Fabiano Fidêncio wrote:
> On Fri, Oct 23, 2015 at 9:53 AM, Victor Toso <victortoso at redhat.com> 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 at lists.freedesktop.org
>> 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.
Ack, I believe the usbredir patch is ready, feel free to push that
with my suggested doc change, then do a follow up patch to bump
the version in usbredirproto.h and configure.ac to 0.7.1 and
update the Changelog file with changes which have landed since
0.7 while you are at it.
Then you can make the usbredirhost_set_buffered_output_size_cb
call in spice-gtk:
#if USBREDIR_VERSION >= 0x000701
usbredirhost_set_buffered_output_size_cb(priv->host, usbredir_buffered_output_size_callback);
#endif
And then once David Alan's "Allow missing capabilities from source host"
patch is also merged we should probably do an official 0.7.1 release.
Regards,
Hans
>
> 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,
>
More information about the Spice-devel
mailing list