[lttng-dev] [rp] [RFC PATCH urcu] Document uatomic operations

Mathieu Desnoyers mathieu.desnoyers at efficios.com
Thu May 17 18:04:13 EDT 2012


* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Thu, May 17, 2012 at 01:59:43PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > On Wed, May 16, 2012 at 11:45:39AM -0700, Josh Triplett wrote:
> > > > On Wed, May 16, 2012 at 11:32:38AM -0700, Paul E. McKenney wrote:
> > > > > On Wed, May 16, 2012 at 02:17:42PM -0400, Mathieu Desnoyers wrote:
> > > > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > > > On Tue, May 15, 2012 at 08:10:03AM -0400, Mathieu Desnoyers wrote:
> > > > > > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > > > > > On Mon, May 14, 2012 at 10:39:01PM -0400, Mathieu Desnoyers wrote:
> > > > > > > > > > Document each atomic operation provided by urcu/uatomic.h, along with
> > > > > > > > > > their memory barrier guarantees.
> > > > > > > > > 
> > > > > > > > > Great to see the documentation!!!  Some comments below.
> > > > > > > > > 
> > > > > > > > > 							Thanx, Paul
> > > > > > > > > 
> > > > > > > > > > Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers at efficios.com>
> > > > > > > > > > ---
> > > > > > > > > > diff --git a/doc/Makefile.am b/doc/Makefile.am
> > > > > > > > > > index bec1d7c..db9811c 100644
> > > > > > > > > > --- a/doc/Makefile.am
> > > > > > > > > > +++ b/doc/Makefile.am
> > > > > > > > > > @@ -1 +1 @@
> > > > > > > > > > -dist_doc_DATA = rcu-api.txt
> > > > > > > > > > +dist_doc_DATA = rcu-api.txt uatomic-api.txt
> > > > > > > > > > diff --git a/doc/uatomic-api.txt b/doc/uatomic-api.txt
> > > > > > > > > > new file mode 100644
> > > > > > > > > > index 0000000..3605acf
> > > > > > > > > > --- /dev/null
> > > > > > > > > > +++ b/doc/uatomic-api.txt
> > > > > > > > > > @@ -0,0 +1,80 @@
> > > > > > > > > > +Userspace RCU Atomic Operations API
> > > > > > > > > > +by Mathieu Desnoyers and Paul E. McKenney
> > > > > > > > > > +
> > > > > > > > > > +
> > > > > > > > > > +This document describes the <urcu/uatomic.h> API. Those are the atomic
> > > > > > > > > > +operations provided by the Userspace RCU library. The general rule
> > > > > > > > > > +regarding memory barriers is that only uatomic_xchg(),
> > > > > > > > > > +uatomic_cmpxchg(), uatomic_add_return(), and uatomic_sub_return() imply
> > > > > > > > > > +full memory barriers before and after the atomic operation. Other
> > > > > > > > > > +primitives don't guarantee any memory barrier.
> > > > > > > > > > +
> > > > > > > > > > +Only atomic operations performed on integers ("int" and "long", signed
> > > > > > > > > > +and unsigned) are supported on all architectures. Some architectures
> > > > > > > > > > +also support 1-byte and 2-byte atomic operations. Those respectively
> > > > > > > > > > +have UATOMIC_HAS_ATOMIC_BYTE and UATOMIC_HAS_ATOMIC_SHORT defined when
> > > > > > > > > > +uatomic.h is included. An architecture trying to perform an atomic write
> > > > > > > > > > +to a type size not supported by the architecture will trigger an illegal
> > > > > > > > > > +instruction.
> > > > > > > > > > +
> > > > > > > > > > +In the description below, "type" is a type that can be atomically
> > > > > > > > > > +written to by the architecture. It needs to be at most word-sized, and
> > > > > > > > > > +its alignment needs to greater or equal to its size.
> > > > > > > > > > +
> > > > > > > > > > +type uatomic_set(type *addr, type v)
> > > > > > > > > > +
> > > > > > > > > > +	Atomically write @v into @addr.
> > > > > > > > > 
> > > > > > > > > Wouldn't this instead be "void uatomic_set(type *addr, type v)"?
> > > > > > > > 
> > > > > > > > Well, in that case, we'd need to change the macro. Currently,
> > > > > > > > _uatomic_set maps directly to:
> > > > > > > > 
> > > > > > > > #define _uatomic_set(addr, v)   CMM_STORE_SHARED(*(addr), (v))
> > > > > > > > 
> > > > > > > > and CMM_STORE_SHARED returns v. The question becomes: should we change
> > > > > > > > _uatomic_set or CMM_STORE_SHARED so they don't return anything, or
> > > > > > > > document that they return something ?
> > > > > > > > 
> > > > > > > > One thing I noticed is that linters often complain that the return value
> > > > > > > > of CMM_STORE_SHARED is never used. One thing we could look into is to
> > > > > > > > try some gcc attributes and/or linter annotations to flag this return
> > > > > > > > value as possibly unused. Thoughts ?
> > > > > > > 
> > > > > > > Hmmm...
> > > > > > > 
> > > > > > > Does the following work?
> > > > > > > 
> > > > > > > #define _uatomic_set(addr, v)   ((void)CMM_STORE_SHARED(*(addr), (v)))
> > > > > > 
> > > > > > Well, it would work, yes, but then we would not be consistent between
> > > > > > return values or no return values of:
> > > > > > 
> > > > > > uatomic_set()
> > > > > > rcu_assign_pointer()
> > > > > > rcu_set_pointer()
> > > > > > 
> > > > > > if you notice, in the Linux kernel, rcu_assign_pointer returns the
> > > > > > new pointer value. But you are right that atomic_set() does not return
> > > > > > anything. So which consistency would be best to keep ?
> > > > > 
> > > > > Hmmm...  I wonder how many people actually use rcu_assign_pointer()'s
> > > > > return value?  If no one, I should make it a do-while(0).  Although
> > > > > cscope does not show anything, I should probably put together a
> > > > > coccinelle script.
> > > > 
> > > > I just searched the entire Linux kernel with git grep
> > > > '\S.*rcu_assign_pointer' (any non-whitespace preceding
> > > > rcu_assign_pointer), and found no instances of anything assuming a
> > > > return value from rcu_assign_pointer.  I'd recommend making it void.
> > > 
> > > Woo-hoo!!!  Thank you both!!!
> > 
> > I'm not sure.. making rcu_assign_pointer() void we would be changing the
> > semantic, which previously was that of an assignment in C, to something
> > else.
> > 
> > In C, if we have the expression:
> > 
> > var = value;
> > 
> > This whole expression evaluates as "value". So we can have e.g.
> > 
> > do {
> >  ...
> > } while ((var = value) != other);
> > 
> > If the semantic we give to "rcu_assign_pointer" is the same as
> > assignment operator "=", then we could think that the following should
> > be allowed:
> > 
> > do {
> >  ...
> > } while (rcu_assign_pointer(addr, value) != other);
> > 
> > But making rcu_assign_pointer "void" would not allow that. So the
> > question I'd ask here is: what do we gain by changing the semantic
> > compared to C assignment operator ?
> 
> It is pretty easy to handle this with a void rcu_assign_pointer():
> 
> do {
> 	...
> 	rcu_assign_pointer(addr, value);
> } while (value != other);
> 
> If "value" is an expression involving side-effects, then yes,
> you would need a temporary.  But there are several hundred uses of
> rcu_assign_pointer() in the Linux kernel, and none of them are
> in a while-loop expression.
> 
> So I am in favor of making rcu_assign_pointer() void.

