Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On 10/14/19 1:49 PM, Manfred Spraul wrote:
> Hello Peter,
>
> On 10/14/19 3:03 PM, Peter Zijlstra wrote:
>> On Sat, Oct 12, 2019 at 07:49:58AM +0200, Manfred Spraul wrote:
>>> The documentation in memory-barriers.txt claims that
>>> smp_mb__{before,after}_atomic() are for atomic ops that do not return a
>>> value.
>>>
>>> This is misleading and doesn't match the example in atomic_t.txt,
>>> and e.g. smp_mb__before_atomic() may and is used together with
>>> cmpxchg_relaxed() in the wake_q code.
>>>
>>> The purpose of e.g. smp_mb__before_atomic() is to "upgrade" a following
>>> RMW atomic operation to a full memory barrier.
>>> The return code of the atomic operation has no impact, so all of the
>>> following examples are valid:
>> The value return of atomic ops is relevant in so far that
>> (traditionally) all value returning atomic ops already implied full
>> barriers. That of course changed when we added
>> _release/_acquire/_relaxed variants.
> I've updated the Change description accordingly
>>> 1)
>>> smp_mb__before_atomic();
>>> atomic_add();
>>>
>>> 2)
>>> smp_mb__before_atomic();
>>> atomic_xchg_relaxed();
>>>
>>> 3)
>>> smp_mb__before_atomic();
>>> atomic_fetch_add_relaxed();
>>>
>>> Invalid would be:
>>> smp_mb__before_atomic();
>>> atomic_set();
>>>
>>> Signed-off-by: Manfred Spraul
>>> Cc: Waiman Long
>>> Cc: Davidlohr Bueso
>>> Cc: Peter Zijlstra
>>> ---
>>> Documentation/memory-barriers.txt | 11 ++-
>>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/memory-barriers.txt
>>> b/Documentation/memory-barriers.txt
>>> index 1adbb8a371c7..52076b057400 100644
>>> --- a/Documentation/memory-barriers.txt
>>> +++ b/Documentation/memory-barriers.txt
>>> @@ -1873,12 +1873,13 @@ There are some more advanced barrier functions:
>>> (*) smp_mb__before_atomic();
>>> (*) smp_mb__after_atomic();
>>> - These are for use with atomic (such as add, subtract,
>>> increment and
>>> - decrement) functions that don't return a value, especially
>>> when used for
>>> - reference counting. These functions do not imply memory
>>> barriers.
>>> + These are for use with atomic RMW functions (such as add,
>>> subtract,
>>> + increment, decrement, failed conditional operations, ...) that do
>>> + not imply memory barriers, but where the code needs a memory
>>> barrier,
>>> + for example when used for reference counting.
>>> - These are also used for atomic bitop functions that do not
>>> return a
>>> - value (such as set_bit and clear_bit).
>>> + These are also used for atomic RMW bitop functions that do
>>> imply a full
>> s/do/do not/ ?
> Sorry, yes, of course
I was wondering the same thing. With the revised patch,
Acked-by: Waiman Long
>>> + memory barrier (such as set_bit and clear_bit).
>
>
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On Tue, 15 Oct 2019, Peter Zijlstra wrote: On Mon, Oct 14, 2019 at 06:26:04PM -0700, Davidlohr Bueso wrote: On Sat, 12 Oct 2019, Manfred Spraul wrote: > Invalid would be: >smp_mb__before_atomic(); >atomic_set(); fyi I've caught a couple of naughty users: drivers/crypto/cavium/nitrox/nitrox_main.c drivers/gpu/drm/msm/adreno/a5xx_preempt.c Yes, there's still some of that. Andrea went and killed a buch a while ago I think. I sent these, which just does smp_mb(): https://lore.kernel.org/lkml/20191015161657.10760-1-d...@stgolabs.net https://lore.kernel.org/lkml/20191015162144.fuyc25tdwvc6ddu3@linux-p48b Similarly, I was doing some barrier auditing in btrfs code recently (completely unrelated to these topics) and noted that there are some cases where we can inverse this exercise. Iow, callers doing atomic Rmw along with smp_mb(), which we can replace with these upgradable calls and benefit, for example in x86. Thanks, Davidlohr
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On Mon, Oct 14, 2019 at 07:49:56PM +0200, Manfred Spraul wrote:
> From 61c85a56994e32ea393af9debef4cccd9cd24abd Mon Sep 17 00:00:00 2001
> From: Manfred Spraul
> Date: Fri, 11 Oct 2019 10:33:26 +0200
> Subject: [PATCH] Update Documentation for _{acquire|release|relaxed}()
>
> When adding the _{acquire|release|relaxed}() variants of some atomic
> operations, it was forgotten to update Documentation/memory_barrier.txt:
>
> smp_mb__{before,after}_atomic() is now indended for all RMW operations
"intended"
Otherwise it looks fine, thanks!
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On Mon, Oct 14, 2019 at 06:26:04PM -0700, Davidlohr Bueso wrote: > On Sat, 12 Oct 2019, Manfred Spraul wrote: > > Invalid would be: > > smp_mb__before_atomic(); > > atomic_set(); > > fyi I've caught a couple of naughty users: > > drivers/crypto/cavium/nitrox/nitrox_main.c > drivers/gpu/drm/msm/adreno/a5xx_preempt.c Yes, there's still some of that. Andrea went and killed a buch a while ago I think.
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On Sat, 12 Oct 2019, Manfred Spraul wrote: Invalid would be: smp_mb__before_atomic(); atomic_set(); fyi I've caught a couple of naughty users: drivers/crypto/cavium/nitrox/nitrox_main.c drivers/gpu/drm/msm/adreno/a5xx_preempt.c Thanks, Davidlohr
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On Mon, 14 Oct 2019, Manfred Spraul wrote: I've updated the Change description accordingly I continue to think my memory-barriers.txt change regarding failed Rmw is still good to have. Unless any strong objections, could you also add the patch to the series? Thanks, Davidlohr
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
Hello Peter,
On 10/14/19 3:03 PM, Peter Zijlstra wrote:
On Sat, Oct 12, 2019 at 07:49:58AM +0200, Manfred Spraul wrote:
The documentation in memory-barriers.txt claims that
smp_mb__{before,after}_atomic() are for atomic ops that do not return a
value.
This is misleading and doesn't match the example in atomic_t.txt,
and e.g. smp_mb__before_atomic() may and is used together with
cmpxchg_relaxed() in the wake_q code.
The purpose of e.g. smp_mb__before_atomic() is to "upgrade" a following
RMW atomic operation to a full memory barrier.
The return code of the atomic operation has no impact, so all of the
following examples are valid:
The value return of atomic ops is relevant in so far that
(traditionally) all value returning atomic ops already implied full
barriers. That of course changed when we added
_release/_acquire/_relaxed variants.
I've updated the Change description accordingly
1)
smp_mb__before_atomic();
atomic_add();
2)
smp_mb__before_atomic();
atomic_xchg_relaxed();
3)
smp_mb__before_atomic();
atomic_fetch_add_relaxed();
Invalid would be:
smp_mb__before_atomic();
atomic_set();
Signed-off-by: Manfred Spraul
Cc: Waiman Long
Cc: Davidlohr Bueso
Cc: Peter Zijlstra
---
Documentation/memory-barriers.txt | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/memory-barriers.txt
b/Documentation/memory-barriers.txt
index 1adbb8a371c7..52076b057400 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1873,12 +1873,13 @@ There are some more advanced barrier functions:
(*) smp_mb__before_atomic();
(*) smp_mb__after_atomic();
- These are for use with atomic (such as add, subtract, increment and
- decrement) functions that don't return a value, especially when used for
- reference counting. These functions do not imply memory barriers.
+ These are for use with atomic RMW functions (such as add, subtract,
+ increment, decrement, failed conditional operations, ...) that do
+ not imply memory barriers, but where the code needs a memory barrier,
+ for example when used for reference counting.
- These are also used for atomic bitop functions that do not return a
- value (such as set_bit and clear_bit).
+ These are also used for atomic RMW bitop functions that do imply a full
s/do/do not/ ?
Sorry, yes, of course
+ memory barrier (such as set_bit and clear_bit).
>From 61c85a56994e32ea393af9debef4cccd9cd24abd Mon Sep 17 00:00:00 2001
From: Manfred Spraul
Date: Fri, 11 Oct 2019 10:33:26 +0200
Subject: [PATCH] Update Documentation for _{acquire|release|relaxed}()
When adding the _{acquire|release|relaxed}() variants of some atomic
operations, it was forgotten to update Documentation/memory_barrier.txt:
smp_mb__{before,after}_atomic() is now indended for all RMW operations
that do not imply a full memory barrier.
1)
smp_mb__before_atomic();
atomic_add();
2)
smp_mb__before_atomic();
atomic_xchg_relaxed();
3)
smp_mb__before_atomic();
atomic_fetch_add_relaxed();
Invalid would be:
smp_mb__before_atomic();
atomic_set();
Fixes: 654672d4ba1a ("locking/atomics: Add _{acquire|release|relaxed}() variants of some atomic operations")
Signed-off-by: Manfred Spraul
Cc: Waiman Long
Cc: Davidlohr Bueso
Cc: Peter Zijlstra
Cc: Will Deacon
---
Documentation/memory-barriers.txt | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 1adbb8a371c7..08090eea3751 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1873,12 +1873,13 @@ There are some more advanced barrier functions:
(*) smp_mb__before_atomic();
(*) smp_mb__after_atomic();
- These are for use with atomic (such as add, subtract, increment and
- decrement) functions that don't return a value, especially when used for
- reference counting. These functions do not imply memory barriers.
+ These are for use with atomic RMW functions (such as add, subtract,
+ increment, decrement, failed conditional operations, ...) that do
+ not imply memory barriers, but where the code needs a memory barrier,
+ for example when used for reference counting.
- These are also used for atomic bitop functions that do not return a
- value (such as set_bit and clear_bit).
+ These are also used for atomic RMW bitop functions that do not imply a
+ full memory barrier (such as set_bit and clear_bit).
As an example, consider a piece of code that marks an object as being dead
and then decrements the object's reference count:
--
2.21.0
Re: [PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
On Sat, Oct 12, 2019 at 07:49:58AM +0200, Manfred Spraul wrote:
> The documentation in memory-barriers.txt claims that
> smp_mb__{before,after}_atomic() are for atomic ops that do not return a
> value.
>
> This is misleading and doesn't match the example in atomic_t.txt,
> and e.g. smp_mb__before_atomic() may and is used together with
> cmpxchg_relaxed() in the wake_q code.
>
> The purpose of e.g. smp_mb__before_atomic() is to "upgrade" a following
> RMW atomic operation to a full memory barrier.
> The return code of the atomic operation has no impact, so all of the
> following examples are valid:
The value return of atomic ops is relevant in so far that
(traditionally) all value returning atomic ops already implied full
barriers. That of course changed when we added
_release/_acquire/_relaxed variants.
>
> 1)
> smp_mb__before_atomic();
> atomic_add();
>
> 2)
> smp_mb__before_atomic();
> atomic_xchg_relaxed();
>
> 3)
> smp_mb__before_atomic();
> atomic_fetch_add_relaxed();
>
> Invalid would be:
> smp_mb__before_atomic();
> atomic_set();
>
> Signed-off-by: Manfred Spraul
> Cc: Waiman Long
> Cc: Davidlohr Bueso
> Cc: Peter Zijlstra
> ---
> Documentation/memory-barriers.txt | 11 ++-
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/memory-barriers.txt
> b/Documentation/memory-barriers.txt
> index 1adbb8a371c7..52076b057400 100644
> --- a/Documentation/memory-barriers.txt
> +++ b/Documentation/memory-barriers.txt
> @@ -1873,12 +1873,13 @@ There are some more advanced barrier functions:
> (*) smp_mb__before_atomic();
> (*) smp_mb__after_atomic();
>
> - These are for use with atomic (such as add, subtract, increment and
> - decrement) functions that don't return a value, especially when used for
> - reference counting. These functions do not imply memory barriers.
> + These are for use with atomic RMW functions (such as add, subtract,
> + increment, decrement, failed conditional operations, ...) that do
> + not imply memory barriers, but where the code needs a memory barrier,
> + for example when used for reference counting.
>
> - These are also used for atomic bitop functions that do not return a
> - value (such as set_bit and clear_bit).
> + These are also used for atomic RMW bitop functions that do imply a full
s/do/do not/ ?
> + memory barrier (such as set_bit and clear_bit).
[PATCH 6/6] Documentation/memory-barriers.txt: Clarify cmpxchg()
The documentation in memory-barriers.txt claims that
smp_mb__{before,after}_atomic() are for atomic ops that do not return a
value.
This is misleading and doesn't match the example in atomic_t.txt,
and e.g. smp_mb__before_atomic() may and is used together with
cmpxchg_relaxed() in the wake_q code.
The purpose of e.g. smp_mb__before_atomic() is to "upgrade" a following
RMW atomic operation to a full memory barrier.
The return code of the atomic operation has no impact, so all of the
following examples are valid:
1)
smp_mb__before_atomic();
atomic_add();
2)
smp_mb__before_atomic();
atomic_xchg_relaxed();
3)
smp_mb__before_atomic();
atomic_fetch_add_relaxed();
Invalid would be:
smp_mb__before_atomic();
atomic_set();
Signed-off-by: Manfred Spraul
Cc: Waiman Long
Cc: Davidlohr Bueso
Cc: Peter Zijlstra
---
Documentation/memory-barriers.txt | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/memory-barriers.txt
b/Documentation/memory-barriers.txt
index 1adbb8a371c7..52076b057400 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1873,12 +1873,13 @@ There are some more advanced barrier functions:
(*) smp_mb__before_atomic();
(*) smp_mb__after_atomic();
- These are for use with atomic (such as add, subtract, increment and
- decrement) functions that don't return a value, especially when used for
- reference counting. These functions do not imply memory barriers.
+ These are for use with atomic RMW functions (such as add, subtract,
+ increment, decrement, failed conditional operations, ...) that do
+ not imply memory barriers, but where the code needs a memory barrier,
+ for example when used for reference counting.
- These are also used for atomic bitop functions that do not return a
- value (such as set_bit and clear_bit).
+ These are also used for atomic RMW bitop functions that do imply a full
+ memory barrier (such as set_bit and clear_bit).
As an example, consider a piece of code that marks an object as being dead
and then decrements the object's reference count:
--
2.21.0
