[PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Scotty Bauer]

[-- Attachment #1: Type: text/plain, Size: 243 bytes --]

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.






[-- Attachment #2: 0001-Add-check-to-determine-if-CLFLUSH-is-actually-necess.patch --]
[-- Type: text/x-diff, Size: 1505 bytes --]

>From 3da1be5c998a8d51f98fdba09b3cb477526c5ff3 Mon Sep 17 00:00:00 2001
From: Scott Bauer <sbauer@eng.utah.edu>
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 <sbauer@eng.utah.edu>
---
 arch/x86/kernel/smpboot.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c..552ff48 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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);
-- 
1.9.1

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Borislav Petkov]

On Fri, Jan 30, 2015 at 02:26:17PM -0700, Scotty Bauer wrote:
> 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 <sbauer@eng.utah.edu>
> 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 <sbauer@eng.utah.edu>
> ---
>  arch/x86/kernel/smpboot.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 6d7022c..552ff48 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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();

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/1422377631-8986-3-git-send-email-ross.zwisler@linux.intel.com

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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Borislav Petkov]

On Sat, Jan 31, 2015 at 12:31:42AM +0100, Borislav Petkov wrote:
> 	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/1422987390-17878-1-git-send-email-bp@alien8.de

---
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c683e3..771ebd6e8b77 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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
...
---

:-)

-- 
Regards/Gruss,
    Boris.

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

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

[Borislav Petkov]

From: Borislav Petkov <bp@suse.de>
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 <sbauer@eng.utah.edu>.

Cc: Scotty Bauer <sbauer@eng.utah.edu>
Signed-off-by: Borislav Petkov <bp@suse.de>
---
 arch/x86/kernel/smpboot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 6d7022c683e3..771ebd6e8b77 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -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();
-- 
2.2.0.33.gc18b867


-- 
Regards/Gruss,
    Boris.

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

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

[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.

--Scotty

On 02/06/2015 09:13 AM, Borislav Petkov wrote:
> From: Borislav Petkov <bp@suse.de>
> 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 <sbauer@eng.utah.edu>.
>
> Cc: Scotty Bauer <sbauer@eng.utah.edu>
> Signed-off-by: Borislav Petkov <bp@suse.de>
> ---
>  arch/x86/kernel/smpboot.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
> index 6d7022c683e3..771ebd6e8b77 100644
> --- a/arch/x86/kernel/smpboot.c
> +++ b/arch/x86/kernel/smpboot.c
> @@ -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();

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

[Borislav Petkov]

Hi Scotty,

On Wed, Feb 11, 2015 at 11:39:32AM -0700, Scotty Bauer wrote:
> 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.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[H. Peter Anvin]

On 01/30/2015 01:26 PM, Scotty Bauer wrote:
> 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

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[H. Peter Anvin]

On 01/30/2015 01:26 PM, Scotty Bauer wrote:
> 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

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Scotty Bauer]

On 02/11/2015 02:55 PM, H. Peter Anvin wrote:
> On 01/30/2015 01:26 PM, Scotty Bauer wrote:
>> 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?

Re: [PATCH] x86/smpboot: check if CLFLUSH is actually necessary

[Borislav Petkov]

On Wed, Feb 11, 2015 at 04:10:12PM -0700, Scotty Bauer wrote:
> 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...

-- 
Regards/Gruss,
    Boris.

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