[Spice-devel] [spice PATCH] Lock the pixmap cache for the fill_bits function call.

[Sandy Stutsman]

Sorry for the duplicate.  I was getting a bogus "mail not sent" message.

-S

----- Original Message -----
> From: "Sandy Stutsman" <sstutsma at redhat.com>
> To: spice-devel at lists.freedesktop.org
> Sent: Monday, June 22, 2015 5:44:28 PM
> Subject: [Spice-devel] [spice PATCH] Lock the pixmap cache for the fill_bits function call.
> 
> Locking the individual calls that access the pixmap cache in fill_bits is
> not adequately thread safe. Often a windows guest with multiple monitors
> will be sending the same image via different threads. Both threads can
> be in fill_bits at the same making changes to the cache for the same
> image. This can result in images being deleted before all the client
> channels are finished with them or with the same image being send multiple
> times.
> 
> Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
> ---
> To replace the possibly corrupt patch from yesterday
> ---
> server/red_client_shared_cache.h | 20 ++++++++++----------
> server/red_worker.c | 13 ++++++++++---
> 2 files changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/server/red_client_shared_cache.h
> b/server/red_client_shared_cache.h
> index 821ee18..4932f2d 100644
> --- a/server/red_client_shared_cache.h
> +++ b/server/red_client_shared_cache.h
> @@ -42,7 +42,6 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id,
> int *lossy, DisplayChannelC
> uint64_t serial;
> 
> serial = red_channel_client_get_message_serial(&dcc->common.base);
> - pthread_mutex_lock(&cache->lock);
> item = cache->hash_table[CACHE_HASH_KEY(id)];
> 
> while (item) {
> @@ -57,15 +56,22 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id,
> int *lossy, DisplayChannelC
> }
> item = item->next;
> }
> - pthread_mutex_unlock(&cache->lock);
> 
> return !!item;
> }
> 
> +static int FUNC_NAME(locked_hit)(CACHE *cache, uint64_t id, int *lossy,
> DisplayChannelClient *dcc)
> +{
> + int hit;
> + pthread_mutex_lock(&cache->lock);
> + hit = FUNC_NAME(hit)(cache,id,lossy, dcc);
> + pthread_mutex_unlock(&cache->lock);
> + return hit;
> +}
> +
> static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)
> {
> NewCacheItem *item;
> - pthread_mutex_lock(&cache->lock);
> 
> item = cache->hash_table[CACHE_HASH_KEY(id)];
> 
> @@ -76,7 +82,6 @@ static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t
> id, int lossy)
> }
> item = item->next;
> }
> - pthread_mutex_unlock(&cache->lock);
> return !!item;
> }
> 
> @@ -91,15 +96,12 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id,
> uint32_t size, int lossy, D
> item = spice_new(NewCacheItem, 1);
> serial = red_channel_client_get_message_serial(&dcc->common.base);
> 
> - pthread_mutex_lock(&cache->lock);
> -
> if (cache->generation != dcc->CACH_GENERATION) {
> if (!dcc->pending_pixmaps_sync) {
> red_channel_client_pipe_add_type(
> &dcc->common.base, PIPE_ITEM_TYPE_PIXMAP_SYNC);
> dcc->pending_pixmaps_sync = TRUE;
> - }
> - pthread_mutex_unlock(&cache->lock);
> + }ixmap_cache
> free(item);
> return FALSE;
> }
> @@ -112,7 +114,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id,
> uint32_t size, int lossy, D
> if (!(tail = (NewCacheItem *)ring_get_tail(&cache->lru)) ||
> tail->sync[dcc->common.id] == serial) {
> cache->available += size;
> - pthread_mutex_unlock(&cache->lock);
> free(item);
> return FALSE;
> }
> @@ -144,7 +145,6 @@ static int FUNC_NAME(add)(CACHE *cache, uint64_t id,
> uint32_t size, int lossy, D
> memset(item->sync, 0, sizeof(item->sync));
> item->sync[dcc->common.id] = serial;
> cache->sync[dcc->common.id] = serial;
> - pthread_mutex_unlock(&cache->lock);
> return TRUE;
> }
> 
> diff --git a/server/red_worker.c b/server/red_worker.c
> index 58a7d00..2e4a94d 100644
> --- a/server/red_worker.c
> +++ b/server/red_worker.c
> @@ -6748,6 +6748,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> *dcc, SpiceMarshaller *m,
> if (simage->descriptor.flags & SPICE_IMAGE_FLAGS_HIGH_BITS_SET) {
> image.descriptor.flags = SPICE_IMAGE_FLAGS_HIGH_BITS_SET;
> }
> + pthread_mutex_lock(&dcc->pixmap_cache->lock);
> 
> if ((simage->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {
> int lossy_cache_item;
> @@ -6769,6 +6770,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> *dcc, SpiceMarshaller *m,
> spice_assert(bitmap_palette_out == NULL);
> spice_assert(lzplt_palette_out == NULL);
> stat_inc_counter(display_channel->cache_hits_counter, 1);
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return FILL_BITS_TYPE_CACHE;
> } else {
> pixmap_cache_set_lossy(dcc->pixmap_cache, simage->descriptor.id,
> @@ -6786,6 +6788,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> *dcc, SpiceMarshaller *m,
> surface_id = simage->u.surface.surface_id;
> if (!validate_surface(worker, surface_id)) {
> rendering_incorrect("SPICE_IMAGE_TYPE_SURFACE");
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return FILL_BITS_TYPE_SURFACE;
> }
> 
> @@ -6800,6 +6803,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> *dcc, SpiceMarshaller *m,
> &bitmap_palette_out, &lzplt_palette_out);
> spice_assert(bitmap_palette_out == NULL);
> spice_assert(lzplt_palette_out == NULL);
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return FILL_BITS_TYPE_SURFACE;
> }
> case SPICE_IMAGE_TYPE_BITMAP: {
> @@ -6831,6 +6835,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> *dcc, SpiceMarshaller *m,
> }
> 
> spice_marshaller_add_ref_chunks(m, bitmap->data);
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return FILL_BITS_TYPE_BITMAP;
> } else {
> red_display_add_image_to_pixmap_cache(rcc, simage, &image,
> @@ -6848,6 +6853,7 @@ static FillBitsType fill_bits(DisplayChannelClient
> *dcc, SpiceMarshaller *m,
> }
> 
> spice_assert(!comp_send_data.is_lossy || can_lossy);
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return (comp_send_data.is_lossy ? FILL_BITS_TYPE_COMPRESS_LOSSY :
> FILL_BITS_TYPE_COMPRESS_LOSSLESS);
> }
> @@ -6861,11 +6867,12 @@ static FillBitsType
> fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
> spice_assert(bitmap_palette_out == NULL);
> spice_assert(lzplt_palette_out == NULL);
> spice_marshaller_add_ref_chunks(m, image.u.quic.data);
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return FILL_BITS_TYPE_COMPRESS_LOSSLESS;
> default:
> spice_error("invalid image type %u", image.descriptor.type);
> }
> -
> + pthread_mutex_unlock(&dcc->pixmap_cache->lock);
> return 0;
> }
> 
> @@ -6996,7 +7003,7 @@ static int is_bitmap_lossy(RedChannelClient *rcc,
> SpiceImage *image, SpiceRect *
> int is_hit_lossy;
> 
> out_data->id = image->descriptor.id;
> - if (pixmap_cache_hit(dcc->pixmap_cache, image->descriptor.id,
> + if (pixmap_cache_locked_hit(dcc->pixmap_cache, image->descriptor.id,
> &is_hit_lossy, dcc)) {
> out_data->type = BITMAP_DATA_TYPE_CACHE;
> if (is_hit_lossy) {
> @@ -8418,7 +8425,7 @@ static inline void
> display_channel_send_free_list(RedChannelClient *rcc)
> * But all this message pixmaps cache references used its old serial.
> * we use pixmap_cache_items to collect these pixmaps, and we update
> their serial
> * by calling pixmap_cache_hit. */
> - pixmap_cache_hit(dcc->pixmap_cache, dcc->send_data.pixmap_cache_items[i],
> + pixmap_cache_locked_hit(dcc->pixmap_cache,
> dcc->send_data.pixmap_cache_items[i],
> &dummy, dcc);
> }
> 
> --
> 1.9.5.msysgit.0
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>