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

[Victor Toso]

Hi,

On Tue, Apr 17, 2018 at 09:45:10AM -0400, Frediano Ziglio wrote:
> > 
> > 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.

That was what I was thinking too...

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

Important is that last ref/free to happen from the main thread
which should happen but I need to double check the code..

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

Yes. Same thread, just context might change (being in one of the
coroutine contexts or not).

        toso

> 
> > > ---
> > >  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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/spice-devel/attachments/20180418/c56639c9/attachment.sig>