* [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
@ 2013-12-04 22:46 Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt Paul E. McKenney
` (2 more replies)
0 siblings, 3 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-04 22:46 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw
Hello!
This series applies some long-needed updates to memory-barriers.txt:
1. Add ACCESS_ONCE() calls where needed to ensure their inclusion
in code copy-and-pasted from this file.
2. Add long atomic examples alongside the existing atomics.
3. Prohibit architectures supporting the Linux kernel from
speculating stores.
4. Document what ACCESS_ONCE() does along with a number of situations
requiring its use.
Changes from v3:
o Fix typos noted by Peter Zijlstra.
o Added the documentation about ACCESS_ONCE(), which expands on
http://thread.gmane.org/gmane.linux.kernel.mm/82891/focus=14696,
ably summarized by Jon Corbet at http://lwn.net/Articles/508991/.
Changes from v2:
o Update examples so that that load against which the subsequent
store is to be ordered is part of the "if" condition.
o Add an example showing how the compiler can remove "if"
conditions and how to prevent it from doing so.
o Add ACCESS_ONCE() to the compiler-barrier section.
o Add a sentence noting that transitivity requires smp_mb().
Changes from v1:
o Combined with Peter Zijlstra's speculative-store-prohibition patch.
o Added more pitfalls to avoid when prohibiting speculative
stores, along with how to avoid them.
o Applied Josh Triplett's review comments.
Thanx, Paul
b/Documentation/memory-barriers.txt | 666 ++++++++++++++++++++++++++++--------
1 file changed, 533 insertions(+), 133 deletions(-)
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt
2013-12-04 22:46 [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Paul E. McKenney
@ 2013-12-04 22:46 ` Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 2/4] Documentation/memory-barriers.txt: Add long atomic examples " Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() Paul E. McKenney
2013-12-05 0:10 ` [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Josh Triplett
2013-12-05 10:59 ` Henrik Austad
2 siblings, 2 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-04 22:46 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
Paul E. McKenney
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
The Documentation/memory-barriers.txt file was written before the need
for ACCESS_ONCE() was fully appreciated. It therefore contains no
ACCESS_ONCE() calls, which can be a problem when people lift examples
from it. This commit therefore adds ACCESS_ONCE() calls.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
---
Documentation/memory-barriers.txt | 206 +++++++++++++++++++++++---------------
1 file changed, 126 insertions(+), 80 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 020cccdbdd0c..1d067235b0bc 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -194,18 +194,22 @@ There are some minimal guarantees that may be expected of a CPU:
(*) On any given CPU, dependent memory accesses will be issued in order, with
respect to itself. This means that for:
- Q = P; D = *Q;
+ ACCESS_ONCE(Q) = P; smp_read_barrier_depends(); D = ACCESS_ONCE(*Q);
the CPU will issue the following memory operations:
Q = LOAD P, D = LOAD *Q
- and always in that order.
+ and always in that order. On most systems, smp_read_barrier_depends()
+ does nothing, but it is required for DEC Alpha. The ACCESS_ONCE()
+ is required to prevent compiler mischief. Please note that you
+ should normally use something like rcu_dereference() instead of
+ open-coding smp_read_barrier_depends().
(*) Overlapping loads and stores within a particular CPU will appear to be
ordered within that CPU. This means that for:
- a = *X; *X = b;
+ a = ACCESS_ONCE(*X); ACCESS_ONCE(*X) = b;
the CPU will only issue the following sequence of memory operations:
@@ -213,7 +217,7 @@ There are some minimal guarantees that may be expected of a CPU:
And for:
- *X = c; d = *X;
+ ACCESS_ONCE(*X) = c; d = ACCESS_ONCE(*X);
the CPU will only issue:
@@ -224,6 +228,41 @@ There are some minimal guarantees that may be expected of a CPU:
And there are a number of things that _must_ or _must_not_ be assumed:
+ (*) It _must_not_ be assumed that the compiler will do what you want with
+ memory references that are not protected by ACCESS_ONCE(). Without
+ ACCESS_ONCE(), the compiler is within its rights to do all sorts
+ of "creative" transformations:
+
+ (-) Repeat the load, possibly getting a different value on the second
+ and subsequent loads. This is especially prone to happen when
+ register pressure is high.
+
+ (-) Merge adjacent loads and stores to the same location. The most
+ familiar example is the transformation from:
+
+ while (a)
+ do_something();
+
+ to something like:
+
+ if (a)
+ for (;;)
+ do_something();
+
+ Using ACCESS_ONCE() as follows prevents this sort of optimization:
+
+ while (ACCESS_ONCE(a))
+ do_something();
+
+ (-) "Store tearing", where a single store in the source code is split
+ into smaller stores in the object code. Note that gcc really
+ will do this on some architectures when storing certain constants.
+ It can be cheaper to do a series of immediate stores than to
+ form the constant in a register and then to store that register.
+
+ (-) "Load tearing", which splits loads in a manner analogous to
+ store tearing.
+
(*) It _must_not_ be assumed that independent loads and stores will be issued
in the order given. This means that for:
@@ -450,14 +489,14 @@ The usage requirements of data dependency barriers are a little subtle, and
it's not always obvious that they're needed. To illustrate, consider the
following sequence of events:
- CPU 1 CPU 2
- =============== ===============
+ CPU 1 CPU 2
+ =============== ===============
{ A == 1, B == 2, C = 3, P == &A, Q == &C }
B = 4;
<write barrier>
- P = &B
- Q = P;
- D = *Q;
+ ACCESS_ONCE(P) = &B
+ Q = ACCESS_ONCE(P);
+ D = *Q;
There's a clear data dependency here, and it would seem that by the end of the
sequence, Q must be either &A or &B, and that:
@@ -477,15 +516,15 @@ Alpha).
To deal with this, a data dependency barrier or better must be inserted
between the address load and the data load:
- CPU 1 CPU 2
- =============== ===============
+ CPU 1 CPU 2
+ =============== ===============
{ A == 1, B == 2, C = 3, P == &A, Q == &C }
B = 4;
<write barrier>
- P = &B
- Q = P;
- <data dependency barrier>
- D = *Q;
+ ACCESS_ONCE(P) = &B
+ Q = ACCESS_ONCE(P);
+ <data dependency barrier>
+ D = *Q;
This enforces the occurrence of one of the two implications, and prevents the
third possibility from arising.
@@ -504,21 +543,22 @@ Another example of where data dependency barriers might be required is where a
number is read from memory and then used to calculate the index for an array
access:
- CPU 1 CPU 2
- =============== ===============
+ CPU 1 CPU 2
+ =============== ===============
{ M[0] == 1, M[1] == 2, M[3] = 3, P == 0, Q == 3 }
M[1] = 4;
<write barrier>
- P = 1
- Q = P;
- <data dependency barrier>
- D = M[Q];
+ ACCESS_ONCE(P) = 1
+ Q = ACCESS_ONCE(P);
+ <data dependency barrier>
+ D = M[Q];
-The data dependency barrier is very important to the RCU system, for example.
-See rcu_dereference() in include/linux/rcupdate.h. This permits the current
-target of an RCU'd pointer to be replaced with a new modified target, without
-the replacement target appearing to be incompletely initialised.
+The data dependency barrier is very important to the RCU system,
+for example. See rcu_assign_pointer() and rcu_dereference() in
+include/linux/rcupdate.h. This permits the current target of an RCU'd
+pointer to be replaced with a new modified target, without the replacement
+target appearing to be incompletely initialised.
See also the subsection on "Cache Coherency" for a more thorough example.
@@ -530,22 +570,23 @@ A control dependency requires a full read memory barrier, not simply a data
dependency barrier to make it work correctly. Consider the following bit of
code:
- q = &a;
+ q = ACCESS_ONCE(a);
if (p) {
<data dependency barrier>
- q = &b;
+ q = ACCESS_ONCE(b);
}
x = *q;
This will not have the desired effect because there is no actual data
-dependency, but rather a control dependency that the CPU may short-circuit by
-attempting to predict the outcome in advance. In such a case what's actually
-required is:
+dependency, but rather a control dependency that the CPU may short-circuit
+by attempting to predict the outcome in advance, so that other CPUs see
+the load from b as having happened before the load from a. In such a
+case what's actually required is:
- q = &a;
+ q = ACCESS_ONCE(a);
if (p) {
<read barrier>
- q = &b;
+ q = ACCESS_ONCE(b);
}
x = *q;
@@ -561,23 +602,23 @@ barrier, though a general barrier would also be viable. Similarly a read
barrier or a data dependency barrier should always be paired with at least an
write barrier, though, again, a general barrier is viable:
- CPU 1 CPU 2
- =============== ===============
- a = 1;
+ CPU 1 CPU 2
+ =============== ===============
+ ACCESS_ONCE(a) = 1;
<write barrier>
- b = 2; x = b;
- <read barrier>
- y = a;
+ ACCESS_ONCE(b) = 2; x = ACCESS_ONCE(b);
+ <read barrier>
+ y = ACCESS_ONCE(a);
Or:
- CPU 1 CPU 2
- =============== ===============================
+ CPU 1 CPU 2
+ =============== ===============================
a = 1;
<write barrier>
- b = &a; x = b;
- <data dependency barrier>
- y = *x;
+ ACCESS_ONCE(b) = &a; x = ACCESS_ONCE(b);
+ <data dependency barrier>
+ y = *x;
Basically, the read barrier always has to be there, even though it can be of
the "weaker" type.
@@ -586,13 +627,13 @@ the "weaker" type.
match the loads after the read barrier or the data dependency barrier, and vice
versa:
- CPU 1 CPU 2
- =============== ===============
- a = 1; }---- --->{ v = c
- b = 2; } \ / { w = d
- <write barrier> \ <read barrier>
- c = 3; } / \ { x = a;
- d = 4; }---- --->{ y = b;
+ CPU 1 CPU 2
+ =================== ===================
+ ACCESS_ONCE(a) = 1; }---- --->{ v = ACCESS_ONCE(c);
+ ACCESS_ONCE(b) = 2; } \ / { w = ACCESS_ONCE(d);
+ <write barrier> \ <read barrier>
+ ACCESS_ONCE(c) = 3; } / \ { x = ACCESS_ONCE(a);
+ ACCESS_ONCE(d) = 4; }---- --->{ y = ACCESS_ONCE(b);
EXAMPLES OF MEMORY BARRIER SEQUENCES
@@ -1435,12 +1476,12 @@ three CPUs; then should the following sequence of events occur:
CPU 1 CPU 2
=============================== ===============================
- *A = a; *E = e;
+ ACCESS_ONCE(*A) = a; ACCESS_ONCE(*E) = e;
LOCK M LOCK Q
- *B = b; *F = f;
- *C = c; *G = g;
+ ACCESS_ONCE(*B) = b; ACCESS_ONCE(*F) = f;
+ ACCESS_ONCE(*C) = c; ACCESS_ONCE(*G) = g;
UNLOCK M UNLOCK Q
- *D = d; *H = h;
+ ACCESS_ONCE(*D) = d; ACCESS_ONCE(*H) = h;
Then there is no guarantee as to what order CPU 3 will see the accesses to *A
through *H occur in, other than the constraints imposed by the separate locks
@@ -1460,17 +1501,17 @@ However, if the following occurs:
CPU 1 CPU 2
=============================== ===============================
- *A = a;
- LOCK M [1]
- *B = b;
- *C = c;
- UNLOCK M [1]
- *D = d; *E = e;
- LOCK M [2]
- *F = f;
- *G = g;
- UNLOCK M [2]
- *H = h;
+ ACCESS_ONCE(*A) = a;
+ LOCK M [1]
+ ACCESS_ONCE(*B) = b;
+ ACCESS_ONCE(*C) = c;
+ UNLOCK M [1]
+ ACCESS_ONCE(*D) = d; ACCESS_ONCE(*E) = e;
+ LOCK M [2]
+ ACCESS_ONCE(*F) = f;
+ ACCESS_ONCE(*G) = g;
+ UNLOCK M [2]
+ ACCESS_ONCE(*H) = h;
CPU 3 might see:
@@ -2177,11 +2218,11 @@ A programmer might take it for granted that the CPU will perform memory
operations in exactly the order specified, so that if the CPU is, for example,
given the following piece of code to execute:
- a = *A;
- *B = b;
- c = *C;
- d = *D;
- *E = e;
+ a = ACCESS_ONCE(*A);
+ ACCESS_ONCE(*B) = b;
+ c = ACCESS_ONCE(*C);
+ d = ACCESS_ONCE(*D);
+ ACCESS_ONCE(*E) = e;
they would then expect that the CPU will complete the memory operation for each
instruction before moving on to the next one, leading to a definite sequence of
@@ -2228,12 +2269,12 @@ However, it is guaranteed that a CPU will be self-consistent: it will see its
_own_ accesses appear to be correctly ordered, without the need for a memory
barrier. For instance with the following code:
- U = *A;
- *A = V;
- *A = W;
- X = *A;
- *A = Y;
- Z = *A;
+ U = ACCESS_ONCE(*A);
+ ACCESS_ONCE(*A) = V;
+ ACCESS_ONCE(*A) = W;
+ X = ACCESS_ONCE(*A);
+ ACCESS_ONCE(*A) = Y;
+ Z = ACCESS_ONCE(*A);
and assuming no intervention by an external influence, it can be assumed that
the final result will appear to be:
@@ -2250,7 +2291,12 @@ accesses:
in that order, but, without intervention, the sequence may have almost any
combination of elements combined or discarded, provided the program's view of
-the world remains consistent.
+the world remains consistent. Note that ACCESS_ONCE() is -not- optional
+in the above example, as there are architectures where a given CPU might
+interchange successive loads to the same location. On such architectures,
+ACCESS_ONCE() does whatever is necessary to prevent this, for example, on
+Itanium the volatile casts used by ACCESS_ONCE() cause GCC to emit the
+special ld.acq and st.rel instructions that prevent such reordering.
The compiler may also combine, discard or defer elements of the sequence before
the CPU even sees them.
@@ -2264,13 +2310,13 @@ may be reduced to:
*A = W;
-since, without a write barrier, it can be assumed that the effect of the
-storage of V to *A is lost. Similarly:
+since, without either a write barrier or an ACCESS_ONCE(), it can be
+assumed that the effect of the storage of V to *A is lost. Similarly:
*A = Y;
Z = *A;
-may, without a memory barrier, be reduced to:
+may, without a memory barrier or an ACCESS_ONCE(), be reduced to:
*A = Y;
Z = Y;
--
1.8.1.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH tip/core/locking 2/4] Documentation/memory-barriers.txt: Add long atomic examples to memory-barriers.txt
2013-12-04 22:46 ` [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt Paul E. McKenney
@ 2013-12-04 22:46 ` Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() Paul E. McKenney
1 sibling, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-04 22:46 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
Paul E. McKenney
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Although the atomic_long_t functions are quite useful, they are a bit
obscure. This commit therefore adds the common ones alongside their
atomic_t counterparts in Documentation/memory-barriers.txt.
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: David Howells <dhowells@redhat.com>
---
Documentation/memory-barriers.txt | 24 +++++++++++++-----------
1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 1d067235b0bc..2d22da095a60 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -1728,21 +1728,23 @@ explicit lock operations, described later). These include:
xchg();
cmpxchg();
- atomic_xchg();
- atomic_cmpxchg();
- atomic_inc_return();
- atomic_dec_return();
- atomic_add_return();
- atomic_sub_return();
- atomic_inc_and_test();
- atomic_dec_and_test();
- atomic_sub_and_test();
- atomic_add_negative();
- atomic_add_unless(); /* when succeeds (returns 1) */
+ atomic_xchg(); atomic_long_xchg();
+ atomic_cmpxchg(); atomic_long_cmpxchg();
+ atomic_inc_return(); atomic_long_inc_return();
+ atomic_dec_return(); atomic_long_dec_return();
+ atomic_add_return(); atomic_long_add_return();
+ atomic_sub_return(); atomic_long_sub_return();
+ atomic_inc_and_test(); atomic_long_inc_and_test();
+ atomic_dec_and_test(); atomic_long_dec_and_test();
+ atomic_sub_and_test(); atomic_long_sub_and_test();
+ atomic_add_negative(); atomic_long_add_negative();
test_and_set_bit();
test_and_clear_bit();
test_and_change_bit();
+ /* when succeeds (returns 1) */
+ atomic_add_unless(); atomic_long_add_unless();
+
These are used for such things as implementing LOCK-class and UNLOCK-class
operations and adjusting reference counters towards object destruction, and as
such the implicit memory barrier effects are necessary.
--
1.8.1.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-04 22:46 ` [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 2/4] Documentation/memory-barriers.txt: Add long atomic examples " Paul E. McKenney
@ 2013-12-04 22:46 ` Paul E. McKenney
2013-12-05 9:33 ` Ingo Molnar
` (2 more replies)
1 sibling, 3 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-04 22:46 UTC (permalink / raw)
To: linux-kernel
Cc: mingo, laijs, dipankar, akpm, mathieu.desnoyers, josh, niv, tglx,
peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
Paul E. McKenney, Oleg Nesterov, Jonathan Corbet, Rusty Russell
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
The situations in which ACCESS_ONCE() is required are not well documented,
so this commit adds some verbiage to memory-barriers.txt.
Reported-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
Documentation/memory-barriers.txt | 253 +++++++++++++++++++++++++++++++++-----
1 file changed, 219 insertions(+), 34 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 3e4429ef1458..886d799e29cb 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -231,37 +231,8 @@ And there are a number of things that _must_ or _must_not_ be assumed:
(*) It _must_not_ be assumed that the compiler will do what you want with
memory references that are not protected by ACCESS_ONCE(). Without
ACCESS_ONCE(), the compiler is within its rights to do all sorts
- of "creative" transformations:
-
- (-) Repeat the load, possibly getting a different value on the second
- and subsequent loads. This is especially prone to happen when
- register pressure is high.
-
- (-) Merge adjacent loads and stores to the same location. The most
- familiar example is the transformation from:
-
- while (a)
- do_something();
-
- to something like:
-
- if (a)
- for (;;)
- do_something();
-
- Using ACCESS_ONCE() as follows prevents this sort of optimization:
-
- while (ACCESS_ONCE(a))
- do_something();
-
- (-) "Store tearing", where a single store in the source code is split
- into smaller stores in the object code. Note that gcc really
- will do this on some architectures when storing certain constants.
- It can be cheaper to do a series of immediate stores than to
- form the constant in a register and then to store that register.
-
- (-) "Load tearing", which splits loads in a manner analogous to
- store tearing.
+ of "creative" transformations, which are covered in the Compiler
+ Barrier section.
(*) It _must_not_ be assumed that independent loads and stores will be issued
in the order given. This means that for:
@@ -749,7 +720,8 @@ In summary:
(*) Control dependencies require that the compiler avoid reordering the
dependency into nonexistence. Careful use of ACCESS_ONCE() or
- barrier() can help to preserve your control dependency.
+ barrier() can help to preserve your control dependency. Please
+ see the Compiler Barrier section for more information.
(*) Control dependencies do -not- provide transitivity. If you
need transitivity, use smp_mb().
@@ -1252,8 +1224,221 @@ of barrier(). Howevever, ACCESS_ONCE() can be thought of as a weak form
for barrier() that affects only the specific accesses flagged by the
ACCESS_ONCE().
-The compiler barrier has no direct effect on the CPU, which may then reorder
-things however it wishes.
+The barrier() function has the following effects:
+
+ (*) Prevents the compiler from reordering accesses following the
+ barrier() to precede any accesses preceding the barrier().
+ One example use for this property is to ease communication between
+ interrupt-handler code and the code that was interrupted.
+
+ (*) Within a loop, forces the compiler to load the variables used
+ in that loop's conditional on each pass through that loop.
+
+The ACCESS_ONCE() function can prevent any number of optimizations that,
+while perfectly safe in single-threaded code, can be fatal in concurrent
+code. Here are some examples of these sorts of optimizations:
+
+ (*) The compiler is within its rights to merge successive loads from
+ the same variable. Such merging can cause the compiler to "optimize"
+ the following code:
+
+ while (tmp = a)
+ do_something_with(tmp);
+
+ into the following code, which, although in some sense legitimate
+ for single-threaded code, is almost certainly not what the developer
+ intended:
+
+ if (tmp = a)
+ for (;;)
+ do_something_with(tmp);
+
+ Use ACCESS_ONCE() to prevent the compiler from doing this to you:
+
+ while (tmp = ACCESS_ONCE(a))
+ do_something_with(tmp);
+
+ (*) The compiler is within its rights to reload a variable, for example,
+ in cases where high register pressure prevents the compiler from
+ keeping all data of interest in registers. The compiler might
+ therefore optimize the variable tmp out of our previous example:
+
+ while (tmp = a)
+ do_something_with(tmp);
+
+ This could result in the following code, which is perfectly safe in
+ single-threaded code, but can be fatal in concurrent code:
+
+ while (a)
+ do_something_with(a);
+
+ For example, the optimized version of this code could result in
+ passing a zero to do_something_with() in the case where the variable
+ a was modified by some other CPU between the "while" statement and
+ the call to do_something_with().
+
+ Again, use ACCESS_ONCE() to prevent the compiler from doing this:
+
+ while (tmp = ACCESS_ONCE(a))
+ do_something_with(tmp);
+
+ Note that if the compiler runs short of registers, it might save
+ tmp onto the stack. The overhead of this saving and later restoring
+ is why compilers reload variables. Doing so is perfectly safe for
+ single-threaded code, so you need to tell the compiler about cases
+ where it is not safe.
+
+ (*) The compiler is within its rights to omit a load entirely if it knows
+ what the value will be. For example, if the compiler can prove that
+ the value of variable a is always zero, it can optimize this code:
+
+ while (tmp = a)
+ do_something_with(tmp);
+
+ Into this:
+
+ do { } while (0);
+
+ This transformation is a win for single-threaded code because it gets
+ rid of a load and a branch. The problem is that the compiler will
+ carry out its proof assuming that the current CPU is the only one
+ updating variable a. If variable a is shared, then the compiler's
+ proof will be erroneous. Use ACCESS_ONCE() to tell the compiler
+ that it doesn't know as much as it thinks it does:
+
+ while (tmp = ACCESS_ONCE(a))
+ do_something_with(tmp);
+
+ But please note that the compiler is also closely watching what you
+ do with the value after the ACCESS_ONCE(). For example, suppose you
+ do the following and MAX is a preprocessor macro with the value 1:
+
+ for ((tmp = ACCESS_ONCE(a)) % MAX)
+ do_something_with(tmp);
+
+ Then the compiler knows that the result of the "%" operator applied
+ to MAX will always be zero, again allowing the compiler to optimize
+ the code into near-nonexistence. (It will still load from the
+ variable a.)
+
+ (*) Similarly, the compiler is within its rights to omit a store entirely
+ if it knows that the variable already has the value being stored.
+ Again, the compiler assumes that the current CPU is the only one
+ storing into the variable, which can cause the compiler to do the
+ wrong thing for shared variables. For example, suppose you have
+ the following:
+
+ a = 0;
+ /* Code that does not store to variable a. */
+ a = 0;
+
+ The compiler sees that the value of variable a is already zero, so
+ it might well omit the second store. This would come as a fatal
+ surprise if some other CPU might have stored to variable a in the
+ meantime.
+
+ Use ACCESS_ONCE() to prevent the compiler from making this sort of
+ wrong guess:
+
+ ACCESS_ONCE(a) = 0;
+ /* Code that does not store to variable a. */
+ ACCESS_ONCE(a) = 0;
+
+ (*) The compiler is within its rights to reorder memory accesses unless
+ you tell it not to. For example, consider the following interaction
+ between process-level code and an interrupt handler:
+
+ void process_level(void)
+ {
+ msg = get_message();
+ flag = true;
+ }
+
+ void interrupt_handler(void)
+ {
+ if (flag)
+ process_message(msg);
+ }
+
+ There is nothing to prevent the the compiler from transforming
+ process_level() to the following, in fact, this might well be a
+ win for single-threaded code:
+
+ void process_level(void)
+ {
+ flag = true;
+ msg = get_message();
+ }
+
+ If the interrupt occurs between these two statement, then
+ interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
+ to prevent this as follows:
+
+ void process_level(void)
+ {
+ ACCESS_ONCE(msg) = get_message();
+ ACCESS_ONCE(flag) = true;
+ }
+
+ void interrupt_handler(void)
+ {
+ if (ACCESS_ONCE(flag))
+ process_message(ACCESS_ONCE(msg));
+ }
+
+ (*) The compiler is within its rights to invent stores to a variable,
+ as in the following example:
+
+ if (a)
+ b = a;
+ else
+ b = 42;
+
+ The compiler might save a branch by optimizing this as follows:
+
+ b = 42;
+ if (a)
+ b = a;
+
+ In single-threaded code, this is not only safe, but also saves
+ a branch. Unfortunately, in concurrent code, this optimization
+ could cause some other CPU to see a spurious value of 42 -- even
+ if variable a was never zero -- when loading variable b.
+ Use ACCESS_ONCE() to prevent this as follows:
+
+ if (a)
+ ACCESS_ONCE(b) = a;
+ else
+ ACCESS_ONCE(b) = 42;
+
+ The compiler can also invent loads. These are usually less
+ damaging, but they can result in cache-line bouncing and thus in
+ poor performance and scalability. Use ACCESS_ONCE() to prevent
+ invented loads.
+
+ (*) For aligned memory locations whose size allows them to be accessed
+ with a single memory-reference instruction, prevents "load tearing"
+ and "store tearing," in which a single large access is replaced by
+ multiple smaller accesses. For example, given an architecture having
+ 16-bit store instructions with 7-bit immediate fields, the compiler
+ might be tempted to use two 16-bit store-immediate instructions to
+ implement the following 32-bit store:
+
+ p = 0x00010002;
+
+ Please note that GCC really does use this sort of optimization,
+ which is not surprising given that it would likely take more
+ than two instructions to build the constant and then store it.
+ This optimization can therefore be a win in single-threaded code.
+ In fact, a recent bug (since fixed) caused GCC to incorrectly use
+ this optimization in a volatile store. In the absence of such bugs,
+ use of ACCESS_ONCE() prevents store tearing:
+
+ ACCESS_ONCE(p) = 0x00010002;
+
+
+Please note that these compiler barriers have no direct effect on the CPU,
+which may then reorder things however it wishes.
CPU MEMORY BARRIERS
--
1.8.1.5
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-04 22:46 [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt Paul E. McKenney
@ 2013-12-05 0:10 ` Josh Triplett
2013-12-05 10:59 ` Henrik Austad
2 siblings, 0 replies; 33+ messages in thread
From: Josh Triplett @ 2013-12-05 0:10 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw
On Wed, Dec 04, 2013 at 02:46:28PM -0800, Paul E. McKenney wrote:
> Hello!
>
> This series applies some long-needed updates to memory-barriers.txt:
>
> 1. Add ACCESS_ONCE() calls where needed to ensure their inclusion
> in code copy-and-pasted from this file.
>
> 2. Add long atomic examples alongside the existing atomics.
>
> 3. Prohibit architectures supporting the Linux kernel from
> speculating stores.
>
> 4. Document what ACCESS_ONCE() does along with a number of situations
> requiring its use.
For all four patches in v4:
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-04 22:46 ` [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() Paul E. McKenney
@ 2013-12-05 9:33 ` Ingo Molnar
2013-12-05 9:52 ` Mathieu Desnoyers
2013-12-05 18:02 ` Paul E. McKenney
2013-12-05 9:50 ` Ingo Molnar
2013-12-05 20:21 ` Jonathan Corbet
2 siblings, 2 replies; 33+ messages in thread
From: Ingo Molnar @ 2013-12-05 9:33 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> + (*) The compiler is within its rights to reorder memory accesses unless
> + you tell it not to. For example, consider the following interaction
> + between process-level code and an interrupt handler:
> +
> + void process_level(void)
> + {
> + msg = get_message();
> + flag = true;
> + }
> +
> + void interrupt_handler(void)
> + {
> + if (flag)
> + process_message(msg);
> + }
> +
> + There is nothing to prevent the the compiler from transforming
> + process_level() to the following, in fact, this might well be a
> + win for single-threaded code:
> +
> + void process_level(void)
> + {
> + flag = true;
> + msg = get_message();
> + }
> +
> + If the interrupt occurs between these two statement, then
> + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> + to prevent this as follows:
> +
> + void process_level(void)
> + {
> + ACCESS_ONCE(msg) = get_message();
> + ACCESS_ONCE(flag) = true;
> + }
> +
> + void interrupt_handler(void)
> + {
> + if (ACCESS_ONCE(flag))
> + process_message(ACCESS_ONCE(msg));
> + }
Technically, if the interrupt handler is the innermost context, the
ACCESS_ONCE() is not needed in the interrupt_handler() code.
Since for the vast majority of Linux code IRQ handlers are the most
atomic contexts (very few drivers deal with NMIs) I suspect we should
either remove that ACCESS_ONCE() from the example or add a comment
explaining that in many cases those are superfluous?
> + (*) For aligned memory locations whose size allows them to be accessed
> + with a single memory-reference instruction, prevents "load tearing"
> + and "store tearing," in which a single large access is replaced by
> + multiple smaller accesses. For example, given an architecture having
> + 16-bit store instructions with 7-bit immediate fields, the compiler
> + might be tempted to use two 16-bit store-immediate instructions to
> + implement the following 32-bit store:
> +
> + p = 0x00010002;
> +
> + Please note that GCC really does use this sort of optimization,
> + which is not surprising given that it would likely take more
> + than two instructions to build the constant and then store it.
> + This optimization can therefore be a win in single-threaded code.
> + In fact, a recent bug (since fixed) caused GCC to incorrectly use
> + this optimization in a volatile store. In the absence of such bugs,
> + use of ACCESS_ONCE() prevents store tearing:
> +
> + ACCESS_ONCE(p) = 0x00010002;
I suspect the last sentence should read:
> + In the absence of such bugs,
> + use of ACCESS_ONCE() prevents store tearing in this example:
> +
> + ACCESS_ONCE(p) = 0x00010002;
Otherwise it could be read as a more generic statement (leaving out
'load tearing')?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-04 22:46 ` [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() Paul E. McKenney
2013-12-05 9:33 ` Ingo Molnar
@ 2013-12-05 9:50 ` Ingo Molnar
2013-12-05 18:05 ` Paul E. McKenney
2013-12-05 20:21 ` Jonathan Corbet
2 siblings, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-05 9:50 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> + (*) The compiler is within its rights to reload a variable, for example,
> + in cases where high register pressure prevents the compiler from
> + keeping all data of interest in registers. The compiler might
> + therefore optimize the variable tmp out of our previous example:
> +
> + while (tmp = a)
> + do_something_with(tmp);
> +
> + This could result in the following code, which is perfectly safe in
> + single-threaded code, but can be fatal in concurrent code:
> +
> + while (a)
> + do_something_with(a);
> +
> + For example, the optimized version of this code could result in
> + passing a zero to do_something_with() in the case where the variable
> + a was modified by some other CPU between the "while" statement and
> + the call to do_something_with().
Nit: I guess references to variable names such as 'a' should be quoted
(same for 'tmp', 'b', etc):
For example, the optimized version of this code could result in
passing a zero to do_something_with() in the case where the variable
'a' was modified by some other CPU between the "while" statement and
the call to do_something_with().
which makes reading it less ambiguous and more fluid IMO. This
observation applies to the whole document as 'a' is used in many
places.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 9:33 ` Ingo Molnar
@ 2013-12-05 9:52 ` Mathieu Desnoyers
2013-12-05 10:11 ` Ingo Molnar
2013-12-05 18:02 ` Paul E. McKenney
1 sibling, 1 reply; 33+ messages in thread
From: Mathieu Desnoyers @ 2013-12-05 9:52 UTC (permalink / raw)
To: Ingo Molnar
Cc: Paul E. McKenney, linux-kernel, laijs, dipankar, akpm, josh, niv,
tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
Oleg Nesterov, Jonathan Corbet, Rusty Russell
----- Original Message -----
> From: "Ingo Molnar" <mingo@kernel.org>
> To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Cc: linux-kernel@vger.kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, "mathieu
> desnoyers" <mathieu.desnoyers@efficios.com>, josh@joshtriplett.org, niv@us.ibm.com, tglx@linutronix.de,
> peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, darren@dvhart.com,
> fweisbec@gmail.com, sbw@mit.edu, "Oleg Nesterov" <oleg@redhat.com>, "Jonathan Corbet" <corbet@lwn.net>, "Rusty
> Russell" <rusty@rustcorp.com.au>
> Sent: Thursday, December 5, 2013 10:33:34 AM
> Subject: Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
>
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > + (*) The compiler is within its rights to reorder memory accesses unless
> > + you tell it not to. For example, consider the following interaction
> > + between process-level code and an interrupt handler:
> > +
> > + void process_level(void)
> > + {
> > + msg = get_message();
> > + flag = true;
> > + }
> > +
> > + void interrupt_handler(void)
> > + {
> > + if (flag)
> > + process_message(msg);
> > + }
> > +
> > + There is nothing to prevent the the compiler from transforming
> > + process_level() to the following, in fact, this might well be a
> > + win for single-threaded code:
> > +
> > + void process_level(void)
> > + {
> > + flag = true;
> > + msg = get_message();
> > + }
> > +
> > + If the interrupt occurs between these two statement, then
> > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > + to prevent this as follows:
> > +
> > + void process_level(void)
> > + {
> > + ACCESS_ONCE(msg) = get_message();
> > + ACCESS_ONCE(flag) = true;
> > + }
> > +
> > + void interrupt_handler(void)
> > + {
> > + if (ACCESS_ONCE(flag))
> > + process_message(ACCESS_ONCE(msg));
> > + }
>
> Technically, if the interrupt handler is the innermost context, the
> ACCESS_ONCE() is not needed in the interrupt_handler() code.
>
> Since for the vast majority of Linux code IRQ handlers are the most
> atomic contexts (very few drivers deal with NMIs) I suspect we should
> either remove that ACCESS_ONCE() from the example or add a comment
> explaining that in many cases those are superfluous?
It might be worthwhile to state clearly those two different use-cases:
* no nesting (e.g. process vs process), where both sides of the access
need ACCESS_ONCE().
* nested access: the outer context needs ACCESS_ONCE(), but not the
inner context.
We don't actually care about IRQs being the "most atomic" context, and
we should not care about NMIs in this example. Only the relative nesting
of the execution contexts accessing the data matter.
Thanks,
Mathieu
>
> > + (*) For aligned memory locations whose size allows them to be accessed
> > + with a single memory-reference instruction, prevents "load tearing"
> > + and "store tearing," in which a single large access is replaced by
> > + multiple smaller accesses. For example, given an architecture having
> > + 16-bit store instructions with 7-bit immediate fields, the compiler
> > + might be tempted to use two 16-bit store-immediate instructions to
> > + implement the following 32-bit store:
> > +
> > + p = 0x00010002;
> > +
> > + Please note that GCC really does use this sort of optimization,
> > + which is not surprising given that it would likely take more
> > + than two instructions to build the constant and then store it.
> > + This optimization can therefore be a win in single-threaded code.
> > + In fact, a recent bug (since fixed) caused GCC to incorrectly use
> > + this optimization in a volatile store. In the absence of such bugs,
> > + use of ACCESS_ONCE() prevents store tearing:
> > +
> > + ACCESS_ONCE(p) = 0x00010002;
>
> I suspect the last sentence should read:
>
> > + In the absence of such bugs,
> > + use of ACCESS_ONCE() prevents store tearing in this example:
> > +
> > + ACCESS_ONCE(p) = 0x00010002;
>
> Otherwise it could be read as a more generic statement (leaving out
> 'load tearing')?
>
> Thanks,
>
> Ingo
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 9:52 ` Mathieu Desnoyers
@ 2013-12-05 10:11 ` Ingo Molnar
0 siblings, 0 replies; 33+ messages in thread
From: Ingo Molnar @ 2013-12-05 10:11 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Paul E. McKenney, linux-kernel, laijs, dipankar, akpm, josh, niv,
tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec, sbw,
Oleg Nesterov, Jonathan Corbet, Rusty Russell
* Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- Original Message -----
> > From: "Ingo Molnar" <mingo@kernel.org>
> > To: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> > Cc: linux-kernel@vger.kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, "mathieu
> > desnoyers" <mathieu.desnoyers@efficios.com>, josh@joshtriplett.org, niv@us.ibm.com, tglx@linutronix.de,
> > peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, darren@dvhart.com,
> > fweisbec@gmail.com, sbw@mit.edu, "Oleg Nesterov" <oleg@redhat.com>, "Jonathan Corbet" <corbet@lwn.net>, "Rusty
> > Russell" <rusty@rustcorp.com.au>
> > Sent: Thursday, December 5, 2013 10:33:34 AM
> > Subject: Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
> >
> >
> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > + (*) The compiler is within its rights to reorder memory accesses unless
> > > + you tell it not to. For example, consider the following interaction
> > > + between process-level code and an interrupt handler:
> > > +
> > > + void process_level(void)
> > > + {
> > > + msg = get_message();
> > > + flag = true;
> > > + }
> > > +
> > > + void interrupt_handler(void)
> > > + {
> > > + if (flag)
> > > + process_message(msg);
> > > + }
> > > +
> > > + There is nothing to prevent the the compiler from transforming
> > > + process_level() to the following, in fact, this might well be a
> > > + win for single-threaded code:
> > > +
> > > + void process_level(void)
> > > + {
> > > + flag = true;
> > > + msg = get_message();
> > > + }
> > > +
> > > + If the interrupt occurs between these two statement, then
> > > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > > + to prevent this as follows:
> > > +
> > > + void process_level(void)
> > > + {
> > > + ACCESS_ONCE(msg) = get_message();
> > > + ACCESS_ONCE(flag) = true;
> > > + }
> > > +
> > > + void interrupt_handler(void)
> > > + {
> > > + if (ACCESS_ONCE(flag))
> > > + process_message(ACCESS_ONCE(msg));
> > > + }
> >
> > Technically, if the interrupt handler is the innermost context, the
> > ACCESS_ONCE() is not needed in the interrupt_handler() code.
> >
> > Since for the vast majority of Linux code IRQ handlers are the most
> > atomic contexts (very few drivers deal with NMIs) I suspect we should
> > either remove that ACCESS_ONCE() from the example or add a comment
> > explaining that in many cases those are superfluous?
>
> It might be worthwhile to state clearly those two different use-cases:
>
> * no nesting (e.g. process vs process), where both sides of the access
> need ACCESS_ONCE().
>
> * nested access: the outer context needs ACCESS_ONCE(), but not the
> inner context.
>
> We don't actually care about IRQs being the "most atomic" context,
> and we should not care about NMIs in this example. Only the relative
> nesting of the execution contexts accessing the data matter.
I meant 'most atomic' in the sense of no NMI context having access to
those same shared variables, obviously.
We do actually have 'triple' nesting in Linux, NMI context code with
non-trivial complexity that accesses variables in different code paths
from IRQ codepaths and from process level code paths, so technically
the original example is valid - I just think misleading to most
readers.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-04 22:46 [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt Paul E. McKenney
2013-12-05 0:10 ` [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Josh Triplett
@ 2013-12-05 10:59 ` Henrik Austad
2013-12-05 12:28 ` Ingo Molnar
2013-12-05 12:29 ` [PATCH v4 tip/core/locking 3/4] Documentation/memory-barriers.txt: Prohibit speculative writes Ingo Molnar
2 siblings, 2 replies; 33+ messages in thread
From: Henrik Austad @ 2013-12-05 10:59 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, niv, tglx, peterz, rostedt, dhowells, edumazet, darren,
fweisbec, sbw
On Wed, Dec 04, 2013 at 02:46:28PM -0800, Paul E. McKenney wrote:
> Hello!
Hi Paul,
> This series applies some long-needed updates to memory-barriers.txt:
>
> 1. Add ACCESS_ONCE() calls where needed to ensure their inclusion
> in code copy-and-pasted from this file.
>
> 2. Add long atomic examples alongside the existing atomics.
>
> 3. Prohibit architectures supporting the Linux kernel from
> speculating stores.
Just me, or is the 3rd patch missing from lkml?
> 4. Document what ACCESS_ONCE() does along with a number of situations
> requiring its use.
This was very useful, thanks! :)
--
Henrik Austad
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 10:59 ` Henrik Austad
@ 2013-12-05 12:28 ` Ingo Molnar
2013-12-05 13:51 ` Steven Rostedt
2013-12-05 12:29 ` [PATCH v4 tip/core/locking 3/4] Documentation/memory-barriers.txt: Prohibit speculative writes Ingo Molnar
1 sibling, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-05 12:28 UTC (permalink / raw)
To: Henrik Austad
Cc: Paul E. McKenney, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw
* Henrik Austad <henrik@austad.us> wrote:
> On Wed, Dec 04, 2013 at 02:46:28PM -0800, Paul E. McKenney wrote:
> > Hello!
>
> Hi Paul,
>
> > This series applies some long-needed updates to memory-barriers.txt:
> >
> > 1. Add ACCESS_ONCE() calls where needed to ensure their inclusion
> > in code copy-and-pasted from this file.
> >
> > 2. Add long atomic examples alongside the existing atomics.
> >
> > 3. Prohibit architectures supporting the Linux kernel from
> > speculating stores.
>
> Just me, or is the 3rd patch missing from lkml?
It had a Cc: list from hell so vger probably rejected it as spam.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH v4 tip/core/locking 3/4] Documentation/memory-barriers.txt: Prohibit speculative writes
2013-12-05 10:59 ` Henrik Austad
2013-12-05 12:28 ` Ingo Molnar
@ 2013-12-05 12:29 ` Ingo Molnar
1 sibling, 0 replies; 33+ messages in thread
From: Ingo Molnar @ 2013-12-05 12:29 UTC (permalink / raw)
To: Henrik Austad
Cc: Paul E. McKenney, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw
(here's #3.)
----- Forwarded message from "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> -----
Date: Wed, 4 Dec 2013 14:46:58 -0800
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
To: linux-kernel@vger.kernel.org
Cc: mingo@kernel.org, laijs@cn.fujitsu.com, dipankar@in.ibm.com, akpm@linux-foundation.org, mathieu.desnoyers@efficios.com, josh@joshtriplett.org, niv@us.ibm.com,
tglx@linutronix.de, peterz@infradead.org, rostedt@goodmis.org, dhowells@redhat.com, edumazet@google.com, darren@dvhart.com, fweisbec@gmail.com, sbw@mit.edu,
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com>, Richard Henderson <rth@twiddle.net>, Ivan Kokshaysky <ink@jurassic.park.msu.ru>, Matt Turner
<mattst88@gmail.com>, Russell King <linux@arm.linux.org.uk>, Arnd Bergmann <arnd@arndb.de>, Olof Johansson <olof@lixom.net>, Catalin Marinas
<catalin.marinas@arm.com>, Will Deacon <will.deacon@arm.com>, Haavard Skinnemoen <hskinnemoen@gmail.com>, Hans-Christian Egtvedt <egtvedt@samfundet.no>, Mike
Frysinger <vapier@gentoo.org>, Mark Salter <msalter@redhat.com>, Aurelien Jacquiot <a-jacquiot@ti.com>, Mikael Starvik <starvik@axis.com>, Jesper Nilsson
<jesper.nilsson@axis.com>, Yoshinori Sato <ysato@users.sourceforge.jp>, Richard Kuo <rkuo@codeaurora.org>, Tony Luck <tony.luck@intel.com>, Fenghua Yu
<fenghua.yu@intel.com>, Hirokazu Takata <takata@linux-m32r.org>, Geert Uytterhoeven <geert@linux-m68k.org>, James Hogan <james.hogan@imgtec.com>, Michal Simek
<monstr@monstr.eu>, Ralf Baechle <ralf@linux-mips.org>, Koichi Yasutake <yasutake.koichi@jp.panasonic.com>, Jonas Bonn <jonas@southpole.se>, "James E.J.
Bottomley" <jejb@parisc-linux.org>, Helge Deller <deller@gmx.de>, Benjamin Herrenschmidt <benh@kernel.crashing.org>, Paul Mackerras <paulus@samba.org>, Anatolij
Gustschin <agust@denx.de>, Josh Boyer <jwboyer@gmail.com>, Matt Porter <mporter@kernel.crashing.org>, Vitaly Bordug <vitb@kernel.crashing.org>, Kumar Gala
<galak@kernel.crashing.org>, Martin Schwidefsky <schwidefsky@de.ibm.com>, Heiko Carstens <heiko.carstens@de.ibm.com>, Chen Liqin <liqin.linux@gmail.com>, Lennox
Wu <lennox.wu@gmail.com>, Paul Mundt <lethal@linux-sh.org>, "David S. Miller" <davem@davemloft.net>, Chris Metcalf <cmetcalf@tilera.com>, Jeff Dike
<jdike@addtoit.com>, Richard Weinberger <richard@nod.at>, Guan Xuetao <gxt@mprc.pku.edu.cn>, Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
Chris Zankel <chris@zankel.net>, Max Filippov <jcmvbkbc@gmail.com>
Subject: [PATCH tip/core/locking 3/4] Documentation/memory-barriers.txt: Prohibit speculative writes
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
No SMP architecture currently supporting Linux allows speculative writes,
so this commit updates Documentation/memory-barriers.txt to prohibit
them in Linux core code. It also records restrictions on their use.
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Matt Turner <mattst88@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Haavard Skinnemoen <hskinnemoen@gmail.com>
Cc: Hans-Christian Egtvedt <egtvedt@samfundet.no>
Cc: Mike Frysinger <vapier@gentoo.org>
Cc: Mark Salter <msalter@redhat.com>
Cc: Aurelien Jacquiot <a-jacquiot@ti.com>
Cc: Mikael Starvik <starvik@axis.com>
Cc: Jesper Nilsson <jesper.nilsson@axis.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
Cc: Richard Kuo <rkuo@codeaurora.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Fenghua Yu <fenghua.yu@intel.com>
Cc: Hirokazu Takata <takata@linux-m32r.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: James Hogan <james.hogan@imgtec.com>
Cc: Michal Simek <monstr@monstr.eu>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Koichi Yasutake <yasutake.koichi@jp.panasonic.com>
Cc: Jonas Bonn <jonas@southpole.se>
Cc: "James E.J. Bottomley" <jejb@parisc-linux.org>
Cc: Helge Deller <deller@gmx.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Anatolij Gustschin <agust@denx.de>
Cc: Josh Boyer <jwboyer@gmail.com>
Cc: Matt Porter <mporter@kernel.crashing.org>
Cc: Vitaly Bordug <vitb@kernel.crashing.org>
Cc: Kumar Gala <galak@kernel.crashing.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: Chen Liqin <liqin.linux@gmail.com>
Cc: Lennox Wu <lennox.wu@gmail.com>
Cc: Paul Mundt <lethal@linux-sh.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Chris Metcalf <cmetcalf@tilera.com>
Cc: Jeff Dike <jdike@addtoit.com>
Cc: Richard Weinberger <richard@nod.at>
Cc: Guan Xuetao <gxt@mprc.pku.edu.cn>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Chris Zankel <chris@zankel.net>
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
Documentation/memory-barriers.txt | 183 ++++++++++++++++++++++++++++++++++++--
1 file changed, 175 insertions(+), 8 deletions(-)
diff --git a/Documentation/memory-barriers.txt b/Documentation/memory-barriers.txt
index 2d22da095a60..3e4429ef1458 100644
--- a/Documentation/memory-barriers.txt
+++ b/Documentation/memory-barriers.txt
@@ -571,11 +571,10 @@ dependency barrier to make it work correctly. Consider the following bit of
code:
q = ACCESS_ONCE(a);
- if (p) {
- <data dependency barrier>
- q = ACCESS_ONCE(b);
+ if (q) {
+ <data dependency barrier> /* BUG: No data dependency!!! */
+ p = ACCESS_ONCE(b);
}
- x = *q;
This will not have the desired effect because there is no actual data
dependency, but rather a control dependency that the CPU may short-circuit
@@ -584,11 +583,176 @@ the load from b as having happened before the load from a. In such a
case what's actually required is:
q = ACCESS_ONCE(a);
- if (p) {
+ if (q) {
<read barrier>
- q = ACCESS_ONCE(b);
+ p = ACCESS_ONCE(b);
}
- x = *q;
+
+However, stores are not speculated. This means that ordering -is- provided
+in the following example:
+
+ q = ACCESS_ONCE(a);
+ if (ACCESS_ONCE(q)) {
+ ACCESS_ONCE(b) = p;
+ }
+
+Please note that ACCESS_ONCE() is not optional! Without the ACCESS_ONCE(),
+the compiler is within its rights to transform this example:
+
+ q = a;
+ if (q) {
+ b = p; /* BUG: Compiler can reorder!!! */
+ do_something();
+ } else {
+ b = p; /* BUG: Compiler can reorder!!! */
+ do_something_else();
+ }
+
+into this, which of course defeats the ordering:
+
+ b = p;
+ q = a;
+ if (q)
+ do_something();
+ else
+ do_something_else();
+
+Worse yet, if the compiler is able to prove (say) that the value of
+variable a is always non-zero, it would be well within its rights
+to optimize the original example by eliminating the "if" statement
+as follows:
+
+ q = a;
+ b = p; /* BUG: Compiler can reorder!!! */
+ do_something();
+
+The solution is again ACCESS_ONCE(), which preserves the ordering between
+the load from variable a and the store to variable b:
+
+ q = ACCESS_ONCE(a);
+ if (q) {
+ ACCESS_ONCE(b) = p;
+ do_something();
+ } else {
+ ACCESS_ONCE(b) = p;
+ do_something_else();
+ }
+
+You could also use barrier() to prevent the compiler from moving
+the stores to variable b, but barrier() would not prevent the
+compiler from proving to itself that a==1 always, so ACCESS_ONCE()
+is also needed.
+
+It is important to note that control dependencies absolutely require a
+a conditional. For example, the following "optimized" version of
+the above example breaks ordering:
+
+ q = ACCESS_ONCE(a);
+ ACCESS_ONCE(b) = p; /* BUG: No ordering vs. load from a!!! */
+ if (q) {
+ /* ACCESS_ONCE(b) = p; -- moved up, BUG!!! */
+ do_something();
+ } else {
+ /* ACCESS_ONCE(b) = p; -- moved up, BUG!!! */
+ do_something_else();
+ }
+
+It is of course legal for the prior load to be part of the conditional,
+for example, as follows:
+
+ if (ACCESS_ONCE(a) > 0) {
+ ACCESS_ONCE(b) = q / 2;
+ do_something();
+ } else {
+ ACCESS_ONCE(b) = q / 3;
+ do_something_else();
+ }
+
+This will again ensure that the load from variable a is ordered before the
+stores to variable b.
+
+In addition, you need to be careful what you do with the local variable q,
+otherwise the compiler might be able to guess the value and again remove
+the needed conditional. For example:
+
+ q = ACCESS_ONCE(a);
+ if (q % MAX) {
+ ACCESS_ONCE(b) = p;
+ do_something();
+ } else {
+ ACCESS_ONCE(b) = p;
+ do_something_else();
+ }
+
+If MAX is defined to be 1, then the compiler knows that (q % MAX) is
+equal to zero, in which case the compiler is within its rights to
+transform the above code into the following:
+
+ q = ACCESS_ONCE(a);
+ ACCESS_ONCE(b) = p;
+ do_something_else();
+
+This transformation loses the ordering between the load from variable a
+and the store to variable b. If you are relying on this ordering, you
+should do something like the following:
+
+ q = ACCESS_ONCE(a);
+ BUILD_BUG_ON(MAX <= 1); /* Order load from a with store to b. */
+ if (q % MAX) {
+ ACCESS_ONCE(b) = p;
+ do_something();
+ } else {
+ ACCESS_ONCE(b) = p;
+ do_something_else();
+ }
+
+Finally, control dependencies do -not- provide transitivity. This is
+demonstrated by two related examples:
+
+ CPU 0 CPU 1
+ ===================== =====================
+ r1 = ACCESS_ONCE(x); r2 = ACCESS_ONCE(y);
+ if (r1 >= 0) if (r2 >= 0)
+ ACCESS_ONCE(y) = 1; ACCESS_ONCE(x) = 1;
+
+ assert(!(r1 == 1 && r2 == 1));
+
+The above two-CPU example will never trigger the assert(). However,
+if control dependencies guaranteed transitivity (which they do not),
+then adding the following two CPUs would guarantee a related assertion:
+
+ CPU 2 CPU 3
+ ===================== =====================
+ ACCESS_ONCE(x) = 2; ACCESS_ONCE(y) = 2;
+
+ assert(!(r1 == 2 && r2 == 2 && x == 1 && y == 1)); /* FAILS!!! */
+
+But because control dependencies do -not- provide transitivity, the
+above assertion can fail after the combined four-CPU example completes.
+If you need the four-CPU example to provide ordering, you will need
+smp_mb() between the loads and stores in the CPU 0 and CPU 1 code fragments.
+
+In summary:
+
+ (*) Control dependencies can order prior loads against later stores.
+ However, they do -not- guarantee any other sort of ordering:
+ Not prior loads against later loads, nor prior stores against
+ later anything. If you need these other forms of ordering,
+ use smb_rmb(), smp_wmb(), or, in the case of prior stores and
+ later loads, smp_mb().
+
+ (*) Control dependencies require at least one run-time conditional
+ between the prior load and the subsequent store. If the compiler
+ is able to optimize the conditional away, it will have also
+ optimized away the ordering. Careful use of ACCESS_ONCE() can
+ help to preserve the needed conditional.
+
+ (*) Control dependencies require that the compiler avoid reordering the
+ dependency into nonexistence. Careful use of ACCESS_ONCE() or
+ barrier() can help to preserve your control dependency.
+
+ (*) Control dependencies do -not- provide transitivity. If you
+ need transitivity, use smp_mb().
SMP BARRIER PAIRING
@@ -1083,7 +1247,10 @@ compiler from moving the memory accesses either side of it to the other side:
barrier();
-This is a general barrier - lesser varieties of compiler barrier do not exist.
+This is a general barrier -- there are no read-read or write-write variants
+of barrier(). Howevever, ACCESS_ONCE() can be thought of as a weak form
+for barrier() that affects only the specific accesses flagged by the
+ACCESS_ONCE().
The compiler barrier has no direct effect on the CPU, which may then reorder
things however it wishes.
--
1.8.1.5
----- End forwarded message -----
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 12:28 ` Ingo Molnar
@ 2013-12-05 13:51 ` Steven Rostedt
2013-12-05 18:05 ` David Miller
0 siblings, 1 reply; 33+ messages in thread
From: Steven Rostedt @ 2013-12-05 13:51 UTC (permalink / raw)
To: Ingo Molnar
Cc: Henrik Austad, Paul E. McKenney, linux-kernel, laijs, dipankar,
akpm, mathieu.desnoyers, josh, niv, tglx, peterz, dhowells,
edumazet, darren, fweisbec, sbw, David Miller
On Thu, 5 Dec 2013 13:28:51 +0100
Ingo Molnar <mingo@kernel.org> wrote:
> > Just me, or is the 3rd patch missing from lkml?
>
> It had a Cc: list from hell so vger probably rejected it as spam.
>
I wish vger wouldn't do that. I wonder how much spam is really flagged
by this one characteristic alone. That is, spam that would have made it
otherwise, but because of the Cc list, it was rejected.
-- Steve
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 9:33 ` Ingo Molnar
2013-12-05 9:52 ` Mathieu Desnoyers
@ 2013-12-05 18:02 ` Paul E. McKenney
2013-12-10 13:24 ` Ingo Molnar
1 sibling, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-05 18:02 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
On Thu, Dec 05, 2013 at 10:33:34AM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > + (*) The compiler is within its rights to reorder memory accesses unless
> > + you tell it not to. For example, consider the following interaction
> > + between process-level code and an interrupt handler:
> > +
> > + void process_level(void)
> > + {
> > + msg = get_message();
> > + flag = true;
> > + }
> > +
> > + void interrupt_handler(void)
> > + {
> > + if (flag)
> > + process_message(msg);
> > + }
> > +
> > + There is nothing to prevent the the compiler from transforming
> > + process_level() to the following, in fact, this might well be a
> > + win for single-threaded code:
> > +
> > + void process_level(void)
> > + {
> > + flag = true;
> > + msg = get_message();
> > + }
> > +
> > + If the interrupt occurs between these two statement, then
> > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > + to prevent this as follows:
> > +
> > + void process_level(void)
> > + {
> > + ACCESS_ONCE(msg) = get_message();
> > + ACCESS_ONCE(flag) = true;
> > + }
> > +
> > + void interrupt_handler(void)
> > + {
> > + if (ACCESS_ONCE(flag))
> > + process_message(ACCESS_ONCE(msg));
> > + }
>
> Technically, if the interrupt handler is the innermost context, the
> ACCESS_ONCE() is not needed in the interrupt_handler() code.
>
> Since for the vast majority of Linux code IRQ handlers are the most
> atomic contexts (very few drivers deal with NMIs) I suspect we should
> either remove that ACCESS_ONCE() from the example or add a comment
> explaining that in many cases those are superfluous?
How about the following additional paragraph?
Note that the ACCESS_ONCE() wrappers in interrupt_handler()
are needed if this interrupt handler can itself be interrupted
by something that also accesses 'flag' and 'msg', for example,
a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not
needed in interrupt_handler() other than for documentation purposes.
> > + (*) For aligned memory locations whose size allows them to be accessed
> > + with a single memory-reference instruction, prevents "load tearing"
> > + and "store tearing," in which a single large access is replaced by
> > + multiple smaller accesses. For example, given an architecture having
> > + 16-bit store instructions with 7-bit immediate fields, the compiler
> > + might be tempted to use two 16-bit store-immediate instructions to
> > + implement the following 32-bit store:
> > +
> > + p = 0x00010002;
> > +
> > + Please note that GCC really does use this sort of optimization,
> > + which is not surprising given that it would likely take more
> > + than two instructions to build the constant and then store it.
> > + This optimization can therefore be a win in single-threaded code.
> > + In fact, a recent bug (since fixed) caused GCC to incorrectly use
> > + this optimization in a volatile store. In the absence of such bugs,
> > + use of ACCESS_ONCE() prevents store tearing:
> > +
> > + ACCESS_ONCE(p) = 0x00010002;
>
> I suspect the last sentence should read:
>
> > + In the absence of such bugs,
> > + use of ACCESS_ONCE() prevents store tearing in this example:
> > +
> > + ACCESS_ONCE(p) = 0x00010002;
>
> Otherwise it could be read as a more generic statement (leaving out
> 'load tearing')?
Good point, fixed.
Indeed, I don't have a good example for load tearing. I do have some -bad-
examples, like the following:
struct __attribute__((__packed__)) foo {
short a;
int b;
short c;
};
struct foo foov;
short aa;
int bb;
short cc;
...
aa = foov.a;
bb = foov.b;
cc = foov.c;
A clever compiler might choose to pack aa, bb, and cc in memory, then
implement the three assignments using two 32-bit loads and two 32-bit
stores, which would result in load tearing of foov.b.
Hmmm... Maybe I should give this example anyway, just to show that
load tearing really could occur in practice... If nothing else, it
should be a cautionary tale for those tempted to pack their structures.
And there are quite a number of packed structures in the Linux kernel.
Sold! I have added this example, but using a pair of struct foo variables
in order to forestall maidenly protests from those who believe that no
production-quality compiler would ever misalign variable bb. ;-)
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 13:51 ` Steven Rostedt
@ 2013-12-05 18:05 ` David Miller
2013-12-05 18:18 ` Paul E. McKenney
2013-12-10 15:24 ` Ingo Molnar
0 siblings, 2 replies; 33+ messages in thread
From: David Miller @ 2013-12-05 18:05 UTC (permalink / raw)
To: rostedt
Cc: mingo, henrik, paulmck, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, dhowells, edumazet,
darren, fweisbec, sbw
From: Steven Rostedt <rostedt@goodmis.org>
Date: Thu, 5 Dec 2013 08:51:46 -0500
> I wish vger wouldn't do that. I wonder how much spam is really flagged
> by this one characteristic alone. That is, spam that would have made it
> otherwise, but because of the Cc list, it was rejected.
10 to 20 spam posts per day are prevented by this rule.
And frankly it's totally rediculous to have such a huge CC: list
in the first place, even if the vger spam filter didn't exist.
If you CC: something to netdev, it's going to reach me, you don't need
to CC: me. If just makes for a dup that I have to delete in my inbox,
so you're actually making more work for me in the end.
That's just one example.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 9:50 ` Ingo Molnar
@ 2013-12-05 18:05 ` Paul E. McKenney
2013-12-05 22:47 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-05 18:05 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
On Thu, Dec 05, 2013 at 10:50:42AM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > + (*) The compiler is within its rights to reload a variable, for example,
> > + in cases where high register pressure prevents the compiler from
> > + keeping all data of interest in registers. The compiler might
> > + therefore optimize the variable tmp out of our previous example:
> > +
> > + while (tmp = a)
> > + do_something_with(tmp);
> > +
> > + This could result in the following code, which is perfectly safe in
> > + single-threaded code, but can be fatal in concurrent code:
> > +
> > + while (a)
> > + do_something_with(a);
> > +
> > + For example, the optimized version of this code could result in
> > + passing a zero to do_something_with() in the case where the variable
> > + a was modified by some other CPU between the "while" statement and
> > + the call to do_something_with().
>
> Nit: I guess references to variable names such as 'a' should be quoted
> (same for 'tmp', 'b', etc):
>
> For example, the optimized version of this code could result in
> passing a zero to do_something_with() in the case where the variable
> 'a' was modified by some other CPU between the "while" statement and
> the call to do_something_with().
>
> which makes reading it less ambiguous and more fluid IMO. This
> observation applies to the whole document as 'a' is used in many
> places.
Good point, fixed.
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 18:05 ` David Miller
@ 2013-12-05 18:18 ` Paul E. McKenney
2013-12-05 18:44 ` David Miller
2013-12-10 15:24 ` Ingo Molnar
1 sibling, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-05 18:18 UTC (permalink / raw)
To: David Miller
Cc: rostedt, mingo, henrik, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, dhowells, edumazet,
darren, fweisbec, sbw
On Thu, Dec 05, 2013 at 01:05:24PM -0500, David Miller wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> Date: Thu, 5 Dec 2013 08:51:46 -0500
>
> > I wish vger wouldn't do that. I wonder how much spam is really flagged
> > by this one characteristic alone. That is, spam that would have made it
> > otherwise, but because of the Cc list, it was rejected.
>
> 10 to 20 spam posts per day are prevented by this rule.
>
> And frankly it's totally rediculous to have such a huge CC: list
> in the first place, even if the vger spam filter didn't exist.
>
> If you CC: something to netdev, it's going to reach me, you don't need
> to CC: me. If just makes for a dup that I have to delete in my inbox,
> so you're actually making more work for me in the end.
>
> That's just one example.
Hello, David,
The situation that leads me to use a large CC list is when I am doing
something that affects all architectures. I could imagine keeping a
smallish CC list, then forwarding or bouncing the email to the remaining
maintainers Would that work reasonably, or is there some better approach?
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 18:18 ` Paul E. McKenney
@ 2013-12-05 18:44 ` David Miller
2013-12-05 19:01 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: David Miller @ 2013-12-05 18:44 UTC (permalink / raw)
To: paulmck
Cc: rostedt, mingo, henrik, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, dhowells, edumazet,
darren, fweisbec, sbw
From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
Date: Thu, 5 Dec 2013 10:18:34 -0800
> The situation that leads me to use a large CC list is when I am doing
> something that affects all architectures. I could imagine keeping a
> smallish CC list, then forwarding or bouncing the email to the remaining
> maintainers Would that work reasonably, or is there some better approach?
Please use linux-arch, that's what that list is for, notifying arch
maintainers of changes that basically have an impact on every target.
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 18:44 ` David Miller
@ 2013-12-05 19:01 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-05 19:01 UTC (permalink / raw)
To: David Miller
Cc: rostedt, mingo, henrik, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, dhowells, edumazet,
darren, fweisbec, sbw
On Thu, Dec 05, 2013 at 01:44:06PM -0500, David Miller wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> Date: Thu, 5 Dec 2013 10:18:34 -0800
>
> > The situation that leads me to use a large CC list is when I am doing
> > something that affects all architectures. I could imagine keeping a
> > smallish CC list, then forwarding or bouncing the email to the remaining
> > maintainers Would that work reasonably, or is there some better approach?
>
> Please use linux-arch, that's what that list is for, notifying arch
> maintainers of changes that basically have an impact on every target.
Thank you, I have updated that commit as you suggested.
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-04 22:46 ` [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() Paul E. McKenney
2013-12-05 9:33 ` Ingo Molnar
2013-12-05 9:50 ` Ingo Molnar
@ 2013-12-05 20:21 ` Jonathan Corbet
2013-12-05 21:44 ` Paul E. McKenney
2 siblings, 1 reply; 33+ messages in thread
From: Jonathan Corbet @ 2013-12-05 20:21 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, niv, tglx, peterz, rostedt, dhowells, edumazet, darren,
fweisbec, sbw, Oleg Nesterov, Rusty Russell
On Wed, 4 Dec 2013 14:46:59 -0800
"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
>
> The situations in which ACCESS_ONCE() is required are not well documented,
> so this commit adds some verbiage to memory-barriers.txt.
[...]
> + But please note that the compiler is also closely watching what you
> + do with the value after the ACCESS_ONCE(). For example, suppose you
> + do the following and MAX is a preprocessor macro with the value 1:
> +
> + for ((tmp = ACCESS_ONCE(a)) % MAX)
> + do_something_with(tmp);
That sure looks like it was meant to be "while" instead of "for"?
[...]
> + (*) The compiler is within its rights to reorder memory accesses unless
> + you tell it not to. For example, consider the following interaction
> + between process-level code and an interrupt handler:
> +
> + void process_level(void)
> + {
> + msg = get_message();
> + flag = true;
> + }
> +
> + void interrupt_handler(void)
> + {
> + if (flag)
> + process_message(msg);
> + }
> +
> + There is nothing to prevent the the compiler from transforming
> + process_level() to the following, in fact, this might well be a
> + win for single-threaded code:
> +
> + void process_level(void)
> + {
> + flag = true;
> + msg = get_message();
> + }
> +
> + If the interrupt occurs between these two statement, then
> + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> + to prevent this as follows:
> +
> + void process_level(void)
> + {
> + ACCESS_ONCE(msg) = get_message();
> + ACCESS_ONCE(flag) = true;
> + }
> +
> + void interrupt_handler(void)
> + {
> + if (ACCESS_ONCE(flag))
> + process_message(ACCESS_ONCE(msg));
> + }
Looking at this, I find myself wondering why you couldn't just put a
barrier() between the two statements in process_level()? ACCESS_ONCE()
seems like a heavy hammer to just avoid reordering of two assignments.
What am I missing, and what could be added here to keep the other folks as
dense as me from missing the same thing?
Thanks,
jon
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 20:21 ` Jonathan Corbet
@ 2013-12-05 21:44 ` Paul E. McKenney
2013-12-10 15:20 ` Ingo Molnar
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-05 21:44 UTC (permalink / raw)
To: Jonathan Corbet
Cc: linux-kernel, mingo, laijs, dipankar, akpm, mathieu.desnoyers,
josh, niv, tglx, peterz, rostedt, dhowells, edumazet, darren,
fweisbec, sbw, Oleg Nesterov, Rusty Russell
On Thu, Dec 05, 2013 at 01:21:01PM -0700, Jonathan Corbet wrote:
> On Wed, 4 Dec 2013 14:46:59 -0800
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
>
> > From: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
> >
> > The situations in which ACCESS_ONCE() is required are not well documented,
> > so this commit adds some verbiage to memory-barriers.txt.
>
> [...]
>
> > + But please note that the compiler is also closely watching what you
> > + do with the value after the ACCESS_ONCE(). For example, suppose you
> > + do the following and MAX is a preprocessor macro with the value 1:
> > +
> > + for ((tmp = ACCESS_ONCE(a)) % MAX)
> > + do_something_with(tmp);
>
> That sure looks like it was meant to be "while" instead of "for"?
Good catch, fixed!
> [...]
>
> > + (*) The compiler is within its rights to reorder memory accesses unless
> > + you tell it not to. For example, consider the following interaction
> > + between process-level code and an interrupt handler:
> > +
> > + void process_level(void)
> > + {
> > + msg = get_message();
> > + flag = true;
> > + }
> > +
> > + void interrupt_handler(void)
> > + {
> > + if (flag)
> > + process_message(msg);
> > + }
> > +
> > + There is nothing to prevent the the compiler from transforming
> > + process_level() to the following, in fact, this might well be a
> > + win for single-threaded code:
> > +
> > + void process_level(void)
> > + {
> > + flag = true;
> > + msg = get_message();
> > + }
> > +
> > + If the interrupt occurs between these two statement, then
> > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > + to prevent this as follows:
> > +
> > + void process_level(void)
> > + {
> > + ACCESS_ONCE(msg) = get_message();
> > + ACCESS_ONCE(flag) = true;
> > + }
> > +
> > + void interrupt_handler(void)
> > + {
> > + if (ACCESS_ONCE(flag))
> > + process_message(ACCESS_ONCE(msg));
> > + }
>
> Looking at this, I find myself wondering why you couldn't just put a
> barrier() between the two statements in process_level()? ACCESS_ONCE()
> seems like a heavy hammer to just avoid reordering of two assignments.
> What am I missing, and what could be added here to keep the other folks as
> dense as me from missing the same thing?
You could use barrier() from an ordering viewpoint. However,
ACCESS_ONCE() is often lighter weight than barrier(). ACCESS_ONCE()
affects only that one access, while barrier() forces the compiler to
forget pretty much anything it previously gleaned from any region of
memory, including private locations that no one else touches.
I am adding a sentence saying that pure ordering can be provided
by barrier(), though often at higher cost.
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 18:05 ` Paul E. McKenney
@ 2013-12-05 22:47 ` Paul E. McKenney
2013-12-10 15:10 ` Ingo Molnar
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-05 22:47 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
On Thu, Dec 05, 2013 at 10:05:47AM -0800, Paul E. McKenney wrote:
> On Thu, Dec 05, 2013 at 10:50:42AM +0100, Ingo Molnar wrote:
> >
> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > + (*) The compiler is within its rights to reload a variable, for example,
> > > + in cases where high register pressure prevents the compiler from
> > > + keeping all data of interest in registers. The compiler might
> > > + therefore optimize the variable tmp out of our previous example:
> > > +
> > > + while (tmp = a)
> > > + do_something_with(tmp);
> > > +
> > > + This could result in the following code, which is perfectly safe in
> > > + single-threaded code, but can be fatal in concurrent code:
> > > +
> > > + while (a)
> > > + do_something_with(a);
> > > +
> > > + For example, the optimized version of this code could result in
> > > + passing a zero to do_something_with() in the case where the variable
> > > + a was modified by some other CPU between the "while" statement and
> > > + the call to do_something_with().
> >
> > Nit: I guess references to variable names such as 'a' should be quoted
> > (same for 'tmp', 'b', etc):
> >
> > For example, the optimized version of this code could result in
> > passing a zero to do_something_with() in the case where the variable
> > 'a' was modified by some other CPU between the "while" statement and
> > the call to do_something_with().
> >
> > which makes reading it less ambiguous and more fluid IMO. This
> > observation applies to the whole document as 'a' is used in many
> > places.
>
> Good point, fixed.
Which reminds me -- the thing that makes me most nervous about prohibiting
speculative stores is the bit about having to anticipate all compiler
optimizations that might get rid of the needed conditionals.
Thoughts?
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 18:02 ` Paul E. McKenney
@ 2013-12-10 13:24 ` Ingo Molnar
2013-12-10 17:36 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-10 13:24 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Dec 05, 2013 at 10:33:34AM +0100, Ingo Molnar wrote:
> >
> > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> >
> > > + (*) The compiler is within its rights to reorder memory accesses unless
> > > + you tell it not to. For example, consider the following interaction
> > > + between process-level code and an interrupt handler:
> > > +
> > > + void process_level(void)
> > > + {
> > > + msg = get_message();
> > > + flag = true;
> > > + }
> > > +
> > > + void interrupt_handler(void)
> > > + {
> > > + if (flag)
> > > + process_message(msg);
> > > + }
> > > +
> > > + There is nothing to prevent the the compiler from transforming
> > > + process_level() to the following, in fact, this might well be a
> > > + win for single-threaded code:
> > > +
> > > + void process_level(void)
> > > + {
> > > + flag = true;
> > > + msg = get_message();
> > > + }
> > > +
> > > + If the interrupt occurs between these two statement, then
> > > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > > + to prevent this as follows:
> > > +
> > > + void process_level(void)
> > > + {
> > > + ACCESS_ONCE(msg) = get_message();
> > > + ACCESS_ONCE(flag) = true;
> > > + }
> > > +
> > > + void interrupt_handler(void)
> > > + {
> > > + if (ACCESS_ONCE(flag))
> > > + process_message(ACCESS_ONCE(msg));
> > > + }
> >
> > Technically, if the interrupt handler is the innermost context, the
> > ACCESS_ONCE() is not needed in the interrupt_handler() code.
> >
> > Since for the vast majority of Linux code IRQ handlers are the most
> > atomic contexts (very few drivers deal with NMIs) I suspect we should
> > either remove that ACCESS_ONCE() from the example or add a comment
> > explaining that in many cases those are superfluous?
>
> How about the following additional paragraph?
>
> Note that the ACCESS_ONCE() wrappers in interrupt_handler()
> are needed if this interrupt handler can itself be interrupted
> by something that also accesses 'flag' and 'msg', for example,
> a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not
> needed in interrupt_handler() other than for documentation purposes.
Sounds great to me!
Note that nested IRQs generally don't happen on modern Linux anymore,
we run almost all hardirqs with irqs disabled and in fact have a
warning to detect irq handlers that enable irqs:
res = action->handler(irq, action->dev_id);
trace_irq_handler_exit(irq, action, res);
if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled interrupts\n",
irq, action->handler))
local_irq_disable();
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 22:47 ` Paul E. McKenney
@ 2013-12-10 15:10 ` Ingo Molnar
2013-12-10 17:37 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-10 15:10 UTC (permalink / raw)
To: Paul E. McKenney
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> On Thu, Dec 05, 2013 at 10:05:47AM -0800, Paul E. McKenney wrote:
> > On Thu, Dec 05, 2013 at 10:50:42AM +0100, Ingo Molnar wrote:
> > >
> > > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > + (*) The compiler is within its rights to reload a variable, for example,
> > > > + in cases where high register pressure prevents the compiler from
> > > > + keeping all data of interest in registers. The compiler might
> > > > + therefore optimize the variable tmp out of our previous example:
> > > > +
> > > > + while (tmp = a)
> > > > + do_something_with(tmp);
> > > > +
> > > > + This could result in the following code, which is perfectly safe in
> > > > + single-threaded code, but can be fatal in concurrent code:
> > > > +
> > > > + while (a)
> > > > + do_something_with(a);
> > > > +
> > > > + For example, the optimized version of this code could result in
> > > > + passing a zero to do_something_with() in the case where the variable
> > > > + a was modified by some other CPU between the "while" statement and
> > > > + the call to do_something_with().
> > >
> > > Nit: I guess references to variable names such as 'a' should be quoted
> > > (same for 'tmp', 'b', etc):
> > >
> > > For example, the optimized version of this code could result in
> > > passing a zero to do_something_with() in the case where the variable
> > > 'a' was modified by some other CPU between the "while" statement and
> > > the call to do_something_with().
> > >
> > > which makes reading it less ambiguous and more fluid IMO. This
> > > observation applies to the whole document as 'a' is used in many
> > > places.
> >
> > Good point, fixed.
>
> Which reminds me -- the thing that makes me most nervous about
> prohibiting speculative stores is the bit about having to anticipate
> all compiler optimizations that might get rid of the needed
> conditionals.
>
> Thoughts?
As long as current compiler versions behave I don't the potential of
future problems is a problem that can (or should) be solved via
documentation - there will always be a colorful tension between
specification and reality, both at the hardware, the code and the
compiler level ;-)
It doesn't hurt to outline our expectations in any case, agreed?
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-05 21:44 ` Paul E. McKenney
@ 2013-12-10 15:20 ` Ingo Molnar
2013-12-10 17:44 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-10 15:20 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Jonathan Corbet, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw, Oleg Nesterov, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > [...]
> >
> > > + (*) The compiler is within its rights to reorder memory accesses unless
> > > + you tell it not to. For example, consider the following interaction
> > > + between process-level code and an interrupt handler:
> > > +
> > > + void process_level(void)
> > > + {
> > > + msg = get_message();
> > > + flag = true;
> > > + }
> > > +
> > > + void interrupt_handler(void)
> > > + {
> > > + if (flag)
> > > + process_message(msg);
> > > + }
> > > +
> > > + There is nothing to prevent the the compiler from transforming
> > > + process_level() to the following, in fact, this might well be a
> > > + win for single-threaded code:
> > > +
> > > + void process_level(void)
> > > + {
> > > + flag = true;
> > > + msg = get_message();
> > > + }
> > > +
> > > + If the interrupt occurs between these two statement, then
> > > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > > + to prevent this as follows:
> > > +
> > > + void process_level(void)
> > > + {
> > > + ACCESS_ONCE(msg) = get_message();
> > > + ACCESS_ONCE(flag) = true;
> > > + }
> > > +
> > > + void interrupt_handler(void)
> > > + {
> > > + if (ACCESS_ONCE(flag))
> > > + process_message(ACCESS_ONCE(msg));
> > > + }
> >
> > Looking at this, I find myself wondering why you couldn't just put a
> > barrier() between the two statements in process_level()? ACCESS_ONCE()
> > seems like a heavy hammer to just avoid reordering of two assignments.
> > What am I missing, and what could be added here to keep the other folks as
> > dense as me from missing the same thing?
>
> You could use barrier() from an ordering viewpoint. However,
> ACCESS_ONCE() is often lighter weight than barrier(). ACCESS_ONCE()
> affects only that one access, while barrier() forces the compiler to
> forget pretty much anything it previously gleaned from any region of
> memory, including private locations that no one else touches.
>
> I am adding a sentence saying that pure ordering can be provided by
> barrier(), though often at higher cost.
I suspect a related question would be, is the compiler allowed to
reorder:
x = ACCESS_ONCE(a);
y = ACCESS_ONCE(b);
?
This wording:
+ [...] Howevever, ACCESS_ONCE() can be thought of as a weak form
+for barrier() that affects only the specific accesses flagged by the
+ACCESS_ONCE().
Does not seem to be obvious enough to me - does it affect accesses to
the variables referenced (but still allows accesses to separate
variables reordered), or does it affect compiler-ordering of all
ACCESS_ONCE() instances, instructing the compiler to preserve program
order?
Also, it's not clear what happens if non-ACCESS_ONCE() access to a
variable is mixed with ACCESS_ONCE() access.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates
2013-12-05 18:05 ` David Miller
2013-12-05 18:18 ` Paul E. McKenney
@ 2013-12-10 15:24 ` Ingo Molnar
1 sibling, 0 replies; 33+ messages in thread
From: Ingo Molnar @ 2013-12-10 15:24 UTC (permalink / raw)
To: David Miller
Cc: rostedt, henrik, paulmck, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, dhowells, edumazet,
darren, fweisbec, sbw
* David Miller <davem@davemloft.net> wrote:
> From: Steven Rostedt <rostedt@goodmis.org>
> Date: Thu, 5 Dec 2013 08:51:46 -0500
>
> > I wish vger wouldn't do that. I wonder how much spam is really flagged
> > by this one characteristic alone. That is, spam that would have made it
> > otherwise, but because of the Cc list, it was rejected.
>
> 10 to 20 spam posts per day are prevented by this rule.
>
> And frankly it's totally rediculous to have such a huge CC: list
> in the first place, even if the vger spam filter didn't exist.
Absolutely agreed - in fact huge Cc: lists are a PITA when committing
patches: I have to trim them down to something reasonable, without
losing important Cc: entries ...
So vger is doing us a service there, and not just by filtering spam.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 13:24 ` Ingo Molnar
@ 2013-12-10 17:36 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-10 17:36 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
On Tue, Dec 10, 2013 at 02:24:48PM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > On Thu, Dec 05, 2013 at 10:33:34AM +0100, Ingo Molnar wrote:
> > >
> > > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > >
> > > > + (*) The compiler is within its rights to reorder memory accesses unless
> > > > + you tell it not to. For example, consider the following interaction
> > > > + between process-level code and an interrupt handler:
> > > > +
> > > > + void process_level(void)
> > > > + {
> > > > + msg = get_message();
> > > > + flag = true;
> > > > + }
> > > > +
> > > > + void interrupt_handler(void)
> > > > + {
> > > > + if (flag)
> > > > + process_message(msg);
> > > > + }
> > > > +
> > > > + There is nothing to prevent the the compiler from transforming
> > > > + process_level() to the following, in fact, this might well be a
> > > > + win for single-threaded code:
> > > > +
> > > > + void process_level(void)
> > > > + {
> > > > + flag = true;
> > > > + msg = get_message();
> > > > + }
> > > > +
> > > > + If the interrupt occurs between these two statement, then
> > > > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > > > + to prevent this as follows:
> > > > +
> > > > + void process_level(void)
> > > > + {
> > > > + ACCESS_ONCE(msg) = get_message();
> > > > + ACCESS_ONCE(flag) = true;
> > > > + }
> > > > +
> > > > + void interrupt_handler(void)
> > > > + {
> > > > + if (ACCESS_ONCE(flag))
> > > > + process_message(ACCESS_ONCE(msg));
> > > > + }
> > >
> > > Technically, if the interrupt handler is the innermost context, the
> > > ACCESS_ONCE() is not needed in the interrupt_handler() code.
> > >
> > > Since for the vast majority of Linux code IRQ handlers are the most
> > > atomic contexts (very few drivers deal with NMIs) I suspect we should
> > > either remove that ACCESS_ONCE() from the example or add a comment
> > > explaining that in many cases those are superfluous?
> >
> > How about the following additional paragraph?
> >
> > Note that the ACCESS_ONCE() wrappers in interrupt_handler()
> > are needed if this interrupt handler can itself be interrupted
> > by something that also accesses 'flag' and 'msg', for example,
> > a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not
> > needed in interrupt_handler() other than for documentation purposes.
>
> Sounds great to me!
>
> Note that nested IRQs generally don't happen on modern Linux anymore,
> we run almost all hardirqs with irqs disabled and in fact have a
> warning to detect irq handlers that enable irqs:
>
> res = action->handler(irq, action->dev_id);
> trace_irq_handler_exit(irq, action, res);
>
> if (WARN_ONCE(!irqs_disabled(),"irq %u handler %pF enabled interrupts\n",
> irq, action->handler))
> local_irq_disable();
Good point! I added the following at the end of the paragraph:
(Note also that nested interrupts do not typically occur in
modern Linux kernels, in fact, if an interrupt handler returns
with interrupts enabled, you will get a WARN_ONCE() splat.)
I guess an IRQ handler could momentarily enable interrupts as long as
it disabled them again before returning, but I don't see any reason
to encourage that practice in Documentation/memory-barriers.txt. ;-)
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 15:10 ` Ingo Molnar
@ 2013-12-10 17:37 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-10 17:37 UTC (permalink / raw)
To: Ingo Molnar
Cc: linux-kernel, laijs, dipankar, akpm, mathieu.desnoyers, josh,
niv, tglx, peterz, rostedt, dhowells, edumazet, darren, fweisbec,
sbw, Oleg Nesterov, Jonathan Corbet, Rusty Russell
On Tue, Dec 10, 2013 at 04:10:50PM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > On Thu, Dec 05, 2013 at 10:05:47AM -0800, Paul E. McKenney wrote:
> > > On Thu, Dec 05, 2013 at 10:50:42AM +0100, Ingo Molnar wrote:
> > > >
> > > > * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > > >
> > > > > + (*) The compiler is within its rights to reload a variable, for example,
> > > > > + in cases where high register pressure prevents the compiler from
> > > > > + keeping all data of interest in registers. The compiler might
> > > > > + therefore optimize the variable tmp out of our previous example:
> > > > > +
> > > > > + while (tmp = a)
> > > > > + do_something_with(tmp);
> > > > > +
> > > > > + This could result in the following code, which is perfectly safe in
> > > > > + single-threaded code, but can be fatal in concurrent code:
> > > > > +
> > > > > + while (a)
> > > > > + do_something_with(a);
> > > > > +
> > > > > + For example, the optimized version of this code could result in
> > > > > + passing a zero to do_something_with() in the case where the variable
> > > > > + a was modified by some other CPU between the "while" statement and
> > > > > + the call to do_something_with().
> > > >
> > > > Nit: I guess references to variable names such as 'a' should be quoted
> > > > (same for 'tmp', 'b', etc):
> > > >
> > > > For example, the optimized version of this code could result in
> > > > passing a zero to do_something_with() in the case where the variable
> > > > 'a' was modified by some other CPU between the "while" statement and
> > > > the call to do_something_with().
> > > >
> > > > which makes reading it less ambiguous and more fluid IMO. This
> > > > observation applies to the whole document as 'a' is used in many
> > > > places.
> > >
> > > Good point, fixed.
> >
> > Which reminds me -- the thing that makes me most nervous about
> > prohibiting speculative stores is the bit about having to anticipate
> > all compiler optimizations that might get rid of the needed
> > conditionals.
> >
> > Thoughts?
>
> As long as current compiler versions behave I don't the potential of
> future problems is a problem that can (or should) be solved via
> documentation - there will always be a colorful tension between
> specification and reality, both at the hardware, the code and the
> compiler level ;-)
There certainly has been in the past. ;-)
> It doesn't hurt to outline our expectations in any case, agreed?
Fair enough, I will leave it as is.
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 15:20 ` Ingo Molnar
@ 2013-12-10 17:44 ` Paul E. McKenney
2013-12-10 18:28 ` Ingo Molnar
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-10 17:44 UTC (permalink / raw)
To: Ingo Molnar
Cc: Jonathan Corbet, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw, Oleg Nesterov, Rusty Russell
On Tue, Dec 10, 2013 at 04:20:06PM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > > [...]
> > >
> > > > + (*) The compiler is within its rights to reorder memory accesses unless
> > > > + you tell it not to. For example, consider the following interaction
> > > > + between process-level code and an interrupt handler:
> > > > +
> > > > + void process_level(void)
> > > > + {
> > > > + msg = get_message();
> > > > + flag = true;
> > > > + }
> > > > +
> > > > + void interrupt_handler(void)
> > > > + {
> > > > + if (flag)
> > > > + process_message(msg);
> > > > + }
> > > > +
> > > > + There is nothing to prevent the the compiler from transforming
> > > > + process_level() to the following, in fact, this might well be a
> > > > + win for single-threaded code:
> > > > +
> > > > + void process_level(void)
> > > > + {
> > > > + flag = true;
> > > > + msg = get_message();
> > > > + }
> > > > +
> > > > + If the interrupt occurs between these two statement, then
> > > > + interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > > > + to prevent this as follows:
> > > > +
> > > > + void process_level(void)
> > > > + {
> > > > + ACCESS_ONCE(msg) = get_message();
> > > > + ACCESS_ONCE(flag) = true;
> > > > + }
> > > > +
> > > > + void interrupt_handler(void)
> > > > + {
> > > > + if (ACCESS_ONCE(flag))
> > > > + process_message(ACCESS_ONCE(msg));
> > > > + }
> > >
> > > Looking at this, I find myself wondering why you couldn't just put a
> > > barrier() between the two statements in process_level()? ACCESS_ONCE()
> > > seems like a heavy hammer to just avoid reordering of two assignments.
> > > What am I missing, and what could be added here to keep the other folks as
> > > dense as me from missing the same thing?
> >
> > You could use barrier() from an ordering viewpoint. However,
> > ACCESS_ONCE() is often lighter weight than barrier(). ACCESS_ONCE()
> > affects only that one access, while barrier() forces the compiler to
> > forget pretty much anything it previously gleaned from any region of
> > memory, including private locations that no one else touches.
> >
> > I am adding a sentence saying that pure ordering can be provided by
> > barrier(), though often at higher cost.
>
> I suspect a related question would be, is the compiler allowed to
> reorder:
>
>
> x = ACCESS_ONCE(a);
> y = ACCESS_ONCE(b);
>
> ?
>
> This wording:
>
> + [...] Howevever, ACCESS_ONCE() can be thought of as a weak form
And I guess I should fix the "Howevever" as well. Or make up a meaning
for it, this being English and all...
> +for barrier() that affects only the specific accesses flagged by the
> +ACCESS_ONCE().
>
> Does not seem to be obvious enough to me - does it affect accesses to
> the variables referenced (but still allows accesses to separate
> variables reordered), or does it affect compiler-ordering of all
> ACCESS_ONCE() instances, instructing the compiler to preserve program
> order?
I cover this in the bullet item about reordering memory accesses:
(*) The compiler is within its rights to reorder memory accesses unless
you tell it not to. For example, consider the following interaction
between process-level code and an interrupt handler:
void process_level(void)
{
msg = get_message();
flag = true;
}
void interrupt_handler(void)
{
if (flag)
process_message(msg);
}
There is nothing to prevent the the compiler from transforming
process_level() to the following, in fact, this might well be a
win for single-threaded code:
void process_level(void)
{
flag = true;
msg = get_message();
}
If the interrupt occurs between these two statement, then
interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
to prevent this as follows:
void process_level(void)
{
ACCESS_ONCE(msg) = get_message();
ACCESS_ONCE(flag) = true;
}
void interrupt_handler(void)
{
if (ACCESS_ONCE(flag))
process_message(ACCESS_ONCE(msg));
}
Note that the ACCESS_ONCE() wrappers in interrupt_handler()
are needed if this interrupt handler can itself be interrupted
by something that also accesses 'flag' and 'msg', for example,
a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not
needed in interrupt_handler() other than for documentation purposes.
(Note also that nested interrupts do not typically occur in modern
Linux kernels, in fact, if an interrupt handler returns with
interrupts enabled, you will get a WARN_ONCE() splat.)
This effect could also be achieved using barrier(), but ACCESS_ONCE()
is more selective: With ACCESS_ONCE(), the compiler need only forget
the contents of the indicated memory located, while with barrier()
the compiler must discard the value of all memory locations that
it has currented cached in any machine registers.
Does that cover it?
> Also, it's not clear what happens if non-ACCESS_ONCE() access to a
> variable is mixed with ACCESS_ONCE() access.
Different versions of the standard say different things here, and the
committee seems quite reluctant to tighten up volatile accesses. I
believe that the best approach is to say that the compiler is free to
order ACCESS_ONCE() with non-ACCESS_ONCE() code. How about the
following?
You should assume that the compiler can move ACCESS_ONCE()
past code not containing ACCESS_ONCE(), barrier(), or similar
primitives.
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 17:44 ` Paul E. McKenney
@ 2013-12-10 18:28 ` Ingo Molnar
2013-12-10 19:01 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-10 18:28 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Jonathan Corbet, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw, Oleg Nesterov, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > +for barrier() that affects only the specific accesses flagged by the
> > +ACCESS_ONCE().
> >
> > Does not seem to be obvious enough to me - does it affect accesses
> > to the variables referenced (but still allows accesses to separate
> > variables reordered), or does it affect compiler-ordering of all
> > ACCESS_ONCE() instances, instructing the compiler to preserve
> > program order?
>
> I cover this in the bullet item about reordering memory accesses:
>
> (*) The compiler is within its rights to reorder memory accesses unless
> you tell it not to. For example, consider the following interaction
> between process-level code and an interrupt handler:
>
> void process_level(void)
> {
> msg = get_message();
> flag = true;
> }
>
> void interrupt_handler(void)
> {
> if (flag)
> process_message(msg);
> }
>
> There is nothing to prevent the the compiler from transforming
> process_level() to the following, in fact, this might well be a
> win for single-threaded code:
>
> void process_level(void)
> {
> flag = true;
> msg = get_message();
> }
>
> If the interrupt occurs between these two statement, then
> interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> to prevent this as follows:
>
> void process_level(void)
> {
> ACCESS_ONCE(msg) = get_message();
> ACCESS_ONCE(flag) = true;
> }
>
> void interrupt_handler(void)
> {
> if (ACCESS_ONCE(flag))
> process_message(ACCESS_ONCE(msg));
> }
>
> Note that the ACCESS_ONCE() wrappers in interrupt_handler()
> are needed if this interrupt handler can itself be interrupted
> by something that also accesses 'flag' and 'msg', for example,
> a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not
> needed in interrupt_handler() other than for documentation purposes.
> (Note also that nested interrupts do not typically occur in modern
> Linux kernels, in fact, if an interrupt handler returns with
> interrupts enabled, you will get a WARN_ONCE() splat.)
>
> This effect could also be achieved using barrier(), but ACCESS_ONCE()
> is more selective: With ACCESS_ONCE(), the compiler need only forget
> the contents of the indicated memory located, while with barrier()
> the compiler must discard the value of all memory locations that
> it has currented cached in any machine registers.
>
> Does that cover it?
btw.:
s/indicated memory located/
indicated memory location
?
So, what I don't see this statement cover (and I might be dense about
it!) is whether two ACCESS_ONCE() macros referring to different
variables are allowed to be reordered with each other.
If the compiler reorders:
ACCESS_ONCE(x);
ACCESS_ONCE(y);
to:
ACCESS_ONCE(y);
ACCESS_ONCE(x);
then AFAICS it still meets the "compiler need only forget the contents
of the indicated memory located" requirement that you listed, right?
[ I have a good excuse for asking this: after a long day my IQ dropped
by 50 points and all that! :-) ]
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 18:28 ` Ingo Molnar
@ 2013-12-10 19:01 ` Paul E. McKenney
2013-12-10 19:46 ` Ingo Molnar
0 siblings, 1 reply; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-10 19:01 UTC (permalink / raw)
To: Ingo Molnar
Cc: Jonathan Corbet, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw, Oleg Nesterov, Rusty Russell
On Tue, Dec 10, 2013 at 07:28:00PM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > > +for barrier() that affects only the specific accesses flagged by the
> > > +ACCESS_ONCE().
> > >
> > > Does not seem to be obvious enough to me - does it affect accesses
> > > to the variables referenced (but still allows accesses to separate
> > > variables reordered), or does it affect compiler-ordering of all
> > > ACCESS_ONCE() instances, instructing the compiler to preserve
> > > program order?
> >
> > I cover this in the bullet item about reordering memory accesses:
> >
> > (*) The compiler is within its rights to reorder memory accesses unless
> > you tell it not to. For example, consider the following interaction
> > between process-level code and an interrupt handler:
> >
> > void process_level(void)
> > {
> > msg = get_message();
> > flag = true;
> > }
> >
> > void interrupt_handler(void)
> > {
> > if (flag)
> > process_message(msg);
> > }
> >
> > There is nothing to prevent the the compiler from transforming
> > process_level() to the following, in fact, this might well be a
> > win for single-threaded code:
> >
> > void process_level(void)
> > {
> > flag = true;
> > msg = get_message();
> > }
> >
> > If the interrupt occurs between these two statement, then
> > interrupt_handler() might be passed a garbled msg. Use ACCESS_ONCE()
> > to prevent this as follows:
> >
> > void process_level(void)
> > {
> > ACCESS_ONCE(msg) = get_message();
> > ACCESS_ONCE(flag) = true;
> > }
> >
> > void interrupt_handler(void)
> > {
> > if (ACCESS_ONCE(flag))
> > process_message(ACCESS_ONCE(msg));
> > }
> >
> > Note that the ACCESS_ONCE() wrappers in interrupt_handler()
> > are needed if this interrupt handler can itself be interrupted
> > by something that also accesses 'flag' and 'msg', for example,
> > a nested interrupt or an NMI. Otherwise, ACCESS_ONCE() is not
> > needed in interrupt_handler() other than for documentation purposes.
> > (Note also that nested interrupts do not typically occur in modern
> > Linux kernels, in fact, if an interrupt handler returns with
> > interrupts enabled, you will get a WARN_ONCE() splat.)
> >
> > This effect could also be achieved using barrier(), but ACCESS_ONCE()
> > is more selective: With ACCESS_ONCE(), the compiler need only forget
> > the contents of the indicated memory located, while with barrier()
> > the compiler must discard the value of all memory locations that
> > it has currented cached in any machine registers.
> >
> > Does that cover it?
>
> btw.:
>
> s/indicated memory located/
> indicated memory location
>
> ?
Good catch, fixed!
> So, what I don't see this statement cover (and I might be dense about
> it!) is whether two ACCESS_ONCE() macros referring to different
> variables are allowed to be reordered with each other.
>
> If the compiler reorders:
>
> ACCESS_ONCE(x);
> ACCESS_ONCE(y);
>
> to:
>
> ACCESS_ONCE(y);
> ACCESS_ONCE(x);
>
> then AFAICS it still meets the "compiler need only forget the contents
> of the indicated memory located" requirement that you listed, right?
True, but if the compiler was willing to reorder ACCESS_ONCE()'s volatile
accesses, it would be really hard to write reliable device drivers. The
standard says the following:
Access to volatile objects are evaluated strictly according to
the rules of the abstract machine.
That said, compiler writers and standards wonks will argue endlessly about
exactly what that does and does not mean. :-/
I added a sentence reading:
Of course, the compiler must also respect the order in which
the ACCESS_ONCE()s occur, though the CPU of course need not do so.
To the end of that paragraph. Does that help?
> [ I have a good excuse for asking this: after a long day my IQ dropped
> by 50 points and all that! :-) ]
I know that feeling! And I cannot resist replying: "Just wait until you
reach your mid-50s!" ;-)
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 19:01 ` Paul E. McKenney
@ 2013-12-10 19:46 ` Ingo Molnar
2013-12-10 20:09 ` Paul E. McKenney
0 siblings, 1 reply; 33+ messages in thread
From: Ingo Molnar @ 2013-12-10 19:46 UTC (permalink / raw)
To: Paul E. McKenney
Cc: Jonathan Corbet, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw, Oleg Nesterov, Rusty Russell
* Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
> > So, what I don't see this statement cover (and I might be dense about
> > it!) is whether two ACCESS_ONCE() macros referring to different
> > variables are allowed to be reordered with each other.
> >
> > If the compiler reorders:
> >
> > ACCESS_ONCE(x);
> > ACCESS_ONCE(y);
> >
> > to:
> >
> > ACCESS_ONCE(y);
> > ACCESS_ONCE(x);
> >
> > then AFAICS it still meets the "compiler need only forget the contents
> > of the indicated memory located" requirement that you listed, right?
>
> True, but if the compiler was willing to reorder ACCESS_ONCE()'s
> volatile accesses, it would be really hard to write reliable device
> drivers. [...]
But nowhere do we link ACCESS_ONCE() to 'volatile' semantics in the
document, do we? (and I'm not sure we should.)
[ In theory a future compiler could offer a smarter, more flexible
'compiler barrier' implementation - at which point we might be
tempted to use that new facility to implement ACCESS_ONCE(). At that
point this ambiguity might arise. ]
> [...] The standard says the following:
>
> Access to volatile objects are evaluated strictly according to
> the rules of the abstract machine.
>
> That said, compiler writers and standards wonks will argue endlessly
> about exactly what that does and does not mean. :-/
>
> I added a sentence reading:
>
> Of course, the compiler must also respect the order in which
> the ACCESS_ONCE()s occur, though the CPU of course need not do so.
>
> To the end of that paragraph. Does that help?
Yeah, that looks perfect!
Thanks,
Ingo
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE()
2013-12-10 19:46 ` Ingo Molnar
@ 2013-12-10 20:09 ` Paul E. McKenney
0 siblings, 0 replies; 33+ messages in thread
From: Paul E. McKenney @ 2013-12-10 20:09 UTC (permalink / raw)
To: Ingo Molnar
Cc: Jonathan Corbet, linux-kernel, laijs, dipankar, akpm,
mathieu.desnoyers, josh, niv, tglx, peterz, rostedt, dhowells,
edumazet, darren, fweisbec, sbw, Oleg Nesterov, Rusty Russell
On Tue, Dec 10, 2013 at 08:46:28PM +0100, Ingo Molnar wrote:
>
> * Paul E. McKenney <paulmck@linux.vnet.ibm.com> wrote:
>
> > > So, what I don't see this statement cover (and I might be dense about
> > > it!) is whether two ACCESS_ONCE() macros referring to different
> > > variables are allowed to be reordered with each other.
> > >
> > > If the compiler reorders:
> > >
> > > ACCESS_ONCE(x);
> > > ACCESS_ONCE(y);
> > >
> > > to:
> > >
> > > ACCESS_ONCE(y);
> > > ACCESS_ONCE(x);
> > >
> > > then AFAICS it still meets the "compiler need only forget the contents
> > > of the indicated memory located" requirement that you listed, right?
> >
> > True, but if the compiler was willing to reorder ACCESS_ONCE()'s
> > volatile accesses, it would be really hard to write reliable device
> > drivers. [...]
>
> But nowhere do we link ACCESS_ONCE() to 'volatile' semantics in the
> document, do we? (and I'm not sure we should.)
Agreed, stating ACCESS_ONCE()'s semantics separately is better.
> [ In theory a future compiler could offer a smarter, more flexible
> 'compiler barrier' implementation - at which point we might be
> tempted to use that new facility to implement ACCESS_ONCE(). At that
> point this ambiguity might arise. ]
And they are trying to obsolete volatile, but haven't quite got there
yet. ;-)
> > [...] The standard says the following:
> >
> > Access to volatile objects are evaluated strictly according to
> > the rules of the abstract machine.
> >
> > That said, compiler writers and standards wonks will argue endlessly
> > about exactly what that does and does not mean. :-/
> >
> > I added a sentence reading:
> >
> > Of course, the compiler must also respect the order in which
> > the ACCESS_ONCE()s occur, though the CPU of course need not do so.
> >
> > To the end of that paragraph. Does that help?
>
> Yeah, that looks perfect!
Very good!
Thanx, Paul
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2013-12-10 20:09 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-12-04 22:46 [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 1/4] Documentation/memory-barriers.txt: Add needed ACCESS_ONCE() calls to memory-barriers.txt Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 2/4] Documentation/memory-barriers.txt: Add long atomic examples " Paul E. McKenney
2013-12-04 22:46 ` [PATCH tip/core/locking 4/4] Documentation/memory-barriers.txt: Document ACCESS_ONCE() Paul E. McKenney
2013-12-05 9:33 ` Ingo Molnar
2013-12-05 9:52 ` Mathieu Desnoyers
2013-12-05 10:11 ` Ingo Molnar
2013-12-05 18:02 ` Paul E. McKenney
2013-12-10 13:24 ` Ingo Molnar
2013-12-10 17:36 ` Paul E. McKenney
2013-12-05 9:50 ` Ingo Molnar
2013-12-05 18:05 ` Paul E. McKenney
2013-12-05 22:47 ` Paul E. McKenney
2013-12-10 15:10 ` Ingo Molnar
2013-12-10 17:37 ` Paul E. McKenney
2013-12-05 20:21 ` Jonathan Corbet
2013-12-05 21:44 ` Paul E. McKenney
2013-12-10 15:20 ` Ingo Molnar
2013-12-10 17:44 ` Paul E. McKenney
2013-12-10 18:28 ` Ingo Molnar
2013-12-10 19:01 ` Paul E. McKenney
2013-12-10 19:46 ` Ingo Molnar
2013-12-10 20:09 ` Paul E. McKenney
2013-12-05 0:10 ` [PATCH v4 tip/core/locking 0/4] Memory-barrier documentation updates Josh Triplett
2013-12-05 10:59 ` Henrik Austad
2013-12-05 12:28 ` Ingo Molnar
2013-12-05 13:51 ` Steven Rostedt
2013-12-05 18:05 ` David Miller
2013-12-05 18:18 ` Paul E. McKenney
2013-12-05 18:44 ` David Miller
2013-12-05 19:01 ` Paul E. McKenney
2013-12-10 15:24 ` Ingo Molnar
2013-12-05 12:29 ` [PATCH v4 tip/core/locking 3/4] Documentation/memory-barriers.txt: Prohibit speculative writes Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).