[PATCH v2 0/3] x86: Prefer MWAIT over HALT on AMD processors

[PATCH v2 0/3] x86: Prefer MWAIT over HALT on AMD processors

[Wyes Karny]

Currently in the absence of the cpuidle driver (eg: when global
C-States are disabled in the BIOS or when cpuidle is driver is not
compiled in), the default idle state on AMD Zen processors uses the
HALT instruction even though there is support for MWAIT instruction
which is more efficient than HALT.

HPC customers who want to optimize for lower latency are known to
disable Global C-States in the BIOS. In this scenario, the cpuidle driver
will not be loaded and the kernel will continue with the default idle state
chosen at boot time. On AMD systems currently the default idle state
is HALT which has a higher exit latency compared to MWAIT.

The reason for this is 

1. Families prior to 10h didn't support MWAIT
2. Families 10h-15h supported MWAIT, but not MWAIT C1. Hence it was
   preferable to use HALT as the default state on these systems.

However, AMD Family 17h onwards supports MWAIT as well as MWAIT
C1. And it is preferable to use MWAIT as the default idle state on
these systems, as it has lower exit latencies.

The below table represents the exit latency for HALT and MWAIT on AMD
Zen 3 system.
Exit latency is measured by issuing a wakeup (IPI) to other
CPU and measuring how many clock cycles it took to wakeup.
Each iteration measures 10K wakeups by pinning source and
destination.

HALT:

25.0000th percentile  :      1900 ns
50.0000th percentile  :      2000 ns
75.0000th percentile  :      2300 ns
90.0000th percentile  :      2500 ns
95.0000th percentile  :      2600 ns
99.0000th percentile  :      2800 ns
99.5000th percentile  :      3000 ns
99.9000th percentile  :      3400 ns
99.9500th percentile  :      3600 ns
99.9900th percentile  :      5900 ns
  Min latency         :      1700 ns
  Max latency         :      5900 ns
Total Samples      9999

MWAIT:

25.0000th percentile  :      1400 ns
50.0000th percentile  :      1500 ns
75.0000th percentile  :      1700 ns
90.0000th percentile  :      1800 ns
95.0000th percentile  :      1900 ns
99.0000th percentile  :      2300 ns
99.5000th percentile  :      2500 ns
99.9000th percentile  :      3200 ns
99.9500th percentile  :      3500 ns
99.9900th percentile  :      4600 ns
  Min latency         :      1200 ns
  Max latency         :      4600 ns
Total Samples      9997

Improvement (99th percentile): 21.74%

Below is another result for context_switch2 micro-benchmark,
which brings out the impact of improved wakeup latency through
increased context-switches per second.

Link: https://ozlabs.org/~anton/junkcode/context_switch2.c

with HALT:
-------------------------------
50.0000th percentile  :  190184
75.0000th percentile  :  191032
90.0000th percentile  :  192314
95.0000th percentile  :  192520
99.0000th percentile  :  192844
MIN  :  190148
MAX  :  192852

with MWAIT:
-------------------------------
50.0000th percentile  :  277444
75.0000th percentile  :  278268
90.0000th percentile  :  278888
95.0000th percentile  :  279164
99.0000th percentile  :  280504
MIN  :  273278
MAX  :  281410


Improvement(99th percentile): ~ 45.46%

A similar trend is observed on older Zen processors also.

Here we enable MWAIT instruction as the default idle call for AMD
Zen processors which support MWAIT. We retain the existing behaviour
for older processors which depend on HALT.

This patchset restores the decision tree that was present in the kernel
earlier due to Thomas Gleixner's patch:
commit 09fd4b4ef5bc ("x86: use cpuid to check MWAIT support for C1")

NOTE: This change only impacts the default idle behaviour in the
absence of cpuidle driver. If the cpuidle driver is present, it
controls the processor idle behaviour.

