Sorry for the duplicate. I was getting a bogus "mail not sent" message. -S ----- Original Message ----- > From: "Sandy Stutsman" <sstutsma@xxxxxxxxxx> > To: spice-devel@xxxxxxxxxxxxxxxxxxxxx > Sent: Monday, June 22, 2015 5:44:28 PM > Subject: [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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/spice-devel > _______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel