LKML Archive on lore.kernel.org
 help / Atom feed
* [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	[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	[flat|nested] 9+ messages in thread

end of thread, back to index

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

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox