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

[Wyes Karny <wyes.karny@amd.com>]

Hello Dave,

On 5/19/2022 9:30 PM, Dave Hansen wrote:
> On 5/10/22 03:18, Wyes Karny wrote:
>>  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;
>>  
>> -	if (!cpu_has(c, X86_FEATURE_MWAIT) || boot_cpu_has_bug(X86_BUG_MONITOR))
>> +	/* Monitor has a bug. Fallback to HALT */
>> +	if (boot_cpu_has_bug(X86_BUG_MONITOR))
>>  		return 0;
> 
> So, before, we pretty much just assume that all Intel CPUs with MWAIT
> should use MWAIT C1.
> 
>> -	return 1;
>> +	cpuid(CPUID_MWAIT_LEAF, &eax, &ebx, &ecx, &edx);
>> +
>> +	/*
>> +	 * If MWAIT extensions are not available, it is safe to use MWAIT
>> +	 * with EAX=0, ECX=0.
>> +	 */
>> +	if (!(ecx & CPUID5_ECX_EXTENSIONS_SUPPORTED))
>> +		return 1;
>> +
>> +	/*
>> +	 * If MWAIT extensions are available, there should be least one
>> +	 * MWAIT C1 substate present.
>> +	 */
>> +	return (edx & MWAIT_C1_SUBSTATE_MASK);
>>  }
> So, I guess the "If MWAIT extensions are not available" check is
> consistent with the "always use it on Intel" behavior.
> 
> But, this would change the behavior on Intel systems that both have
> CPUID5_ECX_EXTENSIONS_SUPPORTED and do not set bits in
> MWAIT_C1_SUBSTATE_MASK.
> 
> Is that a problem or an improvement?

Prior to commit 69fb3676df3 ("x86 idle: remove mwait_idle() and "idle=mwait" cmdline param")
the decision tree only allowed MWAIT to be the preferred idle state even on Intel CPUs only
when C1 substates were present. It was unconditionally allowed on Intel CPUs only after the
commit b253149b843f ("sched/idle/x86: Restore mwait_idle() to fix boot hangs caused by the 
removal of mwait_idle(). Are there any subsequent Intel processors which have MWAIT but 
no C1 substates ? And is MWAIT preferred to be the default idle state on those CPUs ?


Thanks,
Wyes