On Tue, Jan 20, 2015 at 09:03:12AM -0500, Peter Hurley wrote:
> On 01/19/2015 07:30 PM, Paul E. McKenney wrote:
> > On Tue, Jan 06, 2015 at 12:47:53PM -0800, Paul E. McKenney wrote:
> >> On Tue, Jan 06, 2015 at 02:57:37PM -0500, Peter Hurley wrote:
> >
> > [ . . . ]
> >
> >> David Miller's call, actually.
> >>
> >> But the rule is that if it is an atomic read-modify-write operation and it
> >> returns a value, then the operation itself needs to include full memory
> >> barriers before and after (as in the caller doesn't need to add them).
> >> Otherwise, the operation does not need to include memory ordering.
> >> Since xchg(), atomic_xchg(), and atomic_long_xchg() all return a value,
> >> their implementations must include full memory barriers before and after.
> >>
> >> Pretty straightforward. ;-)
> >
> > Hello again, Peter,
> >
> > Were you going to push a patch clarifying this?
>
> Hi Paul,
>
> As you pointed out, atomic_ops.txt is for arch implementors, so I wasn't
> planning on patching that file.
>
> I've been meaning to write up something specifically for everyone else but
> my own bugs have kept me from that. [That, and I'm not sure what I write
> will be suitable for Documentation.]
Well, upon revisiting this after coming back from travel, I am more inclined
to agree that a change would be good. I doubt if you would be the only
person who might be confused. So how about the following patch?
Thanx, Paul
------------------------------------------------------------------------
documentation: Clarify memory-barrier semantics of atomic operations
All value-returning atomic read-modify-write operations must provide full
memory-barrier semantics on both sides of the operation. This commit
clarifies the documentation to make it clear that these memory-barrier
semantics are provided by the operations themselves, not by their callers.
Reported-by: Peter Hurley <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
diff --git a/Documentation/atomic_ops.txt b/Documentation/atomic_ops.txt
index 183e41bdcb69..672201e79c49 100644
--- a/Documentation/atomic_ops.txt
+++ b/Documentation/atomic_ops.txt
@@ -201,11 +201,11 @@ These routines add 1 and subtract 1, respectively, from the given
atomic_t and return the new counter value after the operation is
performed.
-Unlike the above routines, it is required that explicit memory
-barriers are performed before and after the operation. It must be
-done such that all memory operations before and after the atomic
-operation calls are strongly ordered with respect to the atomic
-operation itself.
+Unlike the above routines, it is required that these primitives
+inlude explicit memory barriers that are performed before and after
+the operation. It must be done such that all memory operations before
+and after the atomic operation calls are strongly ordered with respect
+to the atomic operation itself.
For example, it should behave as if a smp_mb() call existed both
before and after the atomic operation.
@@ -233,21 +233,21 @@ These two routines increment and decrement by 1, respectively, the
given atomic counter. They return a boolean indicating whether the
resulting counter value was zero or not.
-It requires explicit memory barrier semantics around the operation as
-above.
+Again, these primitive provide explicit memory barrier semantics around
+the atomic operation.
int atomic_sub_and_test(int i, atomic_t *v);
This is identical to atomic_dec_and_test() except that an explicit
-decrement is given instead of the implicit "1". It requires explicit
-memory barrier semantics around the operation.
+decrement is given instead of the implicit "1". This primitive must
+provide explicit memory barrier semantics around the operation.
int atomic_add_negative(int i, atomic_t *v);
-The given increment is added to the given atomic counter value. A
-boolean is return which indicates whether the resulting counter value
-is negative. It requires explicit memory barrier semantics around the
-operation.
+The given increment is added to the given atomic counter value. A boolean
+is return which indicates whether the resulting counter value is negative.
+This primitive must provide explicit memory barrier semantics around
+the operation.
Then:
@@ -257,7 +257,7 @@ This performs an atomic exchange operation on the atomic variable v, setting
the given new value. It returns the old value that the atomic variable v had
just before the operation.
-atomic_xchg requires explicit memory barriers around the operation.
+atomic_xchg must provide explicit memory barriers around the operation.
int atomic_cmpxchg(atomic_t *v, int old, int new);
@@ -266,7 +266,7 @@ with the given old and new values. Like all atomic_xxx operations,
atomic_cmpxchg will only satisfy its atomicity semantics as long as all
other accesses of *v are performed through atomic_xxx operations.
-atomic_cmpxchg requires explicit memory barriers around the operation.
+atomic_cmpxchg must provide explicit memory barriers around the operation.
The semantics for atomic_cmpxchg are the same as those defined for 'cas'
below.
@@ -279,8 +279,8 @@ If the atomic value v is not equal to u, this function adds a to v, and
returns non zero. If v is equal to u then it returns zero. This is done as
an atomic operation.
-atomic_add_unless requires explicit memory barriers around the operation
-unless it fails (returns 0).
+atomic_add_unless must provide explicit memory barriers around the
+operation unless it fails (returns 0).
atomic_inc_not_zero, equivalent to atomic_add_unless(v, 1, 0)
@@ -460,9 +460,9 @@ the return value into an int. There are other places where things
like this occur as well.
These routines, like the atomic_t counter operations returning values,
-require explicit memory barrier semantics around their execution. All
-memory operations before the atomic bit operation call must be made
-visible globally before the atomic bit operation is made visible.
+must provide explicit memory barrier semantics around their execution.
+All memory operations before the atomic bit operation call must be
+made visible globally before the atomic bit operation is made visible.
Likewise, the atomic bit operation must be visible globally before any
subsequent memory operation is made visible. For example:
@@ -536,8 +536,9 @@ except that two underscores are prefixed to the interface name.
These non-atomic variants also do not require any special memory
barrier semantics.
-The routines xchg() and cmpxchg() need the same exact memory barriers
-as the atomic and bit operations returning values.
+The routines xchg() and cmpxchg() must provide the same exact
+memory-barrier semantics as the atomic and bit operations returning
+values.
Spinlocks and rwlocks have memory barrier expectations as well.
The rule to follow is simple:
On 02/02/2015 11:11 AM, Paul E. McKenney wrote:
> On Tue, Jan 20, 2015 at 09:03:12AM -0500, Peter Hurley wrote:
>> On 01/19/2015 07:30 PM, Paul E. McKenney wrote:
>>> On Tue, Jan 06, 2015 at 12:47:53PM -0800, Paul E. McKenney wrote:
>>>> On Tue, Jan 06, 2015 at 02:57:37PM -0500, Peter Hurley wrote:
>>>
>>> [ . . . ]
>>>
>>>> David Miller's call, actually.
>>>>
>>>> But the rule is that if it is an atomic read-modify-write operation and it
>>>> returns a value, then the operation itself needs to include full memory
>>>> barriers before and after (as in the caller doesn't need to add them).
>>>> Otherwise, the operation does not need to include memory ordering.
>>>> Since xchg(), atomic_xchg(), and atomic_long_xchg() all return a value,
>>>> their implementations must include full memory barriers before and after.
>>>>
>>>> Pretty straightforward. ;-)
>>>
>>> Hello again, Peter,
>>>
>>> Were you going to push a patch clarifying this?
>>
>> Hi Paul,
>>
>> As you pointed out, atomic_ops.txt is for arch implementors, so I wasn't
>> planning on patching that file.
>>
>> I've been meaning to write up something specifically for everyone else but
>> my own bugs have kept me from that. [That, and I'm not sure what I write
>> will be suitable for Documentation.]
>
> Well, upon revisiting this after coming back from travel, I am more inclined
> to agree that a change would be good. I doubt if you would be the only
> person who might be confused. So how about the following patch?
I think this is much clearer to both audiences. Thanks!
Regards,
Peter Hurley
[minor corrections below]
> ------------------------------------------------------------------------
>
> documentation: Clarify memory-barrier semantics of atomic operations
>
> All value-returning atomic read-modify-write operations must provide full
> memory-barrier semantics on both sides of the operation. This commit
> clarifies the documentation to make it clear that these memory-barrier
> semantics are provided by the operations themselves, not by their callers.
>
> Reported-by: Peter Hurley <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
>
> diff --git a/Documentation/atomic_ops.txt b/Documentation/atomic_ops.txt
> index 183e41bdcb69..672201e79c49 100644
> --- a/Documentation/atomic_ops.txt
> +++ b/Documentation/atomic_ops.txt
> @@ -201,11 +201,11 @@ These routines add 1 and subtract 1, respectively, from the given
> atomic_t and return the new counter value after the operation is
> performed.
>
> -Unlike the above routines, it is required that explicit memory
> -barriers are performed before and after the operation. It must be
> -done such that all memory operations before and after the atomic
> -operation calls are strongly ordered with respect to the atomic
> -operation itself.
> +Unlike the above routines, it is required that these primitives
> +inlude explicit memory barriers that are performed before and after
^^^^^^
include|provide
> +the operation. It must be done such that all memory operations before
> +and after the atomic operation calls are strongly ordered with respect
> +to the atomic operation itself.
>
> For example, it should behave as if a smp_mb() call existed both
> before and after the atomic operation.
> @@ -233,21 +233,21 @@ These two routines increment and decrement by 1, respectively, the
> given atomic counter. They return a boolean indicating whether the
> resulting counter value was zero or not.
>
> -It requires explicit memory barrier semantics around the operation as
> -above.
> +Again, these primitive provide explicit memory barrier semantics around
^
s
> +the atomic operation.
>
> int atomic_sub_and_test(int i, atomic_t *v);
>
> This is identical to atomic_dec_and_test() except that an explicit
> -decrement is given instead of the implicit "1". It requires explicit
> -memory barrier semantics around the operation.
> +decrement is given instead of the implicit "1". This primitive must
> +provide explicit memory barrier semantics around the operation.
>
> int atomic_add_negative(int i, atomic_t *v);
>
> -The given increment is added to the given atomic counter value. A
> -boolean is return which indicates whether the resulting counter value
> -is negative. It requires explicit memory barrier semantics around the
> -operation.
> +The given increment is added to the given atomic counter value. A boolean
> +is return which indicates whether the resulting counter value is negative.
> +This primitive must provide explicit memory barrier semantics around
> +the operation.
>
> Then:
>
> @@ -257,7 +257,7 @@ This performs an atomic exchange operation on the atomic variable v, setting
> the given new value. It returns the old value that the atomic variable v had
> just before the operation.
>
> -atomic_xchg requires explicit memory barriers around the operation.
> +atomic_xchg must provide explicit memory barriers around the operation.
>
> int atomic_cmpxchg(atomic_t *v, int old, int new);
>
> @@ -266,7 +266,7 @@ with the given old and new values. Like all atomic_xxx operations,
> atomic_cmpxchg will only satisfy its atomicity semantics as long as all
> other accesses of *v are performed through atomic_xxx operations.
>
> -atomic_cmpxchg requires explicit memory barriers around the operation.
> +atomic_cmpxchg must provide explicit memory barriers around the operation.
>
> The semantics for atomic_cmpxchg are the same as those defined for 'cas'
> below.
> @@ -279,8 +279,8 @@ If the atomic value v is not equal to u, this function adds a to v, and
> returns non zero. If v is equal to u then it returns zero. This is done as
> an atomic operation.
>
> -atomic_add_unless requires explicit memory barriers around the operation
> -unless it fails (returns 0).
> +atomic_add_unless must provide explicit memory barriers around the
> +operation unless it fails (returns 0).
>
> atomic_inc_not_zero, equivalent to atomic_add_unless(v, 1, 0)
>
> @@ -460,9 +460,9 @@ the return value into an int. There are other places where things
> like this occur as well.
>
> These routines, like the atomic_t counter operations returning values,
> -require explicit memory barrier semantics around their execution. All
> -memory operations before the atomic bit operation call must be made
> -visible globally before the atomic bit operation is made visible.
> +must provide explicit memory barrier semantics around their execution.
> +All memory operations before the atomic bit operation call must be
> +made visible globally before the atomic bit operation is made visible.
> Likewise, the atomic bit operation must be visible globally before any
> subsequent memory operation is made visible. For example:
>
> @@ -536,8 +536,9 @@ except that two underscores are prefixed to the interface name.
> These non-atomic variants also do not require any special memory
> barrier semantics.
>
> -The routines xchg() and cmpxchg() need the same exact memory barriers
> -as the atomic and bit operations returning values.
> +The routines xchg() and cmpxchg() must provide the same exact
> +memory-barrier semantics as the atomic and bit operations returning
> +values.
>
> Spinlocks and rwlocks have memory barrier expectations as well.
> The rule to follow is simple:
>
On Mon, Feb 02, 2015 at 02:03:33PM -0500, Peter Hurley wrote:
> On 02/02/2015 11:11 AM, Paul E. McKenney wrote:
> > On Tue, Jan 20, 2015 at 09:03:12AM -0500, Peter Hurley wrote:
> >> On 01/19/2015 07:30 PM, Paul E. McKenney wrote:
> >>> On Tue, Jan 06, 2015 at 12:47:53PM -0800, Paul E. McKenney wrote:
> >>>> On Tue, Jan 06, 2015 at 02:57:37PM -0500, Peter Hurley wrote:
> >>>
> >>> [ . . . ]
> >>>
> >>>> David Miller's call, actually.
> >>>>
> >>>> But the rule is that if it is an atomic read-modify-write operation and it
> >>>> returns a value, then the operation itself needs to include full memory
> >>>> barriers before and after (as in the caller doesn't need to add them).
> >>>> Otherwise, the operation does not need to include memory ordering.
> >>>> Since xchg(), atomic_xchg(), and atomic_long_xchg() all return a value,
> >>>> their implementations must include full memory barriers before and after.
> >>>>
> >>>> Pretty straightforward. ;-)
> >>>
> >>> Hello again, Peter,
> >>>
> >>> Were you going to push a patch clarifying this?
> >>
> >> Hi Paul,
> >>
> >> As you pointed out, atomic_ops.txt is for arch implementors, so I wasn't
> >> planning on patching that file.
> >>
> >> I've been meaning to write up something specifically for everyone else but
> >> my own bugs have kept me from that. [That, and I'm not sure what I write
> >> will be suitable for Documentation.]
> >
> > Well, upon revisiting this after coming back from travel, I am more inclined
> > to agree that a change would be good. I doubt if you would be the only
> > person who might be confused. So how about the following patch?
>
> I think this is much clearer to both audiences. Thanks!
>
> Regards,
> Peter Hurley
>
> [minor corrections below]
>
> > ------------------------------------------------------------------------
> >
> > documentation: Clarify memory-barrier semantics of atomic operations
> >
> > All value-returning atomic read-modify-write operations must provide full
> > memory-barrier semantics on both sides of the operation. This commit
> > clarifies the documentation to make it clear that these memory-barrier
> > semantics are provided by the operations themselves, not by their callers.
> >
> > Reported-by: Peter Hurley <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> >
> > diff --git a/Documentation/atomic_ops.txt b/Documentation/atomic_ops.txt
> > index 183e41bdcb69..672201e79c49 100644
> > --- a/Documentation/atomic_ops.txt
> > +++ b/Documentation/atomic_ops.txt
> > @@ -201,11 +201,11 @@ These routines add 1 and subtract 1, respectively, from the given
> > atomic_t and return the new counter value after the operation is
> > performed.
> >
> > -Unlike the above routines, it is required that explicit memory
> > -barriers are performed before and after the operation. It must be
> > -done such that all memory operations before and after the atomic
> > -operation calls are strongly ordered with respect to the atomic
> > -operation itself.
> > +Unlike the above routines, it is required that these primitives
> > +inlude explicit memory barriers that are performed before and after
> ^^^^^^
> include|provide
Good catch, switched to "include".
> > +the operation. It must be done such that all memory operations before
> > +and after the atomic operation calls are strongly ordered with respect
> > +to the atomic operation itself.
> >
> > For example, it should behave as if a smp_mb() call existed both
> > before and after the atomic operation.
> > @@ -233,21 +233,21 @@ These two routines increment and decrement by 1, respectively, the
> > given atomic counter. They return a boolean indicating whether the
> > resulting counter value was zero or not.
> >
> > -It requires explicit memory barrier semantics around the operation as
> > -above.
> > +Again, these primitive provide explicit memory barrier semantics around
> ^
> s
Good eyes, fixed.
Thanx, Paul
> > +the atomic operation.
> >
> > int atomic_sub_and_test(int i, atomic_t *v);
> >
> > This is identical to atomic_dec_and_test() except that an explicit
> > -decrement is given instead of the implicit "1". It requires explicit
> > -memory barrier semantics around the operation.
> > +decrement is given instead of the implicit "1". This primitive must
> > +provide explicit memory barrier semantics around the operation.
> >
> > int atomic_add_negative(int i, atomic_t *v);
> >
> > -The given increment is added to the given atomic counter value. A
> > -boolean is return which indicates whether the resulting counter value
> > -is negative. It requires explicit memory barrier semantics around the
> > -operation.
> > +The given increment is added to the given atomic counter value. A boolean
> > +is return which indicates whether the resulting counter value is negative.
> > +This primitive must provide explicit memory barrier semantics around
> > +the operation.
> >
> > Then:
> >
> > @@ -257,7 +257,7 @@ This performs an atomic exchange operation on the atomic variable v, setting
> > the given new value. It returns the old value that the atomic variable v had
> > just before the operation.
> >
> > -atomic_xchg requires explicit memory barriers around the operation.
> > +atomic_xchg must provide explicit memory barriers around the operation.
> >
> > int atomic_cmpxchg(atomic_t *v, int old, int new);
> >
> > @@ -266,7 +266,7 @@ with the given old and new values. Like all atomic_xxx operations,
> > atomic_cmpxchg will only satisfy its atomicity semantics as long as all
> > other accesses of *v are performed through atomic_xxx operations.
> >
> > -atomic_cmpxchg requires explicit memory barriers around the operation.
> > +atomic_cmpxchg must provide explicit memory barriers around the operation.
> >
> > The semantics for atomic_cmpxchg are the same as those defined for 'cas'
> > below.
> > @@ -279,8 +279,8 @@ If the atomic value v is not equal to u, this function adds a to v, and
> > returns non zero. If v is equal to u then it returns zero. This is done as
> > an atomic operation.
> >
> > -atomic_add_unless requires explicit memory barriers around the operation
> > -unless it fails (returns 0).
> > +atomic_add_unless must provide explicit memory barriers around the
> > +operation unless it fails (returns 0).
> >
> > atomic_inc_not_zero, equivalent to atomic_add_unless(v, 1, 0)
> >
> > @@ -460,9 +460,9 @@ the return value into an int. There are other places where things
> > like this occur as well.
> >
> > These routines, like the atomic_t counter operations returning values,
> > -require explicit memory barrier semantics around their execution. All
> > -memory operations before the atomic bit operation call must be made
> > -visible globally before the atomic bit operation is made visible.
> > +must provide explicit memory barrier semantics around their execution.
> > +All memory operations before the atomic bit operation call must be
> > +made visible globally before the atomic bit operation is made visible.
> > Likewise, the atomic bit operation must be visible globally before any
> > subsequent memory operation is made visible. For example:
> >
> > @@ -536,8 +536,9 @@ except that two underscores are prefixed to the interface name.
> > These non-atomic variants also do not require any special memory
> > barrier semantics.
> >
> > -The routines xchg() and cmpxchg() need the same exact memory barriers
> > -as the atomic and bit operations returning values.
> > +The routines xchg() and cmpxchg() must provide the same exact
> > +memory-barrier semantics as the atomic and bit operations returning
> > +values.
> >
> > Spinlocks and rwlocks have memory barrier expectations as well.
> > The rule to follow is simple:
> >
>