Fixes: commit b253149b843f ("sched/idle/x86: Restore mwait_idle() to fix boot hangs, to improve power savings and to improve performance")

Changelog:
v2:
- Remove vendor checks, fix idle=nomwait condition, fix documentation

Wyes Karny (3):
  x86: Use HALT in default_idle when idle=nomwait cmdline arg is passed
  x86: Remove vendor checks from prefer_mwait_c1_over_halt
  x86: Fix comment for X86_FEATURE_ZEN

 arch/x86/include/asm/cpufeatures.h |  2 +-
 arch/x86/include/asm/mwait.h       |  1 +
 arch/x86/kernel/process.c          | 39 ++++++++++++++++++++++--------
 3 files changed, 31 insertions(+), 11 deletions(-)

-- 
2.27.0

[PATCH v2 1/3] x86: Use HALT in default_idle when idle=nomwait cmdline arg is passed

[Wyes Karny]

When kernel is booted with idle=nomwait avoid selecting MWAIT
as the  default idle state.

If the user boots the kernel with idle=nomwait, it is a clear
indication that they do not prefer the use of mwait as the default
idle state.  However, the current code does not take this into
consideration while selecting the default idle state on x86.

This patch fixes it by checking for the idle=nomwait boot option in
prefer_mwait_c1_over_halt().

