[Spice-devel] [spice-gtk PATCH v2] This adds reference counting to cached images.

Sandy Stutsman sstutsma at redhat.com
Thu Jun 25 07:17:09 PDT 2015


Hi.

----- Original Message -----
> From: "Fabiano FidĂȘncio" <fabiano at fidencio.org>
> To: "Sandy Stutsman" <sstutsma at redhat.com>
> Cc: spice-devel at lists.freedesktop.org
> Sent: Thursday, June 25, 2015 6:50:11 AM
> Subject: Re: [Spice-devel] [spice-gtk PATCH v2] This adds reference counting to cached images.
> 
> 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?
Yep.
> 
> >  } 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*)
> > &current_item,
> > +                                        (gpointer*) &current_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) { ... }
> 
actually changed to 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