Immunize rcu_dereference() against crazy compiler writers

PATCH In review Paul E. McKenney 4 participants 7 messages Jul 12, 2007 – Jul 17, 2007

Subsystems (2)

Network drivers RCU

lore

Share & tools

lore

Paul E. McKenney

author

2007-07-12 01:01 UTC #0 lore raw

PATCH 1/1 Immunize rcu_dereference() against crazy compiler writers

Turns out that compiler writers are a bit more aggressive about optimizing than one might expect. This patch prevents a number of such optimizations from messing up rcu_deference(). This is not merely a theoretical problem, as evidenced by the rmb() in mce_log().

Signed-off-by Paul E. McKenney <[email protected]>

---

 rcupdate.h |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

include/linux/rcupdate.h +13 -1

diff -urpNa -X dontdiff linux-2.6.22/include/linux/rcupdate.h linux-2.6.22-volrcud/include/linux/rcupdate.h
@@ -217,6 +217,18 @@ extern int rcu_needs_cpu(int cpu);
 		local_bh_enable(); \
 	} while(0)
 
+/*
+ * Prevent the compiler from merging or refetching accesses.  The compiler
+ * is also forbidden from reordering successive instances of ACCESS_ONCE(),
+ * but only when the compiler is aware of some particular ordering.  One way
+ * to make the compiler aware of ordering is to put the two invocations of 
+ * ACCESS_ONCE() in different C statements.
+ *
+ * This macro does absolutely -nothing- to prevent the CPU from reordering,
+ * merging, or refetching absolutely anything at any time.
+ */
+#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))
+
 /**
  * rcu_dereference - fetch an RCU-protected pointer in an
  * RCU read-side critical section.  This pointer may later

@@ -228,7 +240,7 @@ extern int rcu_needs_cpu(int cpu);
  */
 
 #define rcu_dereference(p)     ({ \
-				typeof(p) _________p1 = p; \
+				typeof(p) _________p1 = ACCESS_ONCE(p); \
 				smp_read_barrier_depends(); \
 				(_________p1); \
 				})

Paul E. McKenney

author

2007-07-12 01:03 UTC #1 lore raw

PATCH 1/1 Remove workaround for unimmunized rcu_dereference from mce_log()

Remove the rmb() from mce_log(), since the immunized version of rcu_dereference() makes it unnecessary.

Signed-off-by Paul E. McKenney <[email protected]>

---

 mce.c |    3 ---
 1 file changed, 3 deletions(-)

arch/x86_64/kernel/mce.c +0 -3

diff -urpNa -X dontdiff linux-2.6.22-volrcud/arch/x86_64/kernel/mce.c linux-2.6.22-volrcud-mcefix/arch/x86_64/kernel/mce.c
@@ -67,9 +67,6 @@ void mce_log(struct mce *mce)
 	wmb();
 	for (;;) {
 		entry = rcu_dereference(mcelog.next);
-		/* The rmb forces the compiler to reload next in each
-		    iteration */
-		rmb();
 		for (;;) {
 			/* When the buffer fills up discard new entries. Assume
 			   that the earlier errors are the more interesting. */

Andi Kleen

2007-07-12 13:08 UTC #2 lore raw

In reply to Paul E. McKenney

Quoted 4 lines · Paul E. McKenney

Turns out that compiler writers are a bit more aggressive about optimizing than one might expect. This patch prevents a number of such optimizations from messing up rcu_deference(). This is not merely a theoretical problem, as evidenced by the rmb() in mce_log().

Don't think that's an improvement. rmb() at least is known to work reliable to prevent such reordering. Memory barriers are well documented in the gcc documentation. Who knows about volatile? The volatile semantics have been traditionally unclear and shakey. The C standard doesn't make much guarantees and i don't think gcc does either.

If anything you might want to embedd rmb(); in a statement expression in rcu_deference instead.

-Andi

Paul E. McKenney

author

2007-07-12 14:49 UTC #3 lore raw

Quoted 7 lines · Andi Kleen

Quoted 4 lines · Paul E. McKenney
Turns out that compiler writers are a bit more aggressive about optimizing than one might expect. This patch prevents a number of such optimizations from messing up rcu_deference(). This is not merely a theoretical problem, as evidenced by the rmb() in mce_log().

Quoted 6 lines

Don't think that's an improvement. rmb() at least is known to work reliable to prevent such reordering. Memory barriers are well documented in the gcc documentation. Who knows about volatile? The volatile semantics have been traditionally unclear and shakey. The C standard doesn't make much guarantees and i don't think gcc does either.

Hello, Andi!

The rcu_dereference() primitive does not need many guarantees, and the ones that it does need are well within those specified in the C standard.

As I understand it, the compiler is not permitted to move volatile accesses past "sequence points". Sequence points include ends of expression statements; control expressions in "if", "switch", "while", and "do" statements; each of the three control expressions in the "for" statement; return statement expressions; and initializers.

See http://www.open-std.org/jtc1/sc22/wg14/www/C99RationaleV5.10.pdf
(PDF page 24, virtual page 18) or any of a number of C texts.

So if gcc adheres to C99, the fetch implied by the new version of rcu_dereference() in mce_log() must be emitted at the beginning of the outermost "for (;;)" loop.

Now it might be that mce_log() also needs the rmb() in order to force the -CPU- to actually -execute- the fetch in order -- and if that is the case, I will happy to update the patch to leave the rmb() but to update the comment, which currently only talks about the compiler messing things up:

"The rmb forces the compiler to reload next in each interation"

However, my quick look through the code convinced me that if rcu_dereference() was guaranteed to force the compiler to emit the load at the top of the loop, that reorderings by the CPU were harmless given the other barriers in that function. But I might well have missed something.

Quoted 2 lines

If anything you might want to embedd rmb(); in a statement expression in rcu_deference instead.

No way am I putting an rmb() or even an smp_rmb(), into rcu_dereference()!!!

Thanx, Paul

Josh Triplett

Acked-by on v1 1/1

2007-07-12 16:21 UTC #4 lore raw

In reply to Paul E. McKenney

Quoted 6 lines · Paul E. McKenney

Turns out that compiler writers are a bit more aggressive about optimizing than one might expect. This patch prevents a number of such optimizations from messing up rcu_deference(). This is not merely a theoretical problem, as evidenced by the rmb() in mce_log().
Signed-off-by Paul E. McKenney <[email protected]>

Acked-by Josh Triplett <[email protected]>

- Josh Triplett

Andrew Morton

2007-07-17 09:46 UTC #5 lore raw

In reply to Paul E. McKenney

Quoted 19 lines · On Wed, 11 Jul 2007 18:00:58 -0700 "Paul E. McKenney"…

Turns out that compiler writers are a bit more aggressive about optimizing than one might expect. This patch prevents a number of such optimizations from messing up rcu_deference(). This is not merely a theoretical problem, as evidenced by the rmb() in mce_log().
Signed-off-by Paul E. McKenney <[email protected]>
---
 rcupdate.h |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
diff -urpNa -X dontdiff linux-2.6.22/include/linux/rcupdate.h linux-2.6.22-volrcud/include/linux/rcupdate.h
include/linux/rcupdate.h +1 -0
@@ -217,6 +217,18 @@ extern int rcu_needs_cpu(int cpu);
 		local_bh_enable(); \
 	} while(0)
 
+/*

include/linux/rcupdate.h extern int rcu_needs_cpu(int cpu); · line 230

Quoted 10 lines

include/linux/rcupdate.h +10 -0

+ * Prevent the compiler from merging or refetching accesses.  The compiler
+ * is also forbidden from reordering successive instances of ACCESS_ONCE(),
+ * but only when the compiler is aware of some particular ordering.  One way
+ * to make the compiler aware of ordering is to put the two invocations of 
+ * ACCESS_ONCE() in different C statements.
+ *
+ * This macro does absolutely -nothing- to prevent the CPU from reordering,
+ * merging, or refetching absolutely anything at any time.
+ */
+#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))

drivers/net/hamradio/bpqether.c: In function 'bpq_seq_next':
drivers/net/hamradio/bpqether.c:421: error: invalid lvalue in unary '&'

Paul E. McKenney

author

2007-07-17 13:53 UTC #6 lore raw

Quoted 20 lines · Andrew Morton

Quoted 18 lines · On Wed, 11 Jul 2007 18:00:58 -0700 "Paul E. McKenney"…
Turns out that compiler writers are a bit more aggressive about optimizing than one might expect. This patch prevents a number of such optimizations from messing up rcu_deference(). This is not merely a theoretical problem, as evidenced by the rmb() in mce_log().
Signed-off-by Paul E. McKenney <[email protected]>
---
 rcupdate.h |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)