Signed-off-by: Wyes Karny <wyes.karny@amd.com>
---
Maintainers,
 Should this be marked to stable ?

 arch/x86/kernel/process.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index b370767f5b19..49b915d1b7b4 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -824,6 +824,10 @@ static void amd_e400_idle(void)
  */
 static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
 {
+	/* User has disallowed the use of MWAIT. Fallback to HALT */
+	if (boot_option_idle_override == IDLE_NOMWAIT)
+		return 0;
+
 	if (c->x86_vendor != X86_VENDOR_INTEL)
 		return 0;
 
-- 
2.27.0

[PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt

[Wyes Karny]

Remove vendor checks form prefer_mwait_c1_over_halt function.
Based on CPUID give preference to MWAIT C1.

Restore the decision tree to support MWAIT as the default idle state
based on CPUID checks as done by Thomas Gleixner in an earlier patch:
commit 09fd4b4ef5bc ("x86: use cpuid to check MWAIT support for C1")

The commit which removed the decision tree is:
commit 69fb3676df33 ("x86 idle: remove mwait_idle() and "idle=mwait" cmdline param")

Below conditions clearly say that is MWAIT is preferred or not:
    1. CPUID_Fn00000001_ECX [Monitor] should be set
    2. CPUID_Fn00000005 should be supported
    3. If CPUID_Fn00000005_ECX [EMX] is set then there should be
       at least one C1 substate available, indicated by
       CPUID_Fn00000005_EDX [MWaitC1SubStates] bits.

Otherwise use HALT for default_idle function.

HPC customers who want to optimize for lower latency are known to
disable Global C-States in the BIOS. In this scenario, the cpuidle driver
will not be loaded and the kernel will continue with the default idle state
chosen at boot time. On AMD systems currently the default idle state
is HALT which has a higher exit latency compared to MWAIT.

The reason for this is

1. Families prior to 10h didn't support MWAIT
2. Families 10h-15h supported MWAIT, but not MWAIT C1. Hence it was
   preferable to use HALT as the default state on these systems.

However, AMD Family 17h onwards supports MWAIT as well as MWAIT
C1. And it is preferable to use MWAIT as the default idle state on
these systems, as it has lower exit latencies.

The below table represents the exit latency for HALT and MWAIT on AMD
Zen 3 system.
Exit latency is measured by issuing a wakeup (IPI) to other
CPU and measuring how many clock cycles it took to wakeup.
Each iteration measures 10K wakeups by pinning source and
destination.

HALT:

25.0000th percentile  :      1900 ns
50.0000th percentile  :      2000 ns
75.0000th percentile  :      2300 ns
90.0000th percentile  :      2500 ns
95.0000th percentile  :      2600 ns
99.0000th percentile  :      2800 ns
99.5000th percentile  :      3000 ns
99.9000th percentile  :      3400 ns
99.9500th percentile  :      3600 ns
99.9900th percentile  :      5900 ns
  Min latency         :      1700 ns
  Max latency         :      5900 ns
Total Samples      9999

MWAIT:

25.0000th percentile  :      1400 ns
50.0000th percentile  :      1500 ns
75.0000th percentile  :      1700 ns
90.0000th percentile  :      1800 ns
95.0000th percentile  :      1900 ns
99.0000th percentile  :      2300 ns
99.5000th percentile  :      2500 ns
99.9000th percentile  :      3200 ns
99.9500th percentile  :      3500 ns
99.9900th percentile  :      4600 ns
  Min latency         :      1200 ns
  Max latency         :      4600 ns
Total Samples      9997

Improvement (99th percentile): 21.74%

Below is another result for context_switch2 micro-benchmark,
which brings out the impact of improved wakeup latency through
increased context-switches per second.

Link: https://ozlabs.org/~anton/junkcode/context_switch2.c

with HALT:
-------------------------------
50.0000th percentile  :  190184
75.0000th percentile  :  191032
90.0000th percentile  :  192314
95.0000th percentile  :  192520
99.0000th percentile  :  192844
MIN  :  190148
MAX  :  192852

with MWAIT:
-------------------------------
50.0000th percentile  :  277444
75.0000th percentile  :  278268
90.0000th percentile  :  278888
95.0000th percentile  :  279164
99.0000th percentile  :  280504
MIN  :  273278
MAX  :  281410

Improvement(99th percentile): ~ 45.46%

Signed-off-by: Wyes Karny <wyes.karny@amd.com>
---
 arch/x86/include/asm/mwait.h |  1 +
 arch/x86/kernel/process.c    | 35 +++++++++++++++++++++++++----------
 2 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
index 29dd27b5a339..3a8fdf881313 100644
--- a/arch/x86/include/asm/mwait.h
+++ b/arch/x86/include/asm/mwait.h
@@ -13,6 +13,7 @@
 #define MWAIT_SUBSTATE_SIZE		4
 #define MWAIT_HINT2CSTATE(hint)		(((hint) >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK)
 #define MWAIT_HINT2SUBSTATE(hint)	((hint) & MWAIT_CSTATE_MASK)
+#define MWAIT_C1_SUBSTATE_MASK  0xf0
 
 #define CPUID_MWAIT_LEAF		5
 #define CPUID5_ECX_EXTENSIONS_SUPPORTED 0x1
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 49b915d1b7b4..314924e9b8e8 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -813,28 +813,43 @@ static void amd_e400_idle(void)
 }
 
 /*
- * Intel Core2 and older machines prefer MWAIT over HALT for C1.
- * We can't rely on cpuidle installing MWAIT, because it will not load
- * on systems that support only C1 -- so the boot default must be MWAIT.
+ * Prefer MWAIT over HALT if MWAIT is supported, MWAIT_CPUID leaf
+ * exists and whenever MONITOR/MWAIT extensions are present there is at
+ * least 1 C1 substate.
  *
- * Some AMD machines are the opposite, they depend on using HALT.
- *
- * So for default C1, which is used during boot until cpuidle loads,
- * use MWAIT-C1 on Intel HW that has it, else use HALT.
+ * Do not prefer MWAIT if MONITOR instruction has a bug or idle=nomwait
+ * is passed to kernel commandline parameter.
  */
 static int prefer_mwait_c1_over_halt(const struct cpuinfo_x86 *c)
 {
+	u32 eax, ebx, ecx, edx;
+
 	/* User has disallowed the use of MWAIT. Fallback to HALT */
 	if (boot_option_idle_override == IDLE_NOMWAIT)
 		return 0;
 
-	if (c->x86_vendor != X86_VENDOR_INTEL)
+	/* MWAIT is not supported on this platform. Fallback to HALT */
+	if (!cpu_has(c, X86_FEATURE_MWAIT))
+		return 0;
+
+	/* Monitor has a bug. Fallback to HALT */
+	if (boot_cpu_has_bug(X86_BUG_MONITOR))
 		return 0;
 
-	if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
+	if (c->cpuid_level < CPUID_MWAIT_LEAF)
 		return 0;
 
-	return 1;
+	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
+
+	/*
+	 * If ECX doesn't have extended info about MWAIT,
+	 * don't need to check substates.
+	 */
+	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED))
+		return 1;
+
+	/* Check, whether at least 1 MWAIT C1 substate is present */
+	return (edx & MWAIT_C1_SUBSTATE_MASK);
 }
 
 /*
-- 
2.27.0

[PATCH v2 3/3] x86: Fix comment for X86_FEATURE_ZEN

[Wyes Karny]

The feature X86_FEATURE_ZEN implies that the CPU supports Zen
microarchitecture. Call this out explicitly in the comment.

Signed-off-by: Wyes Karny <wyes.karny@amd.com>
---
 arch/x86/include/asm/cpufeatures.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 73e643ae94b6..c851ab4e45b9 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -219,7 +219,7 @@
 #define X86_FEATURE_IBRS		( 7*32+25) /* Indirect Branch Restricted Speculation */
 #define X86_FEATURE_IBPB		( 7*32+26) /* Indirect Branch Prediction Barrier */
 #define X86_FEATURE_STIBP		( 7*32+27) /* Single Thread Indirect Branch Predictors */
