[vlc-devel] [PATCH] core: Add atomic refcounter helper
Romain Vimont
rom1v at videolabs.io
Sat Jun 30 22:49:23 CEST 2018
On Sat, Jun 30, 2018 at 10:28:14PM +0200, Rémi Denis-Courmont wrote:
> The only times I recall that somebody screwed up reference counting where nontrivial cases not handled by this patch.
In network/httpd.c, httpd_HostDelete():
if (atomic_fetch_sub_explicit(&host->ref, 1, memory_order_relaxed) > 1)
I'm not sure a memory_order_relaxed is sufficient here, for the reasons
explained in the video (the data need to be seen by the thread that
destroys the object).
> And the Boolean result is not obvious at all. As far as error proneness comes, this is actually *worse*.
That's an opinion, I don't share it.
> Le 30 juin 2018 22:19:12 GMT+02:00, Romain Vimont <rom1v at videolabs.io> a écrit :
> >On Sat, Jun 30, 2018 at 09:12:27PM +0200, Rémi Denis-Courmont wrote:
> >> Hei,
> >>
> >> It is not that simple. Leaving aside that asserts in public headers
> >are troublesome, there is a lot of variance in reference counters:
> >> - type sizes (uint, uintptr...),
> >
> >(which refcount would need uintptr_t?)
> >
> >> - order on decrement - acq_rel is not always needed,
> >
> >That's not a problem to guarantee a memory order stronger than
> >required.
> >
> >> - corner cases where the decrement is known to not reach zero,
> >
> >Like?
> >
> >> - corner cases where the decrement cannot be permitted to reach
> >zero(e.g. vlc_object_t, IIRC).
> >>
> >> We used to have a wrapper and it was removed because it turned more
> >into an inconvenience.
> >
> >Even if it does not fit all (corner) cases, I think it is worth having
> >a
> >simple refcounter for the most general case (a refcounted structure).
> >
> >In the typical case:
> >
> > if (vlc_atomic_rc_dec(rc))
> > destroy_object();
> >
> >is simpler and less error-prone than:
> >
> > if (atomic_fetch_sub_explicit(refs, 1, memory_order_acq_rel) == 1)
> > destroy_object();
> >
> >> Le 29 juin 2018 21:03:49 GMT+02:00, Romain Vimont
> ><rom1v at videolabs.io> a écrit :
> >> >Implement an atomic refcounter with a weak but correct (1) memory
> >> >order,
> >> >and expose a simple API.
> >> >
> >> >Use it in some existing code to demo its usage.
> >> >
> >> >(1) See for example "Using weakly ordered C++ atomics correctly" by
> >> >Hans
> >> > Boehm at CppCon 2016 (the refcounting part also applies to C11):
> >> > <https://www.youtube.com/watch?v=M15UKpNlpeM&t=45m15s>
> >> >---
> >> > include/vlc_atomic.h | 25 +++++++++++++++++++++++++
> >> > src/input/item.c | 6 +++---
> >> > src/input/item.h | 4 ++--
> >> > src/input/resource.c | 10 +++++-----
> >> > src/misc/picture.c | 9 +++------
> >> > src/misc/picture.h | 5 ++---
> >> > 6 files changed, 40 insertions(+), 19 deletions(-)
> >> >
> >> >diff --git a/include/vlc_atomic.h b/include/vlc_atomic.h
> >> >index 868918fe8e6..2cb5e23f489 100644
> >> >--- a/include/vlc_atomic.h
> >> >+++ b/include/vlc_atomic.h
> >> >@@ -30,6 +30,7 @@
> >> >* Atomic operations do not require locking, but they are not very
> >> >powerful.
> >> > */
> >> >
> >> >+# include <assert.h>
> >> > # include <stdatomic.h>
> >> >
> >> > typedef atomic_uint_least32_t vlc_atomic_float;
> >> >@@ -57,4 +58,28 @@ static inline void
> >> >vlc_atomic_store_float(vlc_atomic_float *atom, float f)
> >> > atomic_store(atom, u.i);
> >> > }
> >> >
> >> >+typedef atomic_uint vlc_atomic_rc_t;
> >> >+
> >> >+/** Init the RC to 1 */
> >> >+static inline void vlc_atomic_rc_init(vlc_atomic_rc_t *var)
> >> >+{
> >> >+ atomic_init(var, 1);
> >> >+}
> >> >+
> >> >+/** Increment the RC */
> >> >+static inline void vlc_atomic_rc_inc(vlc_atomic_rc_t *var)
> >> >+{
> >> >+ unsigned prev = atomic_fetch_add_explicit(var, 1,
> >> >memory_order_relaxed);
> >> >+ assert(prev);
> >> >+ VLC_UNUSED(prev);
> >> >+}
> >> >+
> >> >+/** Decrement the RC and return true if it reaches 0 */
> >> >+static inline bool vlc_atomic_rc_dec(vlc_atomic_rc_t *var)
> >> >+{
> >> >+ unsigned prev = atomic_fetch_sub_explicit(var, 1,
> >> >memory_order_acq_rel);
> >> >+ assert(prev);
> >> >+ return prev == 1;
> >> >+}
> >> >+
> >> > #endif
> >> >diff --git a/src/input/item.c b/src/input/item.c
> >> >index 6789f73caa5..8e846425b4b 100644
> >> >--- a/src/input/item.c
> >> >+++ b/src/input/item.c
> >> >@@ -471,7 +471,7 @@ input_item_t *input_item_Hold( input_item_t
> >*p_item
> >> >)
> >> > {
> >> > input_item_owner_t *owner = item_owner(p_item);
> >> >
> >> >- atomic_fetch_add( &owner->refs, 1 );
> >> >+ vlc_atomic_rc_inc( &owner->rc );
> >> > return p_item;
> >> > }
> >> >
> >> >@@ -479,7 +479,7 @@ void input_item_Release( input_item_t *p_item )
> >> > {
> >> > input_item_owner_t *owner = item_owner(p_item);
> >> >
> >> >- if( atomic_fetch_sub(&owner->refs, 1) != 1 )
> >> >+ if( !vlc_atomic_rc_dec( &owner->rc ) )
> >> > return;
> >> >
> >> > vlc_event_manager_fini( &p_item->event_manager );
> >> >@@ -1060,7 +1060,7 @@ input_item_NewExt( const char *psz_uri, const
> >> >char *psz_name,
> >> > if( unlikely(owner == NULL) )
> >> > return NULL;
> >> >
> >> >- atomic_init( &owner->refs, 1 );
> >> >+ vlc_atomic_rc_init( &owner->rc );
> >> >
> >> > input_item_t *p_input = &owner->item;
> >> > vlc_event_manager_t * p_em = &p_input->event_manager;
> >> >diff --git a/src/input/item.h b/src/input/item.h
> >> >index dce1dfc0584..bce49777f8d 100644
> >> >--- a/src/input/item.h
> >> >+++ b/src/input/item.h
> >> >@@ -25,7 +25,7 @@
> >> > #define LIBVLC_INPUT_ITEM_H 1
> >> >
> >> > #include "input_interface.h"
> >> >-#include <stdatomic.h>
> >> >+#include <vlc_atomic.h>
> >> >
> >> >void input_item_SetErrorWhenReading( input_item_t *p_i, bool b_error
> >);
> >> >void input_item_UpdateTracksInfo( input_item_t *item, const
> >es_format_t
> >> >*fmt );
> >> >@@ -34,7 +34,7 @@ bool input_item_ShouldPreparseSubItems(
> >input_item_t
> >> >*p_i );
> >> > typedef struct input_item_owner
> >> > {
> >> > input_item_t item;
> >> >- atomic_uint refs;
> >> >+ vlc_atomic_rc_t rc;
> >> > } input_item_owner_t;
> >> >
> >> > # define item_owner(item) ((struct input_item_owner *)(item))
> >> >diff --git a/src/input/resource.c b/src/input/resource.c
> >> >index f806cfeb2c9..0cad860b106 100644
> >> >--- a/src/input/resource.c
> >> >+++ b/src/input/resource.c
> >> >@@ -28,10 +28,10 @@
> >> > # include "config.h"
> >> > #endif
> >> >
> >> >-#include <stdatomic.h>
> >> > #include <assert.h>
> >> >
> >> > #include <vlc_common.h>
> >> >+#include <vlc_atomic.h>
> >> > #include <vlc_vout.h>
> >> > #include <vlc_spu.h>
> >> > #include <vlc_aout.h>
> >> >@@ -45,7 +45,7 @@
> >> >
> >> > struct input_resource_t
> >> > {
> >> >- atomic_uint refs;
> >> >+ vlc_atomic_rc_t rc;
> >> >
> >> > vlc_object_t *p_parent;
> >> >
> >> >@@ -422,7 +422,7 @@ input_resource_t *input_resource_New(
> >vlc_object_t
> >> >*p_parent )
> >> > if( !p_resource )
> >> > return NULL;
> >> >
> >> >- atomic_init( &p_resource->refs, 1 );
> >> >+ vlc_atomic_rc_init( &p_resource->rc );
> >> > p_resource->p_parent = p_parent;
> >> > vlc_mutex_init( &p_resource->lock );
> >> > vlc_mutex_init( &p_resource->lock_hold );
> >> >@@ -431,7 +431,7 @@ input_resource_t *input_resource_New(
> >vlc_object_t
> >> >*p_parent )
> >> >
> >> > void input_resource_Release( input_resource_t *p_resource )
> >> > {
> >> >- if( atomic_fetch_sub( &p_resource->refs, 1 ) != 1 )
> >> >+ if( !vlc_atomic_rc_dec( &p_resource->rc ) )
> >> > return;
> >> >
> >> > DestroySout( p_resource );
> >> >@@ -446,7 +446,7 @@ void input_resource_Release( input_resource_t
> >> >*p_resource )
> >> >
> >> > input_resource_t *input_resource_Hold( input_resource_t *p_resource
> >)
> >> > {
> >> >- atomic_fetch_add( &p_resource->refs, 1 );
> >> >+ vlc_atomic_rc_inc( &p_resource->rc );
> >> > return p_resource;
> >> > }
> >> >
> >> >diff --git a/src/misc/picture.c b/src/misc/picture.c
> >> >index e633cee42bc..5cabd420aab 100644
> >> >--- a/src/misc/picture.c
> >> >+++ b/src/misc/picture.c
> >> >@@ -205,7 +205,7 @@ static picture_priv_t *picture_NewPrivate(const
> >> >video_format_t *restrict p_fmt)
> >> > return NULL;
> >> > }
> >> >
> >> >- atomic_init( &priv->gc.refs, 1 );
> >> >+ vlc_atomic_rc_init( &priv->gc.rc );
> >> > priv->gc.opaque = NULL;
> >> >
> >> > return priv;
> >> >@@ -305,8 +305,7 @@ picture_t *picture_Hold( picture_t *p_picture )
> >> > assert( p_picture != NULL );
> >> >
> >> > picture_priv_t *priv = (picture_priv_t *)p_picture;
> >> >- uintptr_t refs = atomic_fetch_add( &priv->gc.refs, 1 );
> >> >- assert( refs > 0 );
> >> >+ vlc_atomic_rc_inc( &priv->gc.rc );
> >> > return p_picture;
> >> > }
> >> >
> >> >@@ -315,9 +314,7 @@ void picture_Release( picture_t *p_picture )
> >> > assert( p_picture != NULL );
> >> >
> >> > picture_priv_t *priv = (picture_priv_t *)p_picture;
> >> >- uintptr_t refs = atomic_fetch_sub( &priv->gc.refs, 1 );
> >> >- assert( refs != 0 );
> >> >- if( refs > 1 )
> >> >+ if( !vlc_atomic_rc_dec( &priv->gc.rc ) )
> >> > return;
> >> >
> >> > PictureDestroyContext( p_picture );
> >> >diff --git a/src/misc/picture.h b/src/misc/picture.h
> >> >index 70ee64878da..4a70b6a6c53 100644
> >> >--- a/src/misc/picture.h
> >> >+++ b/src/misc/picture.h
> >> >@@ -18,8 +18,7 @@
> >> > * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301,
> >USA.
> >>
> >>*****************************************************************************/
> >> >
> >> >-#include <stdatomic.h>
> >> >-
> >> >+#include <vlc_atomic.h>
> >> > #include <vlc_picture.h>
> >> >
> >> > typedef struct
> >> >@@ -27,7 +26,7 @@ typedef struct
> >> > picture_t picture;
> >> > struct
> >> > {
> >> >- atomic_uintptr_t refs;
> >> >+ vlc_atomic_rc_t rc;
> >> > void (*destroy)(picture_t *);
> >> > void *opaque;
> >> > } gc;
> >> >--
> >> >2.18.0
> >> >
> >> >_______________________________________________
> >> >vlc-devel mailing list
> >> >To unsubscribe or modify your subscription options:
> >> >https://mailman.videolan.org/listinfo/vlc-devel
> >>
> >> --
> >> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
> >excuser ma brièveté.
> >
> >> _______________________________________________
> >> vlc-devel mailing list
> >> To unsubscribe or modify your subscription options:
> >> https://mailman.videolan.org/listinfo/vlc-devel
> >
> >_______________________________________________
> >vlc-devel mailing list
> >To unsubscribe or modify your subscription options:
> >https://mailman.videolan.org/listinfo/vlc-devel
>
> --
> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel
More information about the vlc-devel
mailing list