x86/tsc: Fix native_sched_clock()

Eric reported that a sequence count loop using this_cpu_read() got optimized out. This is wrong, this_cpu_read() must imply READ_ONCE() because the interface is IRQ-safe, therefore an interrupt can have changed the per-cpu value.

Fixes 59eaef78bfea ("x86/tsc: Remodel cyc2ns to use seqcount_latch()")

Reported-by Eric Dumazet <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/x86/include/asm/percpu.h |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

arch/x86/include/asm/percpu.h +4 -4

@@ -185,22 +185,22 @@ do {                                                                      \
        typeof(var) pfo_ret__;                          \
        switch (sizeof(var)) {                          \
        case 1:                                         \
-               asm(op "b "__percpu_arg(1)",%0"         \
+               asm volatile(op "b "__percpu_arg(1)",%0"\
                    : "=q" (pfo_ret__)                  \
                    : "m" (var));                       \
                break;                                  \
        case 2:                                         \
-               asm(op "w "__percpu_arg(1)",%0"         \
+               asm volatile(op "w "__percpu_arg(1)",%0"\
                    : "=r" (pfo_ret__)                  \
                    : "m" (var));                       \
                break;                                  \
        case 4:                                         \
-               asm(op "l "__percpu_arg(1)",%0"         \
+               asm volatile(op "l "__percpu_arg(1)",%0"\
                    : "=r" (pfo_ret__)                  \
                    : "m" (var));                       \
                break;                                  \
        case 8:                                         \
-               asm(op "q "__percpu_arg(1)",%0"         \
+               asm volatile(op "q "__percpu_arg(1)",%0"\
                    : "=r" (pfo_ret__)                  \
                    : "m" (var));                       \
                break;                                  \

Quoted 6 lines · Peter Zijlstra

Eric reported that a sequence count loop using this_cpu_read() got optimized out. This is wrong, this_cpu_read() must imply READ_ONCE() because the interface is IRQ-safe, therefore an interrupt can have changed the per-cpu value.

Quoted 3 lines

Fixes 59eaef78bfea ("x86/tsc: Remodel cyc2ns to use seqcount_latch()")

Reported-by Eric Dumazet <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Eric Dumazet <[email protected]>

Quoted 8 lines · Eric Dumazet

Quoted 6 lines · Peter Zijlstra

Eric reported that a sequence count loop using this_cpu_read() got optimized out. This is wrong, this_cpu_read() must imply READ_ONCE() because the interface is IRQ-safe, therefore an interrupt can have changed the per-cpu value.

Quoted 6 lines

Quoted 3 lines

Fixes 59eaef78bfea ("x86/tsc: Remodel cyc2ns to use seqcount_latch()")

Reported-by Eric Dumazet <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Acked-by Eric Dumazet <[email protected]>

Actually the Fixes: tag seems funky.

Bug was not added by 59eaef78bfea

Your patch probably needs to be backported to older versions of linux, just to be safe, since
we might have other places where authors relied on this_cpu_read() semantic (different than this_cpu_read_stable())

Right; it goes back a long long way... is:

7c3576d261ce ("[PATCH] i386: Convert PDA into the percpu section")

early enough? That introduces percpu_from_op(), but arguably the pda_from_op() it replaces was buggy already.