Hey,
On Tue, Jun 23, 2015 at 05:47:14PM -0400, Sandy Stutsman wrote:
> 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.
>
A more detailed description of the race in the commit log would be very
useful, both in terms of reviewing, and for future reference when
looking at git history.
The patch in general looks correct to me, but I suspect there are still
some races around pixmap_cache_locked_hit() in
is_bitmap_lossy()/is_brush_lossy() as code is doing:
is_lossy = is_bitmap_lossy(); (which calls pixmap_cache_locked_hit())
[some code]
if (is_lossy) {
do_something();
} else {
do_something_else();
}
It seems to be that fill_bits could have been called from another thread
and changed the lossiness in between the is_bitmap_lossy() call and the
if (is_lossy) test.
> Addresses: https://bugzilla.redhat.com/show_bug.cgi?id=1194354
> ---
> 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)
If we do this change, after applying this patch, we've got eg
pixmap_cache_locked_hit() and pixmap_cache_freeze() both locking the
mutex appropriately but with inconsistant names. I'd keep the
pixmap_cache_hit() name for the locked case, and add an _unlocked_
variant (see attached patch). I also have a patch renaming
pixmap_cache_set_lossy() for consistency (it's not taking a lock).
Christophe
Patch on top of yours:
From 072e9357e858dc51e82f884f8d8cbf45345a9b95 Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
Date: Thu, 25 Jun 2015 14:10:33 +0200
Subject: [PATCH] fixup! Lock the pixmap cache for the fill_bits function call
---
server/red_client_shared_cache.h | 8 ++++----
server/red_worker.c | 18 +++++++++---------
2 files changed, 13 insertions(+), 13 deletions(-)
diff --git a/server/red_client_shared_cache.h b/server/red_client_shared_cache.h
index 3b56e92..3afed18 100644
--- a/server/red_client_shared_cache.h
+++ b/server/red_client_shared_cache.h
@@ -36,7 +36,7 @@
#define CHANNEL_FROM_RCC(rcc) SPICE_CONTAINEROF((rcc)->channel, CHANNEL, common.base);
-static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
+static int FUNC_NAME(unlocked_hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
{
NewCacheItem *item;
uint64_t serial;
@@ -60,11 +60,11 @@ static int FUNC_NAME(hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelC
return !!item;
}
-static int FUNC_NAME(locked_hit)(CACHE *cache, uint64_t id, int *lossy, DisplayChannelClient *dcc)
+static int FUNC_NAME(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);
+ hit = FUNC_NAME(unlocked_hit)(cache,id,lossy, dcc);
pthread_mutex_unlock(&cache->lock);
return hit;
}
@@ -85,7 +85,7 @@ static int FUNC_NAME(set_lossy)(CACHE *cache, uint64_t id, int lossy)
return !!item;
}
-static int FUNC_NAME(add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc)
+static int FUNC_NAME(unlocked_add)(CACHE *cache, uint64_t id, uint32_t size, int lossy, DisplayChannelClient *dcc)
{
NewCacheItem *item;
uint64_t serial;
diff --git a/server/red_worker.c b/server/red_worker.c
index dd91179..301b4af 100644
--- a/server/red_worker.c
+++ b/server/red_worker.c
@@ -6686,9 +6686,9 @@ static inline void red_display_add_image_to_pixmap_cache(RedChannelClient *rcc,
if ((image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {
spice_assert(image->descriptor.width * image->descriptor.height > 0);
if (!(io_image->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_REPLACE_ME)) {
- if (pixmap_cache_add(dcc->pixmap_cache, image->descriptor.id,
- image->descriptor.width * image->descriptor.height, is_lossy,
- dcc)) {
+ if (pixmap_cache_unlocked_add(dcc->pixmap_cache, image->descriptor.id,
+ image->descriptor.width * image->descriptor.height, is_lossy,
+ dcc)) {
io_image->descriptor.flags |= SPICE_IMAGE_FLAGS_CACHE_ME;
dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] =
image->descriptor.id;
@@ -6738,8 +6738,8 @@ static FillBitsType fill_bits(DisplayChannelClient *dcc, SpiceMarshaller *m,
if ((simage->descriptor.flags & SPICE_IMAGE_FLAGS_CACHE_ME)) {
int lossy_cache_item;
- if (pixmap_cache_hit(dcc->pixmap_cache, image.descriptor.id,
- &lossy_cache_item, dcc)) {
+ if (pixmap_cache_unlocked_hit(dcc->pixmap_cache, image.descriptor.id,
+ &lossy_cache_item, dcc)) {
dcc->send_data.pixmap_cache_items[dcc->send_data.num_pixmap_cache_items++] =
image.descriptor.id;
if (can_lossy || !lossy_cache_item) {
@@ -6990,8 +6990,8 @@ static int is_bitmap_lossy(RedChannelClient *rcc, SpiceImage *image, SpiceRect *
int is_hit_lossy;
out_data->id = image->descriptor.id;
- if (pixmap_cache_locked_hit(dcc->pixmap_cache, image->descriptor.id,
- &is_hit_lossy, dcc)) {
+ if (pixmap_cache_hit(dcc->pixmap_cache, image->descriptor.id,
+ &is_hit_lossy, dcc)) {
out_data->type = BITMAP_DATA_TYPE_CACHE;
if (is_hit_lossy) {
return TRUE;
@@ -8412,8 +8412,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_locked_hit(dcc->pixmap_cache, dcc->send_data.pixmap_cache_items[i],
- &dummy, dcc);
+ pixmap_cache_hit(dcc->pixmap_cache, dcc->send_data.pixmap_cache_items[i],
+ &dummy, dcc);
}
if (free_list->wait.header.wait_count) {
--
2.4.3
Attachment:
pgppBbLN818Ym.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel