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

Mon Jun 22 08:27:26 PDT 2015

----- Original Message -----
> Hello
> 
> ----- Original Message -----
> > From: "Marc-André Lureau" <mlureau at redhat.com>
> > To: "Sandy Stutsman" <sstutsma at redhat.com>
> > Cc: spice-devel at lists.freedesktop.org
> > Sent: Friday, June 19, 2015 6:58:36 PM
> > Subject: Re: [Spice-devel] [spice-gtk PATCH] This adds reference counting
> > to cached images.
> > 
> > Hi
> > 
> > ----- Original Message -----
> > > 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 looks like a change in the protocol.
> > - an image with an existing id will no longer replace it
> > - there is extra data being sent by the server
> > - this looks like a server bug to me if an image is being removed and later
> > referenced
> > 
> > Could this be addressed on server side to not change the current behaviour
> > and avoid unnecessary data being sent to the client?
> > 
> 
> Actually, when a new image arrives with an id that is currently in the cache,
> it will replace the older one. It will just be created with a +1 reference
> count.
> 
> In the case I've been debugging, the image in question is the Windows
> wallpaper.
> As there is a driver instance for each monitor, each driver is sending
> down it's own copy of the same image, with the same id. As one channel may be
> busier than another there is no way to guarantee the order that the client
> will handle commands from the server.  What I'm seeing is this:
>  On the server side
>     Monitor 1 sends image A
>     Monitor 1 sends mulitple render commands for  image A
>     Monitor 1 removes image A
>     Monitor 2 sends image A
>     Monitor 2 sends render command for image A ...
> 
>   What happens on the client end:
>     Monitor 1 adds Image A added to cache
>     Monitor 1 starts rendering from image A
>     Monitor 2 adds Image A to cache- has the same id, so just replaces it
>     Monitor 1 removes image A - same id, so the id is no longer in the cache
>     Monitor 2 hangs waiting to render image A - which is no longer in the
>     cache
> 
> In the above sequence, I don't think there is a way to prevent the image from
> being sent 2x from the server.  What helps keep this to a minimum is to
> lock the server cache for the whole of the call to fill_bits.  If only the
> pixmap_cache_xxx calls are locked it is possible, in the case that two
> channels are both executing in the fill_bits call, that the server will end
> up sending the same image twice.

Thanks for the explanation. Could this race also happen with other cache items?
If yes, then shouldn't the refcounted approach be generalized?

> I know this seems like a lot of hand waving but I think the basic problem
> is handling the Windows Multi-Monitor/multi-channel case where the channels
> share a number of common images.
> 
> > > Addresses bug: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
> > > ---
> > > 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;
> > > } 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 ) {
> > > + 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
> > > 
> > 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
> 