Paul E. McKenney wrote:
On Tue, Jan 15, 2013 at 02:30:32PM +0400, Konstantin Khlebnikov wrote:
Documentation/atomic_ops.txt (182dd4b277177e8465ad11cd9f85f282946b5578)
says that pointers, longs, ints, and chars are stored and loaded atomically.
But GCC actually may split assignment to 'long' variable into two instructions.
see example in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981
GCC also splits assignments to 'volatile' variables and this is actually a bug
in gcc.
volatile unsigned long y;
y = 0x100000001ul;
400728: c7 05 66 06 20 00 01 movl $0x1,0x200666(%rip) #
600d98<y>
40072f: 00 00 00
400732: c7 05 60 06 20 00 01 movl $0x1,0x200660(%rip) #
600d9c<y+0x4>
400739: 00 00 00
fortunately for y = 0; it generates this:
40071d: 48 c7 05 70 06 20 00 movq $0x0,0x200670(%rip) #
600d98<y>
400724: 00 00 00 00
Thus NULL is safe, but constant ERR_PTR may be dangerous.
Probably rcu_assign_pointer() should use ACCESS_ONCE() around lvalue, because
splitting assignment for non-volatile variable seems like completely valid,
but this may help only after fixing that bug in GCC.
Good catch! I has queued the following patch.
Thanx, Paul
------------------------------------------------------------------------
rcu: Add ACCESS_ONCE() to rcu_assign_pointer()
GCC may split assignment to 'long' variable into two instructions:
volatile unsigned long y;
y = 0x100000001ul;
movl $0x1,0x200666(%rip)
movl $0x1,0x200660(%rip)
This commit fixes this by applying ACCESS_ONCE() within
__rcu_assign_pointer(), but note that some versions and architectures
of GCC have a bug that defeats ACCESS_ONCE():
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=55981
I added a comment to this bug report asking that the bug be fixed for
volatiles as well as atomics, citing a device driver storing a constant
into a 64-bit device register as motivation.
Reported-by: Konstantin Khlebnikov<khlebni...@openvz.org>
Signed-off-by: Paul E. McKenney<paul...@linux.vnet.ibm.com>
diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 9ed2c9a..3435174 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -556,7 +556,7 @@ static inline void rcu_preempt_sleep_check(void)
#define __rcu_assign_pointer(p, v, space) \
do { \
smp_wmb(); \
- (p) = (typeof(*v) __force space *)(v); \
+ ACCESS_ONCE(p) = (typeof(*v) __force space *)(v); \
} while (0)
Seems like RCU_INIT_POINTER() need this too.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
