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

[Frediano Ziglio]

> 
> 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
> 

No, Qemu must provide a thread safe release_resource.
Two reasons:
- release_resource is called from RedWorker which is a different
  thread than Qemu or other users (it's just created inside spice-server);
- I looked at Qemu code and there is a clear comment on this ;-) .

Frediano