diff -urpNa -X dontdiff linux-2.6.22/include/linux/rcupdate.h linux-2.6.22-volrcud/include/linux/rcupdate.h
include/linux/rcupdate.h +0 -0
@@ -217,6 +217,18 @@ extern int rcu_needs_cpu(int cpu);
 		local_bh_enable(); \
 	} while(0)
 

Quoted 14 lines

include/linux/rcupdate.h extern int rcu_needs_cpu(int cpu); · line 230

Quoted 11 lines

include/linux/rcupdate.h +11 -0

+/*
+ * Prevent the compiler from merging or refetching accesses.  The compiler
+ * is also forbidden from reordering successive instances of ACCESS_ONCE(),
+ * but only when the compiler is aware of some particular ordering.  One way
+ * to make the compiler aware of ordering is to put the two invocations of 
+ * ACCESS_ONCE() in different C statements.
+ *
+ * This macro does absolutely -nothing- to prevent the CPU from reordering,
+ * merging, or refetching absolutely anything at any time.
+ */
+#define ACCESS_ONCE(x) (*(volatile typeof(x) *)&(x))

drivers/net/hamradio/bpqether.c: In function 'bpq_seq_next':
drivers/net/hamradio/bpqether.c:421: error: invalid lvalue in unary '&'

This is a bug in bpqether.c, and here is a patch that compiles. I don't have the necessary hardware to test. As you might expect, I assert that the fact that the new implementation of rcu_dereference() detects improper use on rvalues is a feature. ;-)

Signed-off-by Paul E. McKenney <[email protected]>

---

 bpqether.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

drivers/net/hamradio/bpqether.c +3 -3

diff -urpNa -X dontdiff linux-2.6.22/drivers/net/hamradio/bpqether.c linux-2.6.22-volrcud-hamfix/drivers/net/hamradio/bpqether.c
@@ -413,12 +413,12 @@ static void *bpq_seq_next(struct seq_fil
 	++*pos;
 
 	if (v == SEQ_START_TOKEN)
-		p = bpq_devices.next;
+		p = rcu_dereference(bpq_devices.next);
 	else
-		p = ((struct bpqdev *)v)->bpq_list.next;
+		p = rcu_dereference(((struct bpqdev *)v)->bpq_list.next);
 
 	return (p == &bpq_devices) ? NULL 
-		: rcu_dereference(list_entry(p, struct bpqdev, bpq_list));
+		: list_entry(p, struct bpqdev, bpq_list);
 }
 
 static void bpq_seq_stop(struct seq_file *seq, void *v)