On 22/06/2016 23:55, Rik van Riel wrote:
> > > + hardirq_time = READ_ONCE(per_cpu(cpu_hardirq_time,
> > > cpu));
> > Which makes this per_cpu(,cpu) usage somewhat curious. What's wrong
> > with __this_cpu_read() ?
>
> I played around with it a bit, and it seems that
> __this_cpu_read does not want to nest inside
> READ_ONCE. Nobody else seems to be doing that,
> either.
According to arch/x86/include/asm/percpu.h, this_cpu_read always has
READ_ONCE semantics, but I cannot find that in include/asm-generic
/percpu.h. It probably just works because of all the layers of goo, but
something like this (101% untested) would make me feel safer:
diff --git a/include/asm-generic/percpu.h b/include/asm-generic/percpu.h
index 4d9f233c4ba8..d057568f1926 100644
--- a/include/asm-generic/percpu.h
+++ b/include/asm-generic/percpu.h
@@ -67,7 +67,7 @@ extern void setup_per_cpu_areas(void);
#define raw_cpu_generic_to_op(pcp, val, op) \
do { \
- *raw_cpu_ptr(&(pcp)) op val; \
+ ACCESS_ONCE(*raw_cpu_ptr(&(pcp))) op val; \
} while (0)
#define raw_cpu_generic_add_return(pcp, val) \
@@ -109,7 +109,7 @@ do {
({ \
typeof(pcp) __ret; \
preempt_disable(); \
- __ret = *this_cpu_ptr(&(pcp)); \
+ __ret = READ_ONCE(this_cpu_ptr(&(pcp))); \
preempt_enable(); \
__ret; \
})
@@ -118,7 +118,7 @@ do {
do { \
unsigned long __flags; \
raw_local_irq_save(__flags); \
- *raw_cpu_ptr(&(pcp)) op val; \
+ ACCESS_ONCE(*raw_cpu_ptr(&(pcp))) op val; \
raw_local_irq_restore(__flags); \
} while (0)
@@ -168,16 +168,16 @@ do {
})
#ifndef raw_cpu_read_1
-#define raw_cpu_read_1(pcp) (*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_1(pcp) READ_ONCE(raw_cpu_ptr(&(pcp)))
#endif
#ifndef raw_cpu_read_2
-#define raw_cpu_read_2(pcp) (*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_2(pcp) READ_ONCE(raw_cpu_ptr(&(pcp)))
#endif
#ifndef raw_cpu_read_4
-#define raw_cpu_read_4(pcp) (*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_4(pcp) READ_ONCE(raw_cpu_ptr(&(pcp)))
#endif
#ifndef raw_cpu_read_8
-#define raw_cpu_read_8(pcp) (*raw_cpu_ptr(&(pcp)))
+#define raw_cpu_read_8(pcp) READ_ONCE(raw_cpu_ptr(&(pcp)))
#endif
#ifndef raw_cpu_write_1
> Back to READ_ONCE(per_cpu(,cpu)) it is...
What about READ_ONCE(this_cpu_ptr())?
Paolo
