* [PATCH 0/2] x86/tsc: Fix native_sched_clock()
@ 2018-10-11 10:38 Peter Zijlstra
2018-10-11 10:38 ` [PATCH 1/2] x86/tsc: Force inlining of cyc2ns bits Peter Zijlstra
2018-10-11 10:38 ` [PATCH 2/2] x86/percpu: Fix this_cpu_read() Peter Zijlstra
0 siblings, 2 replies; 9+ messages in thread
From: Peter Zijlstra @ 2018-10-11 10:38 UTC (permalink / raw)
To: mingo, tglx; +Cc: linux-kernel, hpa, edumazet, eric.dumazet, bp, peterz
Eric reported that native_sched_clock() gets miscompiled; we loose a seqcount
loop. These patches cure that.
After these patches:
native_sched_clock:
pushq %rbp #
movq %rsp, %rbp #,
... jump label ...
rdtsc
salq $32, %rdx #, tmp110
orq %rax, %rdx # low, tmp110
movq %rdx, %r10 # tmp110, _23
movq $cyc2ns, %r9 #, tmp136
.L235:
movl %gs:cyc2ns+32(%rip),%eax # cyc2ns.seq.sequence, pfo_ret__
movl %eax, %ecx # pfo_ret__, idx
andl $1, %ecx #, idx
salq $4, %rcx #, tmp116
addq %r9, %rcx # tmp136, tmp117
movq %gs:8(%rcx),%rdi # cyc2ns.data[idx_14].cyc2ns_offset, pfo_ret__
movl %gs:(%rcx),%esi # cyc2ns.data[idx_14].cyc2ns_mul, pfo_ret__
movl %gs:4(%rcx),%ecx # cyc2ns.data[idx_14].cyc2ns_shift, pfo_ret__
movl %gs:cyc2ns+32(%rip),%r8d # cyc2ns.seq.sequence, pfo_ret__
cmpl %r8d, %eax # pfo_ret__, pfo_ret__
jne .L235 #,
movl %esi, %esi # pfo_ret__, pfo_ret__
movq %rsi, %rax # pfo_ret__, tmp133
mulq %r10 # _23
shrdq %rdx, %rax # pfo_ret__,, tmp134
shrq %cl, %rdx # pfo_ret__,
testb $64, %cl #, pfo_ret__
cmovne %rdx, %rax #,, tmp134
addq %rdi, %rax # pfo_ret__, <retval>
popq %rbp #
ret
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] x86/tsc: Force inlining of cyc2ns bits
2018-10-11 10:38 [PATCH 0/2] x86/tsc: Fix native_sched_clock() Peter Zijlstra
@ 2018-10-11 10:38 ` Peter Zijlstra
2018-10-14 9:15 ` [tip:x86/urgent] " tip-bot for Peter Zijlstra
2018-10-11 10:38 ` [PATCH 2/2] x86/percpu: Fix this_cpu_read() Peter Zijlstra
1 sibling, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2018-10-11 10:38 UTC (permalink / raw)
To: mingo, tglx; +Cc: linux-kernel, hpa, edumazet, eric.dumazet, bp, peterz
Looking at the asm for native_sched_clock() I noticed we don't inline
enough. Mostly caused by sharing code with cyc2ns_read_begin(), which
we didn't used to do. So mark all that __force_inline to make it DTRT.
Fixes: 59eaef78bfea ("x86/tsc: Remodel cyc2ns to use seqcount_latch()")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
arch/x86/kernel/tsc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -57,7 +57,7 @@ struct cyc2ns {
static DEFINE_PER_CPU_ALIGNED(struct cyc2ns, cyc2ns);
-void cyc2ns_read_begin(struct cyc2ns_data *data)
+void __always_inline cyc2ns_read_begin(struct cyc2ns_data *data)
{
int seq, idx;
@@ -74,7 +74,7 @@ void cyc2ns_read_begin(struct cyc2ns_dat
} while (unlikely(seq != this_cpu_read(cyc2ns.seq.sequence)));
}
-void cyc2ns_read_end(void)
+void __always_inline cyc2ns_read_end(void)
{
preempt_enable_notrace();
}
@@ -103,7 +103,7 @@ void cyc2ns_read_end(void)
* -johnstul@us.ibm.com "math is hard, lets go shopping!"
*/
-static inline unsigned long long cycles_2_ns(unsigned long long cyc)
+static __always_inline unsigned long long cycles_2_ns(unsigned long long cyc)
{
struct cyc2ns_data data;
unsigned long long ns;
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] x86/percpu: Fix this_cpu_read()
2018-10-11 10:38 [PATCH 0/2] x86/tsc: Fix native_sched_clock() Peter Zijlstra
2018-10-11 10:38 ` [PATCH 1/2] x86/tsc: Force inlining of cyc2ns bits Peter Zijlstra
@ 2018-10-11 10:38 ` Peter Zijlstra
2018-10-11 15:02 ` Eric Dumazet
2018-10-14 9:16 ` [tip:x86/urgent] " tip-bot for Peter Zijlstra
1 sibling, 2 replies; 9+ messages in thread
From: Peter Zijlstra @ 2018-10-11 10:38 UTC (permalink / raw)
To: mingo, tglx; +Cc: linux-kernel, hpa, edumazet, eric.dumazet, bp, peterz
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 <edumazet@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
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; \
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/percpu: Fix this_cpu_read()
2018-10-11 10:38 ` [PATCH 2/2] x86/percpu: Fix this_cpu_read() Peter Zijlstra
@ 2018-10-11 15:02 ` Eric Dumazet
2018-10-11 15:24 ` Eric Dumazet
2018-10-14 9:16 ` [tip:x86/urgent] " tip-bot for Peter Zijlstra
1 sibling, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2018-10-11 15:02 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Thomas Gleixner, LKML, H. Peter Anvin, Eric Dumazet,
Borislav Petkov
On Thu, Oct 11, 2018 at 3:45 AM Peter Zijlstra <peterz@infradead.org> 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 <edumazet@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Eric Dumazet <edumazet@google.com>
> ---
> 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; \
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/percpu: Fix this_cpu_read()
2018-10-11 15:02 ` Eric Dumazet
@ 2018-10-11 15:24 ` Eric Dumazet
2018-10-11 15:50 ` Peter Zijlstra
0 siblings, 1 reply; 9+ messages in thread
From: Eric Dumazet @ 2018-10-11 15:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Thomas Gleixner, LKML, H. Peter Anvin, Eric Dumazet,
Borislav Petkov
On Thu, Oct 11, 2018 at 8:02 AM Eric Dumazet <edumazet@google.com> wrote:
>
> On Thu, Oct 11, 2018 at 3:45 AM Peter Zijlstra <peterz@infradead.org> 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 <edumazet@google.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
>
>
> Acked-by: Eric Dumazet <edumazet@google.com>
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())
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/percpu: Fix this_cpu_read()
2018-10-11 15:24 ` Eric Dumazet
@ 2018-10-11 15:50 ` Peter Zijlstra
2018-10-11 16:08 ` Eric Dumazet
0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2018-10-11 15:50 UTC (permalink / raw)
To: Eric Dumazet
Cc: Ingo Molnar, Thomas Gleixner, LKML, H. Peter Anvin, Eric Dumazet,
Borislav Petkov
On Thu, Oct 11, 2018 at 08:24:49AM -0700, Eric Dumazet wrote:
> On Thu, Oct 11, 2018 at 8:02 AM Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Thu, Oct 11, 2018 at 3:45 AM Peter Zijlstra <peterz@infradead.org> 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 <edumazet@google.com>
> > > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> >
> >
> > Acked-by: Eric Dumazet <edumazet@google.com>
>
> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] x86/percpu: Fix this_cpu_read()
2018-10-11 15:50 ` Peter Zijlstra
@ 2018-10-11 16:08 ` Eric Dumazet
0 siblings, 0 replies; 9+ messages in thread
From: Eric Dumazet @ 2018-10-11 16:08 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Ingo Molnar, Thomas Gleixner, LKML, H. Peter Anvin, Eric Dumazet,
Borislav Petkov
On Thu, Oct 11, 2018 at 8:50 AM Peter Zijlstra <peterz@infradead.org> 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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [tip:x86/urgent] x86/tsc: Force inlining of cyc2ns bits
2018-10-11 10:38 ` [PATCH 1/2] x86/tsc: Force inlining of cyc2ns bits Peter Zijlstra
@ 2018-10-14 9:15 ` tip-bot for Peter Zijlstra
0 siblings, 0 replies; 9+ messages in thread
From: tip-bot for Peter Zijlstra @ 2018-10-14 9:15 UTC (permalink / raw)
To: linux-tip-commits; +Cc: tglx, hpa, linux-kernel, peterz, edumazet, mingo
Commit-ID: 4907c68abd3f60f650f98d5a69d4ec77c0bde44f
Gitweb: https://git.kernel.org/tip/4907c68abd3f60f650f98d5a69d4ec77c0bde44f
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 11 Oct 2018 12:38:26 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
CommitDate: Sun, 14 Oct 2018 11:11:22 +0200
x86/tsc: Force inlining of cyc2ns bits
Looking at the asm for native_sched_clock() I noticed we don't inline
enough. Mostly caused by sharing code with cyc2ns_read_begin(), which
we didn't used to do. So mark all that __force_inline to make it DTRT.
Fixes: 59eaef78bfea ("x86/tsc: Remodel cyc2ns to use seqcount_latch()")
Reported-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: hpa@zytor.com
Cc: eric.dumazet@gmail.com
Cc: bp@alien8.de
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20181011104019.695196158@infradead.org
---
arch/x86/kernel/tsc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index b52bd2b6cdb4..6d5dc5dabfd7 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -58,7 +58,7 @@ struct cyc2ns {
static DEFINE_PER_CPU_ALIGNED(struct cyc2ns, cyc2ns);
-void cyc2ns_read_begin(struct cyc2ns_data *data)
+void __always_inline cyc2ns_read_begin(struct cyc2ns_data *data)
{
int seq, idx;
@@ -75,7 +75,7 @@ void cyc2ns_read_begin(struct cyc2ns_data *data)
} while (unlikely(seq != this_cpu_read(cyc2ns.seq.sequence)));
}
-void cyc2ns_read_end(void)
+void __always_inline cyc2ns_read_end(void)
{
preempt_enable_notrace();
}
@@ -104,7 +104,7 @@ void cyc2ns_read_end(void)
* -johnstul@us.ibm.com "math is hard, lets go shopping!"
*/
-static inline unsigned long long cycles_2_ns(unsigned long long cyc)
+static __always_inline unsigned long long cycles_2_ns(unsigned long long cyc)
{
struct cyc2ns_data data;
unsigned long long ns;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [tip:x86/urgent] x86/percpu: Fix this_cpu_read()
2018-10-11 10:38 ` [PATCH 2/2] x86/percpu: Fix this_cpu_read() Peter Zijlstra
2018-10-11 15:02 ` Eric Dumazet
@ 2018-10-14 9:16 ` tip-bot for Peter Zijlstra
1 sibling, 0 replies; 9+ messages in thread
From: tip-bot for Peter Zijlstra @ 2018-10-14 9:16 UTC (permalink / raw)
To: linux-tip-commits; +Cc: hpa, edumazet, tglx, linux-kernel, peterz, mingo
Commit-ID: b59167ac7bafd804c91e49ad53c6d33a7394d4c8
Gitweb: https://git.kernel.org/tip/b59167ac7bafd804c91e49ad53c6d33a7394d4c8
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Thu, 11 Oct 2018 12:38:27 +0200
Committer: Thomas Gleixner <tglx@linutronix.de>
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 <edumazet@google.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Eric Dumazet <edumazet@google.com>
Cc: hpa@zytor.com
Cc: eric.dumazet@gmail.com
Cc: bp@alien8.de
Cc: stable@vger.kernel.org
Link: https://lkml.kernel.org/r/20181011104019.748208519@infradead.org
---
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; \
^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-14 9:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-11 10:38 [PATCH 0/2] x86/tsc: Fix native_sched_clock() Peter Zijlstra
2018-10-11 10:38 ` [PATCH 1/2] x86/tsc: Force inlining of cyc2ns bits Peter Zijlstra
2018-10-14 9:15 ` [tip:x86/urgent] " tip-bot for Peter Zijlstra
2018-10-11 10:38 ` [PATCH 2/2] x86/percpu: Fix this_cpu_read() Peter Zijlstra
2018-10-11 15:02 ` Eric Dumazet
2018-10-11 15:24 ` Eric Dumazet
2018-10-11 15:50 ` Peter Zijlstra
2018-10-11 16:08 ` Eric Dumazet
2018-10-14 9:16 ` [tip:x86/urgent] " tip-bot for Peter Zijlstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).