Hi Christophe,
Just sent out a patch with your revisions and a more detailed commit message.
You may be right about race conditions around the lossy state.
I didn't notice any when testing when testing, but as I don't think it would result
in the client hanging, it doesn't mean it isn't happening. I'm not sure what
would happen other than some (transient ?) rendering artifacts.
-S
On 6/26/2015 9:14 AM, Christophe Fergeau wrote:
> 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) {
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel