x86/smpboot: check if CLFLUSH is actually necessary

PATCH In review Scotty Bauer 3 participants 10 messages Jan 30, 2015 – Feb 12, 2015 x86

Share & tools

Scotty Bauer

author

2015-01-30 22:02 UTC #0 lore raw

PATCH 1/1 x86/smpboot: check if CLFLUSH is actually necessary

mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon processors. We can now determine if the CPU is a buggy CPU. This patch checks if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the CLFLUSH.

Quoted 1 line

From 3da1be5c998a8d51f98fdba09b3cb477526c5ff3 Mon Sep 17 00:00:00 2001

From: Scott Bauer <[email protected]>
Date: Fri, 30 Jan 2015 14:10:37 -0700
Subject: [PATCH] Add check to determine if CLFLUSH is actually necessary
before monitor/wait

Signed-off-by Scott Bauer <[email protected]>

---
 arch/x86/kernel/smpboot.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

arch/x86/kernel/smpboot.c +10 -3

index 6d7022c..552ff48 100644
@@ -1384,6 +1384,7 @@ static inline void mwait_play_dead(void)
 	unsigned int highest_subcstate = 0;
 	void *mwait_ptr;
 	int i;
+	int cpu;
 
 	if (!this_cpu_has(X86_FEATURE_MWAIT))
 		return;

@@ -1420,6 +1421,7 @@ static inline void mwait_play_dead(void)
 	 * content is immaterial as it is not actually modified in any way.
 	 */
 	mwait_ptr = &current_thread_info()->flags;
+	cpu = smp_processor_id();
 
 	wbinvd();

@@ -1430,10 +1432,15 @@ static inline void mwait_play_dead(void)
 		 * needed, but it should be harmless in either case.
 		 * The WBINVD is insufficient due to the spurious-wakeup
 		 * case where we return around the loop.
+		 *
+		 * Check if the CLFLUSH is actually necessary before calling
 		 */
-		mb();
-		clflush(mwait_ptr);
-		mb();
+		if (cpu_has_bug(&cpu_data(cpu), X86_BUG_CLFLUSH_MONITOR)) {
+			mb();
+			clflush(mwait_ptr);
+			mb();
+		}
+
 		__monitor(mwait_ptr, 0, 0);
 		mb();
 		__mwait(eax, 0);

Signature · 1 line

1.9.1

Borislav Petkov

maintainer

2015-01-30 23:32 UTC #1 lore raw

Quoted 42 lines · Scotty Bauer

mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon processors. We can now determine if the CPU is a buggy CPU. This patch checks if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the CLFLUSH.
From 3da1be5c998a8d51f98fdba09b3cb477526c5ff3 Mon Sep 17 00:00:00 2001
From: Scott Bauer <[email protected]>
Date: Fri, 30 Jan 2015 14:10:37 -0700
Subject: [PATCH] Add check to determine if CLFLUSH is actually necessary
before monitor/wait
Signed-off-by Scott Bauer <[email protected]>
---
 arch/x86/kernel/smpboot.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
arch/x86/kernel/smpboot.c +2 -0
index 6d7022c..552ff48 100644
@@ -1384,6 +1384,7 @@ static inline void mwait_play_dead(void)
 	unsigned int highest_subcstate = 0;
 	void *mwait_ptr;
 	int i;
+	int cpu;
 
 	if (!this_cpu_has(X86_FEATURE_MWAIT))
 		return;
@@ -1420,6 +1421,7 @@ static inline void mwait_play_dead(void)
 	 * content is immaterial as it is not actually modified in any way.
 	 */
 	mwait_ptr = &current_thread_info()->flags;
+	cpu = smp_processor_id();
 
 	wbinvd();
 
@@ -1430,10 +1432,15 @@ static inline void mwait_play_dead(void)
 		 * needed, but it should be harmless in either case.
 		 * The WBINVD is insufficient due to the spurious-wakeup
 		 * case where we return around the loop.

arch/x86/kernel/smpboot.c static inline void mwait_play_dead(void) · line 1441

Quoted 10 lines

arch/x86/kernel/smpboot.c +6 -3

+		 *
+		 * Check if the CLFLUSH is actually necessary before calling
 		 */
