Re: [spice PATCH] Lock the pixmap image cache for the entiry fill_bits call

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]