[Spice-devel] [PATCH spice-gtk] spice-channel: Use atomic operations for SpiceMsgIn::refcount

[Frediano Ziglio]

> 
> On Tue, Apr 17, 2018 at 01:32:53PM +0100, Frediano Ziglio wrote:
> > This structure is potentially used in multiple thread.
> > Currently in Gstreamer thread using streaming data and coroutine
> > thread.
> 
> But only GStreamer uses another thread, so this might be
> suboptimal for all other channels.
> 
> > 
> > Signed-off-by: Frediano Ziglio <fziglio at redhat.com>
> > ---
> > I would avoid the atomic penalty but better safe then sorry

Yes, that is my comment too... do you have another safe solution?

I was thinking about adding an atomic reference counting for
SpiceFrame instead and using it for instance, but I'm not sure
it fixes all the cases. It would need to remove ref_data/unref_data
and have ownership from SpiceFrame to "data" (raw message basically)
but this would mean that refcount would be 2 in queue_frame and
decremented by coroutine thread and lately by GStreamer thread which
in theory is racy too.

OT: not much familiar with the threading model of spice-gtk.
I supposed (beside GStreamer) there were a main thread and a
coroutine thread. Is the main thread the same as coroutine one?

> > ---
> >  src/spice-channel-priv.h | 2 +-
> >  src/spice-channel.c      | 5 ++---
> >  2 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/src/spice-channel-priv.h b/src/spice-channel-priv.h
> > index b431037..706df9f 100644
> > --- a/src/spice-channel-priv.h
> > +++ b/src/spice-channel-priv.h
> > @@ -55,7 +55,7 @@ struct _SpiceMsgOut {
> >  };
> >  
> >  struct _SpiceMsgIn {
> > -    int                   refcount;
> > +    gint                  refcount;
> >      SpiceChannel          *channel;
> >      uint8_t               header[MAX_SPICE_DATA_HEADER_SIZE];
> >      uint8_t               *data;
> > diff --git a/src/spice-channel.c b/src/spice-channel.c
> > index 7e3e3b7..49897b7 100644
> > --- a/src/spice-channel.c
> > +++ b/src/spice-channel.c
> > @@ -531,7 +531,7 @@ void spice_msg_in_ref(SpiceMsgIn *in)
> >  {
> >      g_return_if_fail(in != NULL);
> >  
> > -    in->refcount++;
> > +    g_atomic_int_inc(&in->refcount);
> >  }
> >  
> >  G_GNUC_INTERNAL
> > @@ -539,8 +539,7 @@ void spice_msg_in_unref(SpiceMsgIn *in)
> >  {
> >      g_return_if_fail(in != NULL);
> >  
> > -    in->refcount--;
> > -    if (in->refcount > 0)
> > +    if (!g_atomic_int_dec_and_test(&in->refcount))
> >          return;
> >      if (in->parsed)
> >          in->pfree(in->parsed);

Frediano