[Spice-devel] [spice v10 10/27] server: Make the RedDrawable refcount thread-safe

[Christophe Fergeau]

On Tue, Mar 01, 2016 at 04:53:10PM +0100, Francois Gouget wrote:
> Signed-off-by: Francois Gouget <fgouget at codeweavers.com>
> ---
> 
> In theory this could be needed by the next patch.
> 
>  server/red-parse-qxl.h | 4 ++--
>  server/red-worker.c    | 7 ++++++-
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/server/red-parse-qxl.h b/server/red-parse-qxl.h
> index 9c30572..220a096 100644
> --- a/server/red-parse-qxl.h
> +++ b/server/red-parse-qxl.h
> @@ -24,7 +24,7 @@
>  #include "memslot.h"
>  
>  typedef struct RedDrawable {
> -    int refs;
> +    gint refs;
>      QXLInstance *qxl;
>      QXLReleaseInfoExt release_info_ext;
>      uint32_t surface_id;
> @@ -60,7 +60,7 @@ typedef struct RedDrawable {
>  
>  static inline RedDrawable *red_drawable_ref(RedDrawable *drawable)
>  {
> -    drawable->refs++;
> +    g_atomic_int_inc(&drawable->refs);
>      return drawable;
>  }
>  
> diff --git a/server/red-worker.c b/server/red-worker.c
> index fd8eefb..f45cc3b 100644
> --- a/server/red-worker.c
> +++ b/server/red-worker.c
> @@ -124,9 +124,14 @@ static void common_release_recv_buf(RedChannelClient *rcc, uint16_t type, uint32
>  
>  void red_drawable_unref(RedDrawable *red_drawable)
>  {
> -    if (--red_drawable->refs) {
> +    gint old_refs;
> +    do {
> +        old_refs = red_drawable->refs;
> +    } while (!g_atomic_int_compare_and_exchange(&red_drawable->refs, old_refs, old_refs - 1));
> +    if (old_refs > 1) {
>          return;
>      }
> +
>      red_drawable->qxl->st->qif->release_resource(red_drawable->qxl,
>                                                   red_drawable->release_info_ext);
>      red_put_drawable(red_drawable);

Making the refcounting thread safe is one thing, but then the code
freeing the drawable needs to be thread-safe too. red_put_drawable might
be fine (only looked briefly), but release_resource which is a vfunc
provided by QEMU seems much less obvious. I think the drawable refcount
is changed in the display channel thread, so I would try to queue an
idle on its main_context, and do the unref from there. Hopefully it will
not be too messy... (and I'll mention again
g_main_context_push_thread_default which could be useful there ;) or
some thread-local storage variable).

By "queueing an idle", I mean something like
GSource *source = g_idle_source_new();
g_source_set_callback(..);
g_source_attach(source, worker->core.main_context);

Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20160303/57ba09f5/attachment.sig>