-#define X86_FEATURE_ZEN			( 7*32+28) /* "" CPU is AMD family 0x17 or above (Zen) */
+#define X86_FEATURE_ZEN			(7*32+28) /* "" CPU supports Zen microarchitecture */
 #define X86_FEATURE_L1TF_PTEINV		( 7*32+29) /* "" L1TF workaround PTE inversion */
 #define X86_FEATURE_IBRS_ENHANCED	( 7*32+30) /* Enhanced IBRS */
 #define X86_FEATURE_MSR_IA32_FEAT_CTL	( 7*32+31) /* "" MSR IA32_FEAT_CTL configured */
-- 
2.27.0

Re: [PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt

[Dave Hansen]

On 5/5/22 04:01, Wyes Karny wrote:
> -	if (c->x86_vendor != X86_VENDOR_INTEL)
> +	/* MWAIT is not supported on this platform. Fallback to HALT */
> +	if (!cpu_has(c, X86_FEATURE_MWAIT))
> +		return 0;
> +
> +	/* Monitor has a bug. Fallback to HALT */
> +	if (boot_cpu_has_bug(X86_BUG_MONITOR))
>  		return 0;
>  
> -	if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
> +	if (c->cpuid_level < CPUID_MWAIT_LEAF)
>  		return 0;

First of all, thanks for all the detail in this new version of the patches!

In arch/x86/kernel/cpu/common.c, we have:

cpuid_dependent_features[] = {
        { X86_FEATURE_MWAIT,            0x00000005 },
	...

Shouldn't that clear X86_FEATURE_MWAIT on all systems without
CPUID_MWAIT_LEAF?  That would make the c->cpuid_level check above
unnecessary.

> +	/*
> +	 * If ECX doesn't have extended info about MWAIT,
> +	 * don't need to check substates.
> +	 */
> +	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED))
> +		return 1;

Could you explain a bit more about this?  I don't know this CPUID leaf
off the top of my head and the line after this is checking EDX.  It's
not apparent from this comment why "!ECX_EXTENSIONS_SUPPORTED" means
that MWAIT should be preferred.

