On Thu, May 17, 2012 at 06:04:13PM -0400, Mathieu Desnoyers wrote: > * Paul E. McKenney (paul...@linux.vnet.ibm.com) wrote: > > On Thu, May 17, 2012 at 01:59:43PM -0400, Mathieu Desnoyers wrote: > > > * Paul E. McKenney (paul...@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 (paul...@linux.vnet.ibm.com) wrote: > > > > > > > > On Tue, May 15, 2012 at 08:10:03AM -0400, Mathieu Desnoyers > > > > > > > > wrote: > > > > > > > > > * Paul E. McKenney (paul...@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.desnoy...@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 ?
Looks good to me! Reviewed-by: Paul E. McKenney <paul...@linux.vnet.ibm.com> > 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 > > _______________________________________________ > rp mailing list > r...@svcs.cs.pdx.edu > http://svcs.cs.pdx.edu/mailman/listinfo/rp > _______________________________________________ lttng-dev mailing list lttng-dev@lists.lttng.org http://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev