[Spice-devel] [spice-gtk PATCH v2] This adds reference counting to cached images.
Fabiano FidĂȘncio
fabiano at fidencio.org
Thu Jun 25 03:50:11 PDT 2015
On Wed, Jun 24, 2015 at 7:09 PM, Sandy Stutsman <sstutsma at redhat.com> wrote:
>
> Windows guests with multi-monitors will often send down the same image
> via different channels. If these instances are not counted, one channel
> can delete an image before all the rest of the channels are finished with
> it. In this case, the client will hang.
>
> This can happen, for instance, when the Windows guest is updating the
> monitor's wallpaper. There is a driver instance for each monitor, so each
> one is sending down its own copy of the same image with the same id. As
> one channel may be busier than another, the order that the client executes
> commands may not be the same order as the server issued them. Here's what
> can happen:
>
> On the server side:
> Monitor 1 sends image A
> Monitor 1 sends multiple rendering commands for image A
> Monitor 1 removes image A
> Monitor 2 sends image A
> Monitor 2 sends rendering commands for image A
> On the client side:
> Monitor 1 adds image A to the cache
> Monitor 1 starts rendering with image A
> Monitor 2 adds image A to the cache - has same id, so image is replaced
> Monitor 1 removes image A - image A is completely removed from cache
> Monitor 2 render command hangs waiting for image A
>
> Addresses bug: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
> ---
> Change since v1
> *Add to commit log
> ---
> src/spice-channel-cache.h | 60 ++++++++++++++++++++++++++++++++++++-----------
> src/spice-session.c | 2 +-
> 2 files changed, 47 insertions(+), 15 deletions(-)
>
> diff --git a/src/spice-channel-cache.h b/src/spice-channel-cache.h
> index 17775e6..968e661 100644
> --- a/src/spice-channel-cache.h
> +++ b/src/spice-channel-cache.h
> @@ -27,15 +27,20 @@ G_BEGIN_DECLS
> typedef struct display_cache_item {
> guint64 id;
> gboolean lossy;
> + uint32_t ref_count;
Could be guint32, right?
> } display_cache_item;
>
> -typedef GHashTable display_cache;
> +typedef struct display_cache {
> + GHashTable *table;
> + gboolean ref_counted;
> +}display_cache;
>
> static inline display_cache_item* cache_item_new(guint64 id, gboolean lossy)
> {
> display_cache_item *self = g_slice_new(display_cache_item);
> self->id = id;
> self->lossy = lossy;
> + self->ref_count = 1;
> return self;
> }
>
> @@ -46,18 +51,24 @@ static inline void cache_item_free(display_cache_item *self)
>
> static inline display_cache* cache_new(GDestroyNotify value_destroy)
> {
> - GHashTable* self;
> -
> - self = g_hash_table_new_full(g_int64_hash, g_int64_equal,
> - (GDestroyNotify)cache_item_free,
> - value_destroy);
> -
> + display_cache * self = g_slice_new(display_cache);
> + self->table = g_hash_table_new_full(g_int64_hash, g_int64_equal,
> + (GDestroyNotify) cache_item_free,
> + value_destroy);
> + self->ref_counted = FALSE;
> return self;
> }
>
> +static inline display_cache * cache_image_new(GDestroyNotify value_destroy)
> +{
> + display_cache * self = cache_new(value_destroy);
> + self->ref_counted = TRUE;
> + return self;
> +};
> +
> static inline gpointer cache_find(display_cache *cache, uint64_t id)
> {
> - return g_hash_table_lookup(cache, &id);
> + return g_hash_table_lookup(cache->table, &id);
> }
>
> static inline gpointer cache_find_lossy(display_cache *cache, uint64_t id, gboolean *lossy)
> @@ -65,7 +76,7 @@ static inline gpointer cache_find_lossy(display_cache *cache, uint64_t id, gbool
> gpointer value;
> display_cache_item *item;
>
> - if (!g_hash_table_lookup_extended(cache, &id, (gpointer*)&item, &value))
> + if (!g_hash_table_lookup_extended(cache->table, &id, (gpointer*)&item, &value))
> return NULL;
>
> *lossy = item->lossy;
> @@ -77,8 +88,17 @@ static inline void cache_add_lossy(display_cache *cache, uint64_t id,
> gpointer value, gboolean lossy)
> {
> display_cache_item *item = cache_item_new(id, lossy);
> -
> - g_hash_table_replace(cache, item, value);
> + display_cache_item *current_item;
> + gpointer current_image;
> +
> + //If image is currently in the table add its reference count before replacing it
> + if(cache->ref_counted) {
> + if(g_hash_table_lookup_extended(cache->table, &id, (gpointer*) ¤t_item,
> + (gpointer*) ¤t_image)) {
> + item->ref_count = current_item->ref_count + 1;
> + }
> + }
> + g_hash_table_replace(cache->table, item, value);
> }
>
> static inline void cache_add(display_cache *cache, uint64_t id, gpointer value)
> @@ -88,17 +108,29 @@ static inline void cache_add(display_cache *cache, uint64_t id, gpointer value)
>
> static inline gboolean cache_remove(display_cache *cache, uint64_t id)
> {
> - return g_hash_table_remove(cache, &id);
> + display_cache_item * item;
> + gpointer value;
> +
> + if( g_hash_table_lookup_extended(cache->table, &id, (gpointer*) &item, &value)) {
> + --item->ref_count;
> + if(!cache->ref_counted || !item->ref_count ) {
I really would prefer to have comparisons (when not booleans) in the
explicit way, like:
if(!cache->ref_counted || item->ref_count > 0) { ... }
> + return g_hash_table_remove(cache->table, &id);
> + }
> + }
> + else {
> + return FALSE;
> + }
> + return TRUE;
> }
>
> static inline void cache_clear(display_cache *cache)
> {
> - g_hash_table_remove_all(cache);
> + g_hash_table_remove_all(cache->table);
> }
>
> static inline void cache_unref(display_cache *cache)
> {
> - g_hash_table_unref(cache);
> + g_hash_table_unref(cache->table);
> }
>
> G_END_DECLS
> diff --git a/src/spice-session.c b/src/spice-session.c
> index 778d82a..a252448 100644
> --- a/src/spice-session.c
> +++ b/src/spice-session.c
> @@ -257,7 +257,7 @@ static void spice_session_init(SpiceSession *session)
> g_free(channels);
>
> ring_init(&s->channels);
> - s->images = cache_new((GDestroyNotify)pixman_image_unref);
> + s->images = cache_image_new((GDestroyNotify)pixman_image_unref);
> s->glz_window = glz_decoder_window_new();
> update_proxy(session, NULL);
> }
> --
> 1.9.5.msysgit.0
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Apart from the minors comments, that can or cannot be addressed (up to
Sandy), the patch looks good to me.
--
Fabiano FidĂȘncio
More information about the Spice-devel
mailing list