OK. How about the following patch then ?


diff --git a/urcu-pointer.h b/urcu-pointer.h
index dd64ec4..1e1e6bf 100644
--- a/urcu-pointer.h
+++ b/urcu-pointer.h
@@ -47,9 +47,9 @@ extern "C" {
 #define rcu_dereference		_rcu_dereference
 
 /*
- * rcu_cmpxchg_pointer(type **ptr, type *new, type *old)
+ * type *rcu_cmpxchg_pointer(type **ptr, type *new, type *old)
  * type *rcu_xchg_pointer(type **ptr, type *new)
- * type *rcu_set_pointer(type **ptr, type *new)
+ * void rcu_set_pointer(type **ptr, type *new)
  *
  * RCU pointer updates.
  * @ptr: address of the pointer to modify
@@ -94,20 +94,24 @@ extern void *rcu_xchg_pointer_sym(void **p, void *v);
 		(_________p1);						     \
 	})
 
+/*
+ * Note: rcu_set_pointer_sym returns @v because we don't want to break
+ * the ABI. At the API level, rcu_set_pointer() now returns void. Use of
+ * the return value is therefore deprecated, and will cause a build
+ * error.
+ */
 extern void *rcu_set_pointer_sym(void **p, void *v);
 #define rcu_set_pointer(p, v)						     \
-	({								     \
+	do {								     \
 		typeof(*(p)) _________pv = (v);			             \
-		typeof(*(p)) _________p1 = URCU_FORCE_CAST(typeof(*(p)),     \
-			rcu_set_pointer_sym(URCU_FORCE_CAST(void **, p),     \
-					    _________pv));		     \
-		(_________p1);						     \
-	})
+		(void) rcu_set_pointer_sym(URCU_FORCE_CAST(void **, p),	     \
+					    _________pv);		     \
+	} while (0)
 
 #endif /* !_LGPL_SOURCE */
 
 /*
- * rcu_assign_pointer(type *ptr, type *new)
+ * void rcu_assign_pointer(type *ptr, type *new)
  *
  * Same as rcu_set_pointer, but takes the pointer to assign to rather than its
  * address as first parameter. Provided for compatibility with the Linux kernel
diff --git a/urcu/static/urcu-pointer.h b/urcu/static/urcu-pointer.h
index acd7cee..906caa0 100644
--- a/urcu/static/urcu-pointer.h
+++ b/urcu/static/urcu-pointer.h
@@ -102,13 +102,13 @@ extern "C" {
 
 
 #define _rcu_set_pointer(p, v)				\
-	({						\
+	do {						\
 		typeof(*p) _________pv = (v);		\
 		if (!__builtin_constant_p(v) || 	\
 		    ((v) != NULL))			\
 			cmm_wmb();				\
 		uatomic_set(p, _________pv);		\
-	})
+	} while (0)
 
 /**
  * _rcu_assign_pointer - assign (publicize) a pointer to a new data structure
diff --git a/urcu/uatomic/generic.h b/urcu/uatomic/generic.h
index bfd9b68..9e2e780 100644
--- a/urcu/uatomic/generic.h
+++ b/urcu/uatomic/generic.h
@@ -29,7 +29,7 @@ extern "C" {
 #endif
 
 #ifndef uatomic_set
-#define uatomic_set(addr, v)	CMM_STORE_SHARED(*(addr), (v))
+#define uatomic_set(addr, v)	((void) CMM_STORE_SHARED(*(addr), (v)))
 #endif
 
 #ifndef uatomic_read


Thanks,

Mathieu

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com



More information about the lttng-dev mailing list