> +	/* Check, whether at least 1 MWAIT C1 substate is present */
> +	return (edx & MWAIT_C1_SUBSTATE_MASK);

Re: [PATCH v2 3/3] x86: Fix comment for X86_FEATURE_ZEN

[Dave Hansen]

On 5/5/22 04:04, Wyes Karny wrote:
> The feature X86_FEATURE_ZEN implies that the CPU supports Zen
> microarchitecture. Call this out explicitly in the comment.

Is "supports" the best word here?

A CPU is based on a microarchitecture, it doesn't really support it.  I
guess we could say the CPU supports a microarchitecture's *features*,
but that's a bit wordy for a tiny comment.

Maybe:

#define X86_FEATURE_ZEN			(7*32+28) /* "" CPU based on Zen uarch */

or spell out "microarchitecture" if there's room.

Re: [PATCH v2 1/3] x86: Use HALT in default_idle when idle=nomwait cmdline arg is passed

[Dave Hansen]

On 5/5/22 03:48, Wyes Karny wrote:
> When kernel is booted with idle=nomwait avoid selecting MWAIT
> as the  default idle state.
> 
> If the user boots the kernel with idle=nomwait, it is a clear
> indication that they do not prefer the use of mwait as the default
> idle state.  However, the current code does not take this into
> consideration while selecting the default idle state on x86.
> 
> This patch fixes it by checking for the idle=nomwait boot option in
> prefer_mwait_c1_over_halt().

There are a couple of places in the existing documentation that talk
about idle=nomwait.  Could you please fix those up to make it clear that
the option is no longer specific to intel_idle?  We don't need a rewrite
or anything, just some small tweaks in the language.

Re: [PATCH v2 3/3] x86: Fix comment for X86_FEATURE_ZEN

[Wyes Karny]

Hello Dave,

On 5/5/2022 10:40 PM, Dave Hansen wrote:
> On 5/5/22 04:04, Wyes Karny wrote:
>> The feature X86_FEATURE_ZEN implies that the CPU supports Zen
>> microarchitecture. Call this out explicitly in the comment.
> 
> Is "supports" the best word here?
> 
> A CPU is based on a microarchitecture, it doesn't really support it.  I
> guess we could say the CPU supports a microarchitecture's *features*,
> but that's a bit wordy for a tiny comment.
> 
> Maybe:
> 
> #define X86_FEATURE_ZEN			(7*32+28) /* "" CPU based on Zen uarch */

Thank you, Dave. Yes, "based on" could be a better choice here. I'll update this.

> 
> or spell out "microarchitecture" if there's room.

Thanks,
Wyes

Re: [PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt

[Wyes Karny]

Hello Dave,

On 5/5/2022 10:34 PM, Dave Hansen wrote:
> On 5/5/22 04:01, Wyes Karny wrote:
>> -	if (c->x86_vendor != X86_VENDOR_INTEL)
>> +	/* MWAIT is not supported on this platform. Fallback to HALT */
>> +	if (!cpu_has(c, X86_FEATURE_MWAIT))
>> +		return 0;
>> +
>> +	/* Monitor has a bug. Fallback to HALT */
>> +	if (boot_cpu_has_bug(X86_BUG_MONITOR))
>>  		return 0;
>>  
>> -	if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
>> +	if (c->cpuid_level < CPUID_MWAIT_LEAF)
>>  		return 0;
> 
> First of all, thanks for all the detail in this new version of the patches!
> 
> In arch/x86/kernel/cpu/common.c, we have:
> 
> cpuid_dependent_features[] = {
>         { X86_FEATURE_MWAIT,            0x00000005 },
> 	...
> 
> Shouldn't that clear X86_FEATURE_MWAIT on all systems without
> CPUID_MWAIT_LEAF?  That would make the c->cpuid_level check above
> unnecessary.

Agreed. I will update it in the next version.

> 
>> +	/*
>> +	 * If ECX doesn't have extended info about MWAIT,
>> +	 * don't need to check substates.
>> +	 */
>> +	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED))
>> +		return 1;
> 
> Could you explain a bit more about this?  I don't know this CPUID leaf
> off the top of my head and the line after this is checking EDX.  It's
> not apparent from this comment why "!ECX_EXTENSIONS_SUPPORTED" means
> that MWAIT should be preferred.

