Hi,
This is v3 of the series.
Link for v1: https://lkml.org/lkml/2015/8/27/798
Link for v2: https://lkml.org/lkml/2015/9/16/527
Paul, Peter and Will, thank you all for the comments and suggestions,
that's really a lot of fun to discuss these with you and very
enlightening to me ;-)
Changes since v2:
* reorder the patches to put the fix of cmpxchg, xchg and their
atomic_ versions first, and Cc stable. (Peter Zijlstra)
* modify the commit log for implementation of cmpxchg family to
explain why we implement some operation using assembly code
(Peter Zijlstra)
* add implementation and test for {inc,dec}_return atomics.
(Will Deacon)
* rebase on current locking/core branch of tip tree (commit
00eb4bab69db3)
* rewrite the macros for generating tests to save some lines of
code
Relaxed/acquire/release variants of atomic operations {add,sub}_return
and {cmp,}xchg are introduced by commit:
"atomics: add acquire/release/relaxed variants of some atomic operations"
and {inc,dec}_return has been introduced by commit:
"locking/asm-generic: Add _{relaxed|acquire|release}() variants for
inc/dec atomics"
Both of these are in the current locking/core branch of the tip tree.
By default, the generic code will implement a relaxed variant as a full
ordered atomic operation and release/acquire a variant as a relaxed
variant with a necessary general barrier before or after.
On powerpc, which has a weak memory order model, a relaxed variant can
be implemented more lightweightly than a full ordered one. Further more,
release and acquire variants can be implemented with arch-specific
lightweight barriers.
Besides, cmpxchg, xchg and their atomic_ versions are only RELEASE+ACQUIRE
rather that full barriers in current PPC implementation, which is
incorrect according to memory-barriers.txt.
Therefore this patchset fix the order guarantee of cmpxchg, xchg and
their atomic_ versions and implements the relaxed/acquire/release
variants based on powerpc memory model and specific barriers, Some
trivial tests for these new variants are also included in this series,
because some of these variants are not used in kernel for now, I think
is a good idea to at least generate the code for these variants
somewhere.
The patchset consists of 6 parts:
1. Make xchg, cmpxchg and their atomic_ versions a full barrier
2. Add trivial tests for the new variants in lib/atomic64_test.c
3. Allow architectures to define their own __atomic_op_*() helpers
to build other variants based on relaxed.
4. Implement atomic{,64}_{add,sub,inc,dec}_return_* variants
5. Implement xchg_* and atomic{,64}_xchg_* variants
6. Implement cmpxchg_* atomic{,64}_cmpxchg_* variants
This patchset is based on current locking/core branch of the tip tree
and all patches are built and boot tested for little endian pseries, and
also tested by 0day.
Looking forward to any suggestion, question and comment ;-)
Regards,
Boqun
According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
versions all need to imply a full barrier, however they are now just
RELEASE+ACQUIRE, which is not a full barrier.
So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
__{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
This patch is a complement of commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics").
Cc: stable@vger.kernel.org # 3.4.y-
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Some atomic operations now have _{relaxed, acquire, release} variants,
this patch then adds some trivial tests for two purpose:
1. test the behavior of these new operations in single-CPU
environment.
2. make their code generated before we actually use them somewhere,
so that we can examine their assembly code.
Signed-off-by: Boqun Feng <redacted>
---
lib/atomic64_test.c | 120 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 79 insertions(+), 41 deletions(-)
Some architectures may have their special barriers for acquire, release
and fence semantics, so that general memory barriers(smp_mb__*_atomic())
in the default __atomic_op_*() may be too strong, so allow architectures
to define their own helpers which can overwrite the default helpers.
Signed-off-by: Boqun Feng <redacted>
---
include/linux/atomic.h | 10 ++++++++++
1 file changed, 10 insertions(+)
On powerpc, acquire and release semantics can be achieved with
lightweight barriers("lwsync" and "ctrl+isync"), which can be used to
implement __atomic_op_{acquire,release}.
For release semantics, since we only need to ensure all memory accesses
that issue before must take effects before the -store- part of the
atomics, "lwsync" is what we only need. On the platform without
"lwsync", "sync" should be used. Therefore, smp_lwsync() is used here.
For acquire semantics, "lwsync" is what we only need for the similar
reason. However on the platform without "lwsync", we can use "isync"
rather than "sync" as an acquire barrier. So a new kind of barrier
smp_acquire_barrier__after_atomic() is introduced, which is barrier() on
UP, "lwsync" if available and "isync" otherwise.
__atomic_op_fence is defined as smp_lwsync() + _relaxed +
smp_mb__after_atomic() to guarantee a full barrier.
Implement atomic{,64}_{add,sub,inc,dec}_return_relaxed, and build other
variants with these helpers.
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/atomic.h | 122 +++++++++++++++++++++++++-------------
1 file changed, 80 insertions(+), 42 deletions(-)
Implement xchg_relaxed and atomic{,64}_xchg_relaxed, based on these
_relaxed variants, release/acquire variants and fully ordered versions
can be built.
Note that xchg_relaxed and atomic_{,64}_xchg_relaxed are not compiler
barriers.
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/atomic.h | 2 ++
arch/powerpc/include/asm/cmpxchg.h | 69 +++++++++++++++++---------------------
2 files changed, 32 insertions(+), 39 deletions(-)
Implement cmpxchg{,64}_relaxed and atomic{,64}_cmpxchg_relaxed, based on
which _release variants can be built.
To avoid superfluous barriers in _acquire variants, we implement these
operations with assembly code rather use __atomic_op_acquire() to build
them automatically.
For the same reason, we keep the assembly implementation of fully
ordered cmpxchg operations.
Note cmpxchg{,64}_relaxed and atomic{,64}_cmpxchg_relaxed are not
compiler barriers.
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/atomic.h | 10 +++
arch/powerpc/include/asm/cmpxchg.h | 141 ++++++++++++++++++++++++++++++++++++-
2 files changed, 150 insertions(+), 1 deletion(-)
Oops.. sorry. I will resend this one with correct address list.
On Mon, Oct 12, 2015 at 10:14:01PM +0800, Boqun Feng wrote:
quoted hunk
According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
versions all need to imply a full barrier, however they are now just
RELEASE+ACQUIRE, which is not a full barrier.
So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
__{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
This patch is a complement of commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics").
Cc: stable@vger.kernel.org # 3.4.y-
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
versions all need to imply a full barrier, however they are now just
RELEASE+ACQUIRE, which is not a full barrier.
So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
__{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
This patch is a complement of commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics").
Cc: <redacted> # 3.4.y-
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-10-13 12:27:33
On Mon, Oct 12, 2015 at 10:14:00PM +0800, Boqun Feng wrote:
The patchset consists of 6 parts:
1. Make xchg, cmpxchg and their atomic_ versions a full barrier
2. Add trivial tests for the new variants in lib/atomic64_test.c
3. Allow architectures to define their own __atomic_op_*() helpers
to build other variants based on relaxed.
4. Implement atomic{,64}_{add,sub,inc,dec}_return_* variants
5. Implement xchg_* and atomic{,64}_xchg_* variants
6. Implement cmpxchg_* atomic{,64}_cmpxchg_* variants
This patchset is based on current locking/core branch of the tip tree
and all patches are built and boot tested for little endian pseries, and
also tested by 0day.
I don't see any immediate problems with this series at this point. Will,
Paul?
From: Will Deacon <hidden> Date: 2015-10-13 13:21:35
On Mon, Oct 12, 2015 at 10:14:04PM +0800, Boqun Feng wrote:
quoted hunk
On powerpc, acquire and release semantics can be achieved with
lightweight barriers("lwsync" and "ctrl+isync"), which can be used to
implement __atomic_op_{acquire,release}.
For release semantics, since we only need to ensure all memory accesses
that issue before must take effects before the -store- part of the
atomics, "lwsync" is what we only need. On the platform without
"lwsync", "sync" should be used. Therefore, smp_lwsync() is used here.
For acquire semantics, "lwsync" is what we only need for the similar
reason. However on the platform without "lwsync", we can use "isync"
rather than "sync" as an acquire barrier. So a new kind of barrier
smp_acquire_barrier__after_atomic() is introduced, which is barrier() on
UP, "lwsync" if available and "isync" otherwise.
__atomic_op_fence is defined as smp_lwsync() + _relaxed +
smp_mb__after_atomic() to guarantee a full barrier.
Implement atomic{,64}_{add,sub,inc,dec}_return_relaxed, and build other
variants with these helpers.
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/atomic.h | 122 +++++++++++++++++++++++++-------------
1 file changed, 80 insertions(+), 42 deletions(-)
I'm not keen on this barrier, as it sounds like it's part of the kernel
memory model, as opposed to an implementation detail on PowerPC (and
we've already got enough of that in the generic code ;).
Can you name it something different please (and maybe #undef it when
you're done)?
Will
From: Will Deacon <hidden> Date: 2015-10-13 13:24:08
On Mon, Oct 12, 2015 at 10:14:06PM +0800, Boqun Feng wrote:
Implement cmpxchg{,64}_relaxed and atomic{,64}_cmpxchg_relaxed, based on
which _release variants can be built.
To avoid superfluous barriers in _acquire variants, we implement these
operations with assembly code rather use __atomic_op_acquire() to build
them automatically.
The "superfluous barriers" are for the case where the cmpxchg fails, right?
And you don't do the same thing for release, because you want to avoid a
barrier in the middle of the critical section?
(just checking I understand your reasoning).
Will
On Tue, Oct 13, 2015 at 02:21:32PM +0100, Will Deacon wrote:
On Mon, Oct 12, 2015 at 10:14:04PM +0800, Boqun Feng wrote:
[snip]
quoted
+/*
+ * Since {add,sub}_return_relaxed and xchg_relaxed are implemented with
+ * a "bne-" instruction at the end, so an isync is enough as a acquire barrier
+ * on the platform without lwsync.
+ */
+#ifdef CONFIG_SMP
+#define smp_acquire_barrier__after_atomic() \
+ __asm__ __volatile__(PPC_ACQUIRE_BARRIER : : : "memory")
I'm not keen on this barrier, as it sounds like it's part of the kernel
memory model, as opposed to an implementation detail on PowerPC (and
we've already got enough of that in the generic code ;).
Indeed, but we still have smp_lwsync() ;-)
Can you name it something different please (and maybe #undef it when
you're done)?
I've considered #undef it after used, but now I think open code this
into __atomic_op_acquire() of PPC is a better idea?
#define __atomic_op_acquire(op, args...) \
({ \
typeof(op##_relaxed(args)) __ret = op##_relaxed(args); \
__asm__ __volatile__(PPC_ACQUIRE_BARRIER : : : "memory"); \
__ret; \
})
PPC_ACQUIRE_BARRIER will be empty if !SMP, so that will become a pure
compiler barrier and just what we need.
Regards,
Boqun
On Tue, Oct 13, 2015 at 02:24:04PM +0100, Will Deacon wrote:
On Mon, Oct 12, 2015 at 10:14:06PM +0800, Boqun Feng wrote:
quoted
Implement cmpxchg{,64}_relaxed and atomic{,64}_cmpxchg_relaxed, based on
which _release variants can be built.
To avoid superfluous barriers in _acquire variants, we implement these
operations with assembly code rather use __atomic_op_acquire() to build
them automatically.
The "superfluous barriers" are for the case where the cmpxchg fails, right?
Yes.
And you don't do the same thing for release, because you want to avoid a
barrier in the middle of the critical section?
Mostly because of the comments in include/linux/atomic.h:
* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failed cmpxchg_acquire
* does -not- imply any memory ordering constraints.
so I thought only the barrier in cmpxchg_acquire() is conditional, and
the barrier in cmpxchg_release() is not. Maybe we'd better call it out
that cmpxchg *family* doesn't have any order guarantee if cmp fails, as
a complement of
ed2de9f74ecb ("locking/Documentation: Clarify failed cmpxchg() memory ordering semantics")
Because it seems this commit only claims that the barriers in fully
ordered version are conditional.
If cmpxchg_release doesn't have order guarantee when failed, I guess I
can implement it with a barrier in the middle as you mentioned:
unsigned int prev;
__asm__ __volatile__ (
"1: lwarx %0,0,%2
cmpw 0,%0,%3\n\
bne- 2f\n"
PPC_RELEASE_BARRIER
" stwcx. %4,0,%2\n\
bne- 1b"
"\n\
2:"
: "=&r" (prev), "+m" (*p)
: "r" (p), "r" (old), "r" (new)
: "cc", "memory");
return prev;
However, I need to check whether the architecture allows this and any
other problem exists.
Besides, I don't think it's a good idea to do the "put barrier in the
middle" thing in this patchset, because that seems a premature
optimization and if we go further, I guess we can also replace the
PPC_RELEASE_BARRIER above with a "sync" to implement a fully ordered
version cmpxchg(). Too much needs to investigate then..
(just checking I understand your reasoning).
That actually helps me find a probably better implementation if allowed,
thank you ;-)
Regards,
Boqun
From: Will Deacon <hidden> Date: 2015-10-13 14:43:36
On Tue, Oct 13, 2015 at 10:32:59PM +0800, Boqun Feng wrote:
On Tue, Oct 13, 2015 at 02:24:04PM +0100, Will Deacon wrote:
quoted
On Mon, Oct 12, 2015 at 10:14:06PM +0800, Boqun Feng wrote:
quoted
Implement cmpxchg{,64}_relaxed and atomic{,64}_cmpxchg_relaxed, based on
which _release variants can be built.
To avoid superfluous barriers in _acquire variants, we implement these
operations with assembly code rather use __atomic_op_acquire() to build
them automatically.
The "superfluous barriers" are for the case where the cmpxchg fails, right?
Yes.
quoted
And you don't do the same thing for release, because you want to avoid a
barrier in the middle of the critical section?
Mostly because of the comments in include/linux/atomic.h:
* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failed cmpxchg_acquire
* does -not- imply any memory ordering constraints.
so I thought only the barrier in cmpxchg_acquire() is conditional, and
the barrier in cmpxchg_release() is not. Maybe we'd better call it out
that cmpxchg *family* doesn't have any order guarantee if cmp fails, as
a complement of
ed2de9f74ecb ("locking/Documentation: Clarify failed cmpxchg() memory ordering semantics")
Because it seems this commit only claims that the barriers in fully
ordered version are conditional.
I didn't think this was ambiguous... A failed cmpxchg_release doesn't
perform a store, so because the RELEASE semantics only apply to the
store portion of the operation, it therefore doesn't have any ordering
guarantees. Acquire is called out as a special case because it *does*
actually perform a load on the failure case.
If cmpxchg_release doesn't have order guarantee when failed, I guess I
can implement it with a barrier in the middle as you mentioned:
unsigned int prev;
__asm__ __volatile__ (
"1: lwarx %0,0,%2
cmpw 0,%0,%3\n\
bne- 2f\n"
PPC_RELEASE_BARRIER
" stwcx. %4,0,%2\n\
bne- 1b"
"\n\
2:"
: "=&r" (prev), "+m" (*p)
: "r" (p), "r" (old), "r" (new)
: "cc", "memory");
return prev;
However, I need to check whether the architecture allows this and any
other problem exists.
Besides, I don't think it's a good idea to do the "put barrier in the
middle" thing in this patchset, because that seems a premature
optimization and if we go further, I guess we can also replace the
PPC_RELEASE_BARRIER above with a "sync" to implement a fully ordered
version cmpxchg(). Too much needs to investigate then..
Putting a barrier in the middle of that critical section is probably a
terrible idea, and that's why I thought you were avoiding it (hence my
original question). Perhaps just add a comment to that effect, since I
fear adding more words to memory-barriers.txt is just likely to create
further confusion.
Will
On Tue, Oct 13, 2015 at 10:32:59PM +0800, Boqun Feng wrote:
On Tue, Oct 13, 2015 at 02:24:04PM +0100, Will Deacon wrote:
quoted
On Mon, Oct 12, 2015 at 10:14:06PM +0800, Boqun Feng wrote:
quoted
Implement cmpxchg{,64}_relaxed and atomic{,64}_cmpxchg_relaxed, based on
which _release variants can be built.
To avoid superfluous barriers in _acquire variants, we implement these
operations with assembly code rather use __atomic_op_acquire() to build
them automatically.
The "superfluous barriers" are for the case where the cmpxchg fails, right?
Yes.
quoted
And you don't do the same thing for release, because you want to avoid a
barrier in the middle of the critical section?
Mostly because of the comments in include/linux/atomic.h:
* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failed cmpxchg_acquire
* does -not- imply any memory ordering constraints.
so I thought only the barrier in cmpxchg_acquire() is conditional, and
the barrier in cmpxchg_release() is not. Maybe we'd better call it out
that cmpxchg *family* doesn't have any order guarantee if cmp fails, as
a complement of
ed2de9f74ecb ("locking/Documentation: Clarify failed cmpxchg() memory ordering semantics")
Because it seems this commit only claims that the barriers in fully
ordered version are conditional.
If cmpxchg_release doesn't have order guarantee when failed, I guess I
can implement it with a barrier in the middle as you mentioned:
unsigned int prev;
__asm__ __volatile__ (
"1: lwarx %0,0,%2
cmpw 0,%0,%3\n\
bne- 2f\n"
PPC_RELEASE_BARRIER
" stwcx. %4,0,%2\n\
bne- 1b"
"\n\
2:"
: "=&r" (prev), "+m" (*p)
: "r" (p), "r" (old), "r" (new)
: "cc", "memory");
return prev;
However, I need to check whether the architecture allows this and any
other problem exists.
Besides, I don't think it's a good idea to do the "put barrier in the
middle" thing in this patchset, because that seems a premature
optimization and if we go further, I guess we can also replace the
PPC_RELEASE_BARRIER above with a "sync" to implement a fully ordered
Correction: we can't just put the sync in the middle to implement a
fully ordered version. Sorry for that mistake..
Regards,
Boqun
version cmpxchg(). Too much needs to investigate then..
quoted
(just checking I understand your reasoning).
That actually helps me find a probably better implementation if allowed,
thank you ;-)
Regards,
Boqun
On Tue, Oct 13, 2015 at 03:43:33PM +0100, Will Deacon wrote:
On Tue, Oct 13, 2015 at 10:32:59PM +0800, Boqun Feng wrote:
[snip]
quoted
Mostly because of the comments in include/linux/atomic.h:
* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failed cmpxchg_acquire
* does -not- imply any memory ordering constraints.
so I thought only the barrier in cmpxchg_acquire() is conditional, and
the barrier in cmpxchg_release() is not. Maybe we'd better call it out
that cmpxchg *family* doesn't have any order guarantee if cmp fails, as
a complement of
ed2de9f74ecb ("locking/Documentation: Clarify failed cmpxchg() memory ordering semantics")
Because it seems this commit only claims that the barriers in fully
ordered version are conditional.
I didn't think this was ambiguous... A failed cmpxchg_release doesn't
perform a store, so because the RELEASE semantics only apply to the
store portion of the operation, it therefore doesn't have any ordering
guarantees. Acquire is called out as a special case because it *does*
actually perform a load on the failure case.
Make sense.
quoted
If cmpxchg_release doesn't have order guarantee when failed, I guess I
can implement it with a barrier in the middle as you mentioned:
unsigned int prev;
__asm__ __volatile__ (
"1: lwarx %0,0,%2
cmpw 0,%0,%3\n\
bne- 2f\n"
PPC_RELEASE_BARRIER
" stwcx. %4,0,%2\n\
bne- 1b"
"\n\
2:"
: "=&r" (prev), "+m" (*p)
: "r" (p), "r" (old), "r" (new)
: "cc", "memory");
return prev;
However, I need to check whether the architecture allows this and any
other problem exists.
Besides, I don't think it's a good idea to do the "put barrier in the
middle" thing in this patchset, because that seems a premature
optimization and if we go further, I guess we can also replace the
PPC_RELEASE_BARRIER above with a "sync" to implement a fully ordered
version cmpxchg(). Too much needs to investigate then..
Putting a barrier in the middle of that critical section is probably a
terrible idea, and that's why I thought you were avoiding it (hence my
The fact is that I haven't thought of that way to implement
cmpxchg_release before you ask that question ;-) And I'm not going to do
that for now and probably not in the future.
original question). Perhaps just add a comment to that effect, since I
Are you suggesting if I put a barrier in the middle I'd better to add a
comment, right? So if I don't do that, it's OK to let this patch as it.
Regards,
Boqun
fear adding more words to memory-barriers.txt is just likely to create
further confusion.
Will
From: Will Deacon <hidden> Date: 2015-10-13 15:04:30
On Tue, Oct 13, 2015 at 10:58:30PM +0800, Boqun Feng wrote:
On Tue, Oct 13, 2015 at 03:43:33PM +0100, Will Deacon wrote:
quoted
Putting a barrier in the middle of that critical section is probably a
terrible idea, and that's why I thought you were avoiding it (hence my
The fact is that I haven't thought of that way to implement
cmpxchg_release before you ask that question ;-) And I'm not going to do
that for now and probably not in the future.
quoted
original question). Perhaps just add a comment to that effect, since I
Are you suggesting if I put a barrier in the middle I'd better to add a
comment, right? So if I don't do that, it's OK to let this patch as it.
No, I mean put a comment in your file to explain the reason why you
override _relaxed and _acquire, but not _release (because overriding
_release would introduce this weird barrier in the middle of the critical
section, which would likely cause the conditional store to fail).
Will
On Tue, Oct 13, 2015 at 04:04:27PM +0100, Will Deacon wrote:
On Tue, Oct 13, 2015 at 10:58:30PM +0800, Boqun Feng wrote:
quoted
On Tue, Oct 13, 2015 at 03:43:33PM +0100, Will Deacon wrote:
quoted
Putting a barrier in the middle of that critical section is probably a
terrible idea, and that's why I thought you were avoiding it (hence my
The fact is that I haven't thought of that way to implement
cmpxchg_release before you ask that question ;-) And I'm not going to do
that for now and probably not in the future.
quoted
original question). Perhaps just add a comment to that effect, since I
Are you suggesting if I put a barrier in the middle I'd better to add a
comment, right? So if I don't do that, it's OK to let this patch as it.
No, I mean put a comment in your file to explain the reason why you
override _relaxed and _acquire, but not _release (because overriding
_release would introduce this weird barrier in the middle of the critical
section, which would likely cause the conditional store to fail).
Good idea, will do that. Thank you ;-)
Regards,
Boqun
From: Paul E. McKenney <hidden> Date: 2015-10-13 15:46:31
On Tue, Oct 13, 2015 at 02:27:13PM +0200, Peter Zijlstra wrote:
On Mon, Oct 12, 2015 at 10:14:00PM +0800, Boqun Feng wrote:
quoted
The patchset consists of 6 parts:
1. Make xchg, cmpxchg and their atomic_ versions a full barrier
2. Add trivial tests for the new variants in lib/atomic64_test.c
3. Allow architectures to define their own __atomic_op_*() helpers
to build other variants based on relaxed.
4. Implement atomic{,64}_{add,sub,inc,dec}_return_* variants
5. Implement xchg_* and atomic{,64}_xchg_* variants
6. Implement cmpxchg_* atomic{,64}_cmpxchg_* variants
This patchset is based on current locking/core branch of the tip tree
and all patches are built and boot tested for little endian pseries, and
also tested by 0day.
I don't see any immediate problems with this series at this point. Will,
Paul?
Every time I have gotten ready to take a close look, someone has pointed
out a problem, and I have deferred until the next version. Looks like
I should take a close look at Boqun's next version regardless. ;-)
Thanx, Paul
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-10-14 00:10:09
On Mon, 2015-10-12 at 22:30 +0800, Boqun Feng wrote:
According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
versions all need to imply a full barrier, however they are now just
RELEASE+ACQUIRE, which is not a full barrier.
So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
__{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
This patch is a complement of commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics").
Cc: <redacted> # 3.4.y-
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Hi Boqun,
Thanks for fixing this. In future you should send a patch like this as a
separate patch. I've not been paying attention to it because I assumed it was
part of your full series and was still under discussion like the other patches.
I don't think we've seen any crashes caused by this have we? So I guess I'll
put it in next to let it get some wider testing rather than sending it straight
to Linus.
To be clear you're doing:
- PPC_RELEASE_BARRIER
+ PPC_ATOMIC_ENTRY_BARRIER
Which is correct but doesn't actually change anything at the moment, because
both macros turn into LWSYNC.
On the other hand:
- PPC_ACQUIRE_BARRIER
+ PPC_ATOMIC_EXIT_BARRIER
Is changing an isync (which is then patched to lwsync on some cpus), with a sync.
Also I'm not clear what your stable line means:
Cc: <redacted> # 3.4.y-
Do you mean 3.4 and anything after? I usually write that as 3.4+, but I'm not
sure if that's the correct syntax either.
cheers
On Wed, Oct 14, 2015 at 11:10:00AM +1100, Michael Ellerman wrote:
On Mon, 2015-10-12 at 22:30 +0800, Boqun Feng wrote:
quoted
According to memory-barriers.txt, xchg, cmpxchg and their atomic{,64}_
versions all need to imply a full barrier, however they are now just
RELEASE+ACQUIRE, which is not a full barrier.
So replace PPC_RELEASE_BARRIER and PPC_ACQUIRE_BARRIER with
PPC_ATOMIC_ENTRY_BARRIER and PPC_ATOMIC_EXIT_BARRIER in
__{cmp,}xchg_{u32,u64} respectively to guarantee a full barrier
semantics of atomic{,64}_{cmp,}xchg() and {cmp,}xchg().
This patch is a complement of commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics").
Cc: <redacted> # 3.4.y-
Signed-off-by: Boqun Feng <redacted>
---
arch/powerpc/include/asm/cmpxchg.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
Hi Boqun,
Hello, Michael
Thanks for fixing this. In future you should send a patch like this as a
separate patch. I've not been paying attention to it because I assumed it was
Got it. However, here is the thing, in previous version, this fix
depends on some of other patches in this patchset. So to make this fix
applied cleanly, I reorder my patchset to put this patch first, and the
result is that some of other patches in this patchset depends on
this(they need to remove code modified by this patch).
So I guess I'd better to stop Cc stable for this one, and wait until
this patchset merged and send a separate patch for -stable tree. Does
that work for you? I think this is what Peter want to suggests me to do
when he asked me about this, right, Peter?
part of your full series and was still under discussion like the other patches.
I don't think we've seen any crashes caused by this have we? So I guess I'll
No, we haven't seen any.
put it in next to let it get some wider testing rather than sending it straight
to Linus.
Good idea, thank you ;-)
To be clear you're doing:
quoted
- PPC_RELEASE_BARRIER
+ PPC_ATOMIC_ENTRY_BARRIER
Which is correct but doesn't actually change anything at the moment, because
both macros turn into LWSYNC.
On the other hand:
quoted
- PPC_ACQUIRE_BARRIER
+ PPC_ATOMIC_EXIT_BARRIER
Is changing an isync (which is then patched to lwsync on some cpus), with a sync.
These macros are introduced by commit b97021f85517 ("powerpc: Fix
atomic_xxx_return barrier semantics") to fix a similar problem, so I use
them to keep code similar.
Also I'm not clear what your stable line means:
quoted
Cc: <redacted> # 3.4.y-
Do you mean 3.4 and anything after? I usually write that as 3.4+, but I'm not
sure if that's the correct syntax either.
Quote from Documentation/stable_kernel_rules.txt:
"""
Also, some patches may have kernel version prerequisites. This can be
specified in the following format in the sign-off area:
Cc: [off-list ref] # 3.3.x-
The tag has the meaning of:
git cherry-pick <this commit>
For each "-stable" tree starting with the specified version.
"""
But yes, I have seen several people use like "3.4+", I'm not sure either
Regards,
Boqun
On Tue, Oct 13, 2015 at 09:35:54PM +0800, Boqun Feng wrote:
On Tue, Oct 13, 2015 at 02:21:32PM +0100, Will Deacon wrote:
quoted
On Mon, Oct 12, 2015 at 10:14:04PM +0800, Boqun Feng wrote:
[snip]
quoted
quoted
+/*
+ * Since {add,sub}_return_relaxed and xchg_relaxed are implemented with
+ * a "bne-" instruction at the end, so an isync is enough as a acquire barrier
+ * on the platform without lwsync.
+ */
+#ifdef CONFIG_SMP
+#define smp_acquire_barrier__after_atomic() \
+ __asm__ __volatile__(PPC_ACQUIRE_BARRIER : : : "memory")
I'm not keen on this barrier, as it sounds like it's part of the kernel
memory model, as opposed to an implementation detail on PowerPC (and
we've already got enough of that in the generic code ;).
Indeed, but we still have smp_lwsync() ;-)
quoted
Can you name it something different please (and maybe #undef it when
you're done)?
I've considered #undef it after used, but now I think open code this
into __atomic_op_acquire() of PPC is a better idea?
#define __atomic_op_acquire(op, args...) \
({ \
typeof(op##_relaxed(args)) __ret = op##_relaxed(args); \
__asm__ __volatile__(PPC_ACQUIRE_BARRIER : : : "memory"); \
Should be:
__asm__ __volatile__(PPC_ACQUIRE_BARRIER "" : : : "memory");
__ret; \
})
PPC_ACQUIRE_BARRIER will be empty if !SMP, so that will become a pure
compiler barrier and just what we need.
Regards,
Boqun
On Tue, Oct 13, 2015 at 04:04:27PM +0100, Will Deacon wrote:
On Tue, Oct 13, 2015 at 10:58:30PM +0800, Boqun Feng wrote:
quoted
On Tue, Oct 13, 2015 at 03:43:33PM +0100, Will Deacon wrote:
quoted
Putting a barrier in the middle of that critical section is probably a
terrible idea, and that's why I thought you were avoiding it (hence my
The fact is that I haven't thought of that way to implement
cmpxchg_release before you ask that question ;-) And I'm not going to do
that for now and probably not in the future.
quoted
original question). Perhaps just add a comment to that effect, since I
Are you suggesting if I put a barrier in the middle I'd better to add a
comment, right? So if I don't do that, it's OK to let this patch as it.
No, I mean put a comment in your file to explain the reason why you
override _relaxed and _acquire, but not _release (because overriding
You mean overriding _acquire and fully order version, right?
_release would introduce this weird barrier in the middle of the critical
section, which would likely cause the conditional store to fail).
Will
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-10-14 08:06:30
On Wed, Oct 14, 2015 at 08:51:34AM +0800, Boqun Feng wrote:
On Wed, Oct 14, 2015 at 11:10:00AM +1100, Michael Ellerman wrote:
quoted
Thanks for fixing this. In future you should send a patch like this as a
separate patch. I've not been paying attention to it because I assumed it was
Got it. However, here is the thing, in previous version, this fix
depends on some of other patches in this patchset. So to make this fix
applied cleanly, I reorder my patchset to put this patch first, and the
result is that some of other patches in this patchset depends on
this(they need to remove code modified by this patch).
So I guess I'd better to stop Cc stable for this one, and wait until
this patchset merged and send a separate patch for -stable tree. Does
that work for you? I think this is what Peter want to suggests me to do
when he asked me about this, right, Peter?
I don't think I had explicit thoughts about any of that, just that it
might make sense to have this patch not depend on the rest such that it
could indeed be stuffed into stable.
I'll leave the details up to Michael since he's PPC maintainer.
On Wed, Oct 14, 2015 at 10:06:13AM +0200, Peter Zijlstra wrote:
On Wed, Oct 14, 2015 at 08:51:34AM +0800, Boqun Feng wrote:
quoted
On Wed, Oct 14, 2015 at 11:10:00AM +1100, Michael Ellerman wrote:
quoted
quoted
Thanks for fixing this. In future you should send a patch like this as a
separate patch. I've not been paying attention to it because I assumed it was
Got it. However, here is the thing, in previous version, this fix
depends on some of other patches in this patchset. So to make this fix
applied cleanly, I reorder my patchset to put this patch first, and the
result is that some of other patches in this patchset depends on
this(they need to remove code modified by this patch).
So I guess I'd better to stop Cc stable for this one, and wait until
this patchset merged and send a separate patch for -stable tree. Does
that work for you? I think this is what Peter want to suggests me to do
when he asked me about this, right, Peter?
I don't think I had explicit thoughts about any of that, just that it
might make sense to have this patch not depend on the rest such that it
could indeed be stuffed into stable.
Got that. Sorry for misunderstanding you...
I'll leave the details up to Michael since he's PPC maintainer.
Michael and Peter, rest of this patchset depends on commits which are
currently in the locking/core branch of the tip, so I would like it as a
whole queued there. Besides, I will keep this patch Cc'ed to stable in
future versions, that works for you both?
Regards,
Boqun
From: Peter Zijlstra <peterz@infradead.org> Date: 2015-10-14 09:33:25
On Wed, Oct 14, 2015 at 05:26:53PM +0800, Boqun Feng wrote:
Michael and Peter, rest of this patchset depends on commits which are
currently in the locking/core branch of the tip, so I would like it as a
whole queued there. Besides, I will keep this patch Cc'ed to stable in
future versions, that works for you both?
From my POV having the Cc stable in there is fine if Michael actually
wants them to go there. GregKH will vacuum them up once they hit Linus'
tree and we don't need to think about it anymore.
Alternatively, Michael could put the patch in a separate branch and we
could both merge that.
Or even, seeing how its a single patch and git mostly does the right
thing, we could just merge it independently in both trees and let git
sort it out at merge time.
From: Will Deacon <hidden> Date: 2015-10-14 09:40:32
On Wed, Oct 14, 2015 at 09:47:35AM +0800, Boqun Feng wrote:
On Tue, Oct 13, 2015 at 04:04:27PM +0100, Will Deacon wrote:
quoted
On Tue, Oct 13, 2015 at 10:58:30PM +0800, Boqun Feng wrote:
quoted
On Tue, Oct 13, 2015 at 03:43:33PM +0100, Will Deacon wrote:
quoted
Putting a barrier in the middle of that critical section is probably a
terrible idea, and that's why I thought you were avoiding it (hence my
The fact is that I haven't thought of that way to implement
cmpxchg_release before you ask that question ;-) And I'm not going to do
that for now and probably not in the future.
quoted
original question). Perhaps just add a comment to that effect, since I
Are you suggesting if I put a barrier in the middle I'd better to add a
comment, right? So if I don't do that, it's OK to let this patch as it.
No, I mean put a comment in your file to explain the reason why you
override _relaxed and _acquire, but not _release (because overriding
You mean overriding _acquire and fully order version, right?
Yes, my mistake. Sounds like you get my drift, though.
Will
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2015-10-14 09:44:05
On Wed, 2015-10-14 at 11:33 +0200, Peter Zijlstra wrote:
On Wed, Oct 14, 2015 at 05:26:53PM +0800, Boqun Feng wrote:
quoted
Michael and Peter, rest of this patchset depends on commits which are
currently in the locking/core branch of the tip, so I would like it as a
whole queued there. Besides, I will keep this patch Cc'ed to stable in
future versions, that works for you both?
From my POV having the Cc stable in there is fine if Michael actually
wants them to go there. GregKH will vacuum them up once they hit Linus'
tree and we don't need to think about it anymore.
Yeah that's fine by me. Here's an Ack if you want one:
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
Alternatively, Michael could put the patch in a separate branch and we
could both merge that.
Or even, seeing how its a single patch and git mostly does the right
thing, we could just merge it independently in both trees and let git
sort it out at merge time.
That probably would work, but I don't think it's necessary.
My tree doesn't get much (or any) more testing than linux-next, so as long as
locking/core is in linux-next then it will be tested just fine that way.
cheers