[Spice-devel] [PATCH] Lock the pixmap cache for the fill_bits function call
Christophe Fergeau
cfergeau at redhat.com
Mon Jun 22 04:05:46 PDT 2015
From: Sandy Stutsman <sstutsma at redhat.com>
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 sent
multiple times.
Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
---
Hey, for some reason this patch arrived corrupted in my inbox (mail client issue on your side?).
Here is a version which should apply cleanly with git am (but I'm not 100% sure
it's identical to your initial patch /o\ )
Christophe
server/red_client_shared_cache.h | 18 +++++++++---------
server/red_worker.c | 17 +++++++++++++----
2 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/server/red_client_shared_cache.h b/server/red_client_shared_cache.h
index 821ee18..3b56e92 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);
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 434343a..5fd4d16 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -6749,6 +6749,8 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
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;
if (pixmap_cache_hit(dcc->pixmap_cache, image.descriptor.id,
@@ -6769,6 +6771,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 +6789,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 +6804,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 +6836,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 +6854,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 +6868,13 @@ 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,8 +7005,8 @@ 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,
- &is_hit_lossy, dcc)) {
+ 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) {
return TRUE;
@@ -8418,8 +8427,8 @@ 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],
- &dummy, dcc);
+ pixmap_cache_locked_hit(dcc->pixmap_cache, dcc->send_data.pixmap_cache_items[i],
+ &dummy, dcc);
}
if (free_list->wait.header.wait_count) {
--
2.4.3
More information about the Spice-devel
mailing list