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(-)
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -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; \
On Thu, Oct 11, 2018 at 8:02 AM Eric Dumazet <[email protected]> wrote:
>
> On Thu, Oct 11, 2018 at 3:45 AM Peter Zijlstra <[email protected]> wrote:
> >
> > 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]>
>
>
> 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())
On Thu, Oct 11, 2018 at 08:24:49AM -0700, Eric Dumazet wrote:
> On Thu, Oct 11, 2018 at 8:02 AM Eric Dumazet <[email protected]> wrote:
> >
> > On Thu, Oct 11, 2018 at 3:45 AM Peter Zijlstra <[email protected]> wrote:
> > >
> > > 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]>
> >
> >
> > 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.
On Thu, Oct 11, 2018 at 8:50 AM Peter Zijlstra <[email protected]> wrote:
> 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.
Yeah I guess all stable versions would need a fix anyway, so this tag
would suffice,
even if bug was older.
On Thu, Oct 11, 2018 at 3:45 AM Peter Zijlstra <[email protected]> wrote:
>
> 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]>
Acked-by: Eric Dumazet <[email protected]>
> ---
> arch/x86/include/asm/percpu.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> --- a/arch/x86/include/asm/percpu.h
> +++ b/arch/x86/include/asm/percpu.h
> @@ -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; \
>
>
Commit-ID: b59167ac7bafd804c91e49ad53c6d33a7394d4c8
Gitweb: https://git.kernel.org/tip/b59167ac7bafd804c91e49ad53c6d33a7394d4c8
Author: Peter Zijlstra <[email protected]>
AuthorDate: Thu, 11 Oct 2018 12:38:27 +0200
Committer: Thomas Gleixner <[email protected]>
CommitDate: Sun, 14 Oct 2018 11:11:22 +0200
x86/percpu: Fix this_cpu_read()
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: 7c3576d261ce ("[PATCH] i386: Convert PDA into the percpu section")
Reported-by: Eric Dumazet <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Acked-by: Eric Dumazet <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/include/asm/percpu.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h
index e9202a0de8f0..1a19d11cfbbd 100644
--- a/arch/x86/include/asm/percpu.h
+++ b/arch/x86/include/asm/percpu.h
@@ -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; \