MWAIT can be used for two cases - address range monitoring and advanced power management.

According to Intel Architectures Software Developer’s Manual, Volume2B:

"MWAIT accepts a hint and optional extension to the processor that it 
can enter a specified target C state while waiting for an event or a store 
operation to the address range armed by MONITOR. Support for MWAIT extensions
for power management is indicated by CPUID.05H:ECX[bit 0] reporting 1.

EAX and ECX are used to communicate the additional information to the MWAIT instruction,
such as the kind of optimized state the processor should enter.

ECX specifies optional extensions for the MWAIT instruction.
EAX may contain hints such as the preferred optimized state the processor should enter."

So, if CPUID.05H:ECX[0] = 0, MWAIT extensions are not available (not allowed) and hence 
it is safe to use MWAIT with EAX=0,ECX=0. Otherwise, we have to check CPUID.05H:EDX[bit 7:4]
to get the number of C1 substates and this should be greater than equal to 1.

> 
>> +	/* Check, whether at least 1 MWAIT C1 substate is present */
>> +	return (edx & MWAIT_C1_SUBSTATE_MASK);
> 

Thanks,
Wyes

Re: [PATCH v2 1/3] x86: Use HALT in default_idle when idle=nomwait cmdline arg is passed

[Wyes Karny]

On 5/5/2022 10:43 PM, Dave Hansen wrote:
> On 5/5/22 03:48, Wyes Karny wrote:
>> When kernel is booted with idle=nomwait avoid selecting MWAIT
>> as the  default idle state.
>>
>> If the user boots the kernel with idle=nomwait, it is a clear
>> indication that they do not prefer the use of mwait as the default
>> idle state.  However, the current code does not take this into
>> consideration while selecting the default idle state on x86.
>>
>> This patch fixes it by checking for the idle=nomwait boot option in
>> prefer_mwait_c1_over_halt().
> 
> There are a couple of places in the existing documentation that talk
> about idle=nomwait.  Could you please fix those up to make it clear that
> the option is no longer specific to intel_idle?  We don't need a rewrite
> or anything, just some small tweaks in the language.

Sure, will do. Thanks!

Re: [PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt

[Dave Hansen]

On 5/6/22 02:42, Wyes Karny wrote:
> 
> So, if CPUID.05H:ECX[0] = 0, MWAIT extensions are not available (not allowed) and hence 
> it is safe to use MWAIT with EAX=0,ECX=0. Otherwise, we have to check CPUID.05H:EDX[bit 7:4]
> to get the number of C1 substates and this should be greater than equal to 1.

Ahh, I misread the comment.  I was confusing the CPUID leaf ECX data
with the use of ECX hints to MWAIT.

Could you add maybe a sentence or two more in that comment to help
clarify the situation?

Re: [PATCH v2 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt

[Wyes Karny]

On 5/6/2022 9:22 PM, Dave Hansen wrote:
> On 5/6/22 02:42, Wyes Karny wrote:
>>
>> So, if CPUID.05H:ECX[0] = 0, MWAIT extensions are not available (not allowed) and hence 
>> it is safe to use MWAIT with EAX=0,ECX=0. Otherwise, we have to check CPUID.05H:EDX[bit 7:4]
>> to get the number of C1 substates and this should be greater than equal to 1.
> 
> Ahh, I misread the comment.  I was confusing the CPUID leaf ECX data
> with the use of ECX hints to MWAIT.
> 
> Could you add maybe a sentence or two more in that comment to help
> clarify the situation?

Sure, will do. Thanks!

>