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

Submitted by Frediano Ziglio on April 17, 2018, 12:32 p.m.

Details

Message ID 20180417123253.8023-1-fziglio@redhat.com
State New
Headers show
Series "spice-channel: Use atomic operations for SpiceMsgIn::refcount" ( rev: 1 ) in Spice

Not browsing as part of any series.

Commit Message

Frediano Ziglio April 17, 2018, 12:32 p.m.
This structure is potentially used in multiple thread.
Currently in Gstreamer thread using streaming data and coroutine
thread.

Signed-off-by: Frediano Ziglio <fziglio@redhat.com>
---
I would avoid the atomic penalty but better safe then sorry
---
 src/spice-channel-priv.h | 2 +-
 src/spice-channel.c      | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

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

Comments

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@redhat.com>
> ---
> I would avoid the atomic penalty but better safe then sorry
> ---
>  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);
> -- 
> 2.14.3
> 
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
> 
> 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@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
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@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