On Tue, Jan 17, 2012 at 01:35:21PM +0100, Hans de Goede wrote:
> spice_msg_out can be not only called from system context and usb event
> handling thread context, but also from co-routine context. Calling from
> co-routine context happens when a response gets send synchronously from
> the handle_msg handler for a certain received packet. This happens with
> certain usbredir commands.
>
> This triggers the following assert in the coroutine code:
> "GSpice-CRITICAL **: g_coroutine_wakeup: assertion `coroutine !=
> g_coroutine_self()' failed"
>
> This patch fixes this by making spice_msg_out_send callable from any
> context and add the same time changing the code to not do unnecessary
> wakeups.
>
> Signed-off-by: Hans de Goede <hdego...@redhat.com>
> ---
> gtk/spice-channel-priv.h | 2 +-
> gtk/spice-channel.c | 52 ++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/gtk/spice-channel-priv.h b/gtk/spice-channel-priv.h
> index ebdc5ce..5cd7ddb 100644
> --- a/gtk/spice-channel-priv.h
> +++ b/gtk/spice-channel-priv.h
> @@ -102,7 +102,7 @@ struct _SpiceChannelPrivate {
> GQueue xmit_queue;
> gboolean xmit_queue_blocked;
> GStaticMutex xmit_queue_lock;
> - GThread *main_thread;
> + guint xmit_queue_wakeup_id;
>
> char name[16];
> enum spice_channel_state state;
> diff --git a/gtk/spice-channel.c b/gtk/spice-channel.c
> index 83cd344..bdfb02b 100644
> --- a/gtk/spice-channel.c
> +++ b/gtk/spice-channel.c
> @@ -110,7 +110,6 @@ static void spice_channel_init(SpiceChannel *channel)
> spice_channel_set_common_capability(channel,
> SPICE_COMMON_CAP_MINI_HEADER);
> g_queue_init(&c->xmit_queue);
> g_static_mutex_init(&c->xmit_queue_lock);
> - c->main_thread = g_thread_self();
> }
>
> static void spice_channel_constructed(GObject *gobject)
> @@ -649,14 +648,32 @@ void spice_msg_out_unref(SpiceMsgOut *out)
> static gboolean spice_channel_idle_wakeup(gpointer user_data)
> {
> SpiceChannel *channel = SPICE_CHANNEL(user_data);
> + SpiceChannelPrivate *c = channel->priv;
> +
> + /*
> + * Note:
> + *
> + * - This must be done before the wakeup as that may eventually
> + * call channel_reset() which checks this.
> + * - The lock calls are really necessary, this fixes the following race:
> + * 1) usb-event-thread calls spice_msg_out_send()
> + * 2) spice_msg_out_send calls g_timeout_add_full(...)
> + * 3) we run, set xmit_queue_wakeup_id to 0
> + * 4) spice_msg_out_send stores the result of g_timeout_add_full() in
> + * xmit_queue_wakeup_id, overwriting the 0 we just stored
> + * 5) xmit_queue_wakeup_id now says there is a wakeup pending which is
> + * false
> + */
> + g_static_mutex_lock(&c->xmit_queue_lock);
> + c->xmit_queue_wakeup_id = 0;
> + g_static_mutex_unlock(&c->xmit_queue_lock);
>
> spice_channel_wakeup(channel, FALSE);
> - g_object_unref(channel);
>
> return FALSE;
> }
>
> -/* system context */
> +/* any context (system/co-routine/usb-event-thread) */
> G_GNUC_INTERNAL
> void spice_msg_out_send(SpiceMsgOut *out)
> {
> @@ -664,17 +681,23 @@ void spice_msg_out_send(SpiceMsgOut *out)
> g_return_if_fail(out->channel != NULL);
>
> g_static_mutex_lock(&out->channel->priv->xmit_queue_lock);
> - if (!out->channel->priv->xmit_queue_blocked)
Just one thing I don't understand, not related to the current patch (was
this way before) - we drop everything on the floor if
xmit_queue_blocked, which is only set between channel_reset and
channel_connect. Why don't we log this error? (no, I'm not saying we
should assert on this..)
> + if (!out->channel->priv->xmit_queue_blocked) {
> + gboolean was_empty;
> +
> + was_empty = g_queue_is_empty(&out->channel->priv->xmit_queue);
> g_queue_push_tail(&out->channel->priv->xmit_queue, out);
> - g_static_mutex_unlock(&out->channel->priv->xmit_queue_lock);
>
> - /* TODO: we currently flush/wakeup immediately all buffered messages */
> - if (g_thread_self() != out->channel->priv->main_thread)
> - /* We use g_timeout_add_full so that can specify the priority */
> - g_timeout_add_full(G_PRIORITY_HIGH, 0, spice_channel_idle_wakeup,
> - g_object_ref(out->channel), NULL);
> - else
> - spice_channel_wakeup(out->channel, FALSE);
> + /* 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. */
> + if (was_empty && !out->channel->priv->xmit_queue_wakeup_id) {
> + out->channel->priv->xmit_queue_wakeup_id =
> + /* Use g_timeout_add_full so that can specify the priority */
> + g_timeout_add_full(G_PRIORITY_HIGH, 0,
> + spice_channel_idle_wakeup,
> + out->channel, NULL);
> + }
> + }
> + g_static_mutex_unlock(&out->channel->priv->xmit_queue_lock);
> }
>
> /* coroutine context */
> @@ -1688,7 +1711,6 @@ error:
> }
>
> /* system context */
> -/* TODO: we currently flush/wakeup immediately all buffered messages */
> G_GNUC_INTERNAL
> void spice_channel_wakeup(SpiceChannel *channel, gboolean cancel)
> {
> @@ -2344,6 +2366,10 @@ static void channel_reset(SpiceChannel *channel,
> gboolean migrating)
> c->xmit_queue_blocked = TRUE; /* Disallow queuing new messages */
> g_queue_foreach(&c->xmit_queue, (GFunc)spice_msg_out_unref, NULL);
> g_queue_clear(&c->xmit_queue);
> + if (c->xmit_queue_wakeup_id) {
> + g_source_remove(c->xmit_queue_wakeup_id);
> + c->xmit_queue_wakeup_id = 0;
> + }
> g_static_mutex_unlock(&c->xmit_queue_lock);
>
> g_array_set_size(c->remote_common_caps, 0);
> --
> 1.7.7.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/spice-devel