-		mb();
-		clflush(mwait_ptr);
-		mb();
+		if (cpu_has_bug(&cpu_data(cpu), X86_BUG_CLFLUSH_MONITOR)) {
+			mb();
+			clflush(mwait_ptr);
+			mb();

Or you can so something even better:

	alternative(ASM_NOP3, "clflush", X86_BUG_CLFLUSH_MONITOR);

Well, almost, you'd also need to pass in mwait_ptr, i.e. something like that:

https://lkml.kernel.org/r/[email protected]

but simpler. Maybe this:

	asm volatile(ALTERNATIVE(ASM_NOP3, "clflush %[p]", X86_BUG_CLFLUSH_MONITOR)
		      : [p] "+m" (*mwait_ptr));

Totally untested though - it is supposed to show the idea only.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Borislav Petkov

maintainer

2015-02-06 16:06 UTC #2 lore raw

Quoted 4 lines · Borislav Petkov

	asm volatile(ALTERNATIVE(ASM_NOP3, "clflush %[p]", X86_BUG_CLFLUSH_MONITOR)
		      : [p] "+m" (*mwait_ptr));
Totally untested though - it is supposed to show the idea only.

Yeah, here's a working diff, ontop of this patchset:

https://lkml.kernel.org/r/[email protected]

---

arch/x86/kernel/smpboot.c +5 -1

index 6d7022c683e3..771ebd6e8b77 100644
@@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
 		 * case where we return around the loop.
 		 */
 		mb();
-		clflush(mwait_ptr);
+
+		asm volatile(ALTERNATIVE("", "clflush %[p]",
+					 X86_BUG_CLFLUSH_MONITOR)
+				: [p] "+m" (*(unsigned long *)mwait_ptr));
+
 		mb();
 		__monitor(mwait_ptr, 0, 0);
 		mb();

At build time you have:

--
...

ffffffff81038dcc:       65 48 8b 1c 25 88 ab    mov    %gs:0xab88,%rbx		# movq %gs:kernel_stack,%rbx      #, pfo_ret__
ffffffff81038dd3:       00 00
ffffffff81038dd5:       48 81 eb c8 3f 00 00    sub    $0x3fc8,%rbx		# subq    $16328, %rbx    #, mwait_ptr
ffffffff81038ddc:       0f 09                   wbinvd
ffffffff81038dde:       45 31 e4                xor    %r12d,%r12d
ffffffff81038de1:       0f 1f 80 00 00 00 00    nopl   0x0(%rax)
ffffffff81038de8:       0f ae f0                mfence
ffffffff81038deb:       90                      nop
ffffffff81038dec:       90                      nop
ffffffff81038ded:       90                      nop
ffffffff81038dee:       0f ae f0                mfence
ffffffff81038df1:       48 89 d8                mov    %rbx,%rax

...
--

which during runtime, on those affected machines only, gets patched to:

---
...

ffffffff81038dcc:  mov    %gs:0xab88,%rbx
ffffffff81038dd5:  sub    $0x3fc8,%rbx
ffffffff81038ddc:  wbinvd
ffffffff81038dde:  xor    %r12d,%r12d
ffffffff81038de1:  nopl   0x0(%rax)
ffffffff81038de8:  mfence
ffffffff81038deb:  clflush (%rbx)		<---
ffffffff81038dee:  mfence
ffffffff81038df1:  mov    %rbx,%rax

...
---

:-)

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Borislav Petkov

maintainer

2015-02-06 16:13 UTC #3 lore raw

PATCH 1/1 x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs

From: Borislav Petkov <[email protected]>
Subject: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs

Make the AAI65 erratum workaround for Xeon 7400 machines only instead of punishing all CPUs doing idle with MWAIT with the CLFLUSH penalty.

Based on a patch originally by Scotty Bauer <[email protected]>.

Cc Scotty Bauer <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/smpboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

arch/x86/kernel/smpboot.c +5 -1

index 6d7022c683e3..771ebd6e8b77 100644
@@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
 		 * case where we return around the loop.
 		 */
 		mb();
-		clflush(mwait_ptr);
+
+		asm volatile(ALTERNATIVE("", "clflush %[p]",
+					 X86_BUG_CLFLUSH_MONITOR)
+				: [p] "+m" (*(unsigned long *)mwait_ptr));
+
 		mb();
 		__monitor(mwait_ptr, 0, 0);
 		mb();

Signature · 9 lines

2.2.0.33.gc18b867


-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Scotty Bauer

author

2015-02-11 18:39 UTC #4 lore raw

Re: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs

For what its worth I tried it out and it works fine on my end.

Thanks for doing the hard work for me, Boris. Also, thanks for a pointer to the alternatives.

I think it may be worth doing a patch that is almost verbatim to this for mwait_idle_with_hints in arch/x86/include/asm/mwait.h to keep things consistent. I can work on that over the weekend.

