[PATCH] Immunize rcu_dereference() against crazy compiler writers

[PATCH] Immunize rcu_dereference() against crazy compiler writers

[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 <paulmck@linux.vnet.ibm.com>
---

 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
--- linux-2.6.22/include/linux/rcupdate.h	2007-07-08 16:32:17.000000000 -0700
+++ linux-2.6.22-volrcud/include/linux/rcupdate.h	2007-07-11 17:21:09.000000000 -0700
@@ -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); \
 				})

[PATCH] Remove workaround for unimmunized rcu_dereference from mce_log()

[Paul E. McKenney]

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

Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
---

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

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
--- linux-2.6.22-volrcud/arch/x86_64/kernel/mce.c	2007-07-08 16:32:17.000000000 -0700
+++ linux-2.6.22-volrcud-mcefix/arch/x86_64/kernel/mce.c	2007-07-11 17:22:06.000000000 -0700
@@ -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. */

Re: [PATCH] Immunize rcu_dereference() against crazy compiler writers

[Andi Kleen]

"Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:

> 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

Re: [PATCH] Immunize rcu_dereference() against crazy compiler writers

[Paul E. McKenney]

On Thu, Jul 12, 2007 at 04:03:19PM +0200, Andi Kleen wrote:
> "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> writes:
> 
> > 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.

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.

> 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

Re: [PATCH] Immunize rcu_dereference() against crazy compiler writers

[Josh Triplett]

On Wed, 2007-07-11 at 18:00 -0700, Paul E. McKenney wrote:
> 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 <paulmck@linux.vnet.ibm.com>
Acked-by: Josh Triplett <josh@kernel.org>

- Josh Triplett

Re: [PATCH] Immunize rcu_dereference() against crazy compiler writers

[Andrew Morton]

On Wed, 11 Jul 2007 18:00:58 -0700 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:

> 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 <paulmck@linux.vnet.ibm.com>
> ---
> 
>  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
> --- linux-2.6.22/include/linux/rcupdate.h	2007-07-08 16:32:17.000000000 -0700
> +++ linux-2.6.22-volrcud/include/linux/rcupdate.h	2007-07-11 17:21:09.000000000 -0700
> @@ -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))

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

Re: [PATCH] Immunize rcu_dereference() against crazy compiler writers

[Paul E. McKenney]

On Tue, Jul 17, 2007 at 02:46:00AM -0700, Andrew Morton wrote:
> On Wed, 11 Jul 2007 18:00:58 -0700 "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> wrote:
> 
> > 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 <paulmck@linux.vnet.ibm.com>
> > ---
> > 
> >  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
> > --- linux-2.6.22/include/linux/rcupdate.h	2007-07-08 16:32:17.000000000 -0700
> > +++ linux-2.6.22-volrcud/include/linux/rcupdate.h	2007-07-11 17:21:09.000000000 -0700
> > @@ -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))
> 
> 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 <paulmck@linux.vnet.ibm.com>
---

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

diff -urpNa -X dontdiff linux-2.6.22/drivers/net/hamradio/bpqether.c linux-2.6.22-volrcud-hamfix/drivers/net/hamradio/bpqether.c
--- linux-2.6.22/drivers/net/hamradio/bpqether.c	2007-07-08 16:32:17.000000000 -0700
+++ linux-2.6.22-volrcud-hamfix/drivers/net/hamradio/bpqether.c	2007-07-17 06:46:16.000000000 -0700
@@ -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)