--Scotty

Quoted 31 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
Subject: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs
Make the AAI65 erratum workaround for Xeon 7400 machines only instead of punishing all CPUs doing idle with MWAIT with the CLFLUSH penalty.
Based on a patch originally by Scotty Bauer <[email protected]>.
Cc Scotty Bauer <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/smpboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
arch/x86/kernel/smpboot.c +5 -1
index 6d7022c683e3..771ebd6e8b77 100644
@@ -1432,7 +1432,11 @@ static inline void mwait_play_dead(void)
 		 * case where we return around the loop.
 		 */
 		mb();
-		clflush(mwait_ptr);
+
+		asm volatile(ALTERNATIVE("", "clflush %[p]",
+					 X86_BUG_CLFLUSH_MONITOR)
+				: [p] "+m" (*(unsigned long *)mwait_ptr));
+
 		mb();
 		__monitor(mwait_ptr, 0, 0);
 		mb();

Borislav Petkov

maintainer

2015-02-11 20:26 UTC #5 lore raw

Re: [PATCH] x86, smpboot: Call CLFLUSH only on X86_BUG_CLFLUSH_MONITOR-affected CPUs

Hi Scotty,

Quoted 5 lines · Scotty Bauer

For what its worth I tried it out and it works fine on my end.
Thanks for doing the hard work for me, Boris. Also, thanks for a pointer to the alternatives.
I think it may be worth doing a patch that is almost verbatim to this for mwait_idle_with_hints in arch/x86/include/asm/mwait.h to keep things consistent. I can work on that over the weekend.

please do not top-post, thanks.

Right, in thinking about this more, your original version actually is, IMO, still the right thing to do.

Why, you ask. Well, because even with the alternatives, we need to alternate not only the CLFLUSH but the surrounding MFENCEs too. And those are different instructions on 32- and 64-bit. And doing that with the alternatives might become uglier/more cluttered in the end than your version.

And so, in the end of the day, having an unconditional, two-byte JMP in there for all machines which are *not* affected shouldn't hurt - we're jumping with great probability to the same I$ cacheline so not even a cache miss. And we're on our way to idle so one more JMP is a dont-care. And the C-code is actually readable. :-)

My only suggestion would be to change your original patch to what is done in mwait_idle_with_hints() and use static_cpu_has_bug().

Thanks.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

H. Peter Anvin

reviewer

2015-02-11 21:56 UTC #6 lore raw

In reply to Scotty Bauer

Quoted 4 lines · Scotty Bauer

mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon processors. We can now determine if the CPU is a buggy CPU. This patch checks if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the CLFLUSH.

Here is my first question: does this matter at all? Otherwise I don't see a point.

-hpa

Scotty Bauer

author

2015-02-11 23:10 UTC #7 lore raw

Quoted 11 lines · H. Peter Anvin

Quoted 4 lines · Scotty Bauer
mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon processors. We can now determine if the CPU is a buggy CPU. This patch checks if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the CLFLUSH.
Here is my first question: does this matter at all? Otherwise I don't see a point.
-hpa

Do you get the same effect? Sure, but is the previous way the right way to do it? In my opinion no, but I'm not the one merging code its up to someone more experienced to determine if the change is warranted. The change is slightly faster on non-buggy cpu, but like you mention, is that relevant when the machine is going into idle?

Borislav Petkov

maintainer

2015-02-12 09:16 UTC #8 lore raw

Quoted 5 lines · Scotty Bauer

Do you get the same effect? Sure, but is the previous way the right way to do it? In my opinion no, but I'm not the one merging code its up to someone more experienced to determine if the change is warranted. The change is slightly faster on non-buggy cpu, but like you mention, is that relevant when the machine is going into idle?

Right, it probably doesn't matter on the way to idle but we could do the change just to make the code the same as in mwait_idle_with_hints() so that there are no more why-is-this-different questions in the future.

Or even make it a small function called prepare_monitor() and put the if-check in there and call that function in both places...

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

H. Peter Anvin

reviewer

2015-02-11 21:56 UTC #9 lore raw

In reply to Scotty Bauer

Quoted 4 lines · Scotty Bauer

mwait_play_dead previously issued a CLFLUSH to work around a bug on some xeon processors. We can now determine if the CPU is a buggy CPU. This patch checks if if we're on a buggy CPU which allows non-buggy cpu's to eliminate the CLFLUSH.

Here is my first question: does this matter at all? Otherwise I don't see a point.

-hpa