Messages in this thread | | | Subject | Re: [PATCH v3 2/3] x86: Remove vendor checks from prefer_mwait_c1_over_halt | From | Zhang Rui <> | Date | Fri, 20 May 2022 23:46:01 +0800 |
| |
On Fri, 2022-05-20 at 21:43 +0800, Zhang Rui wrote: > On Thu, 2022-05-19 at 09:00 -0700, 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; > > I'm new to x86 code, a dumb question, what about the other vendors? > with this patch, prefer_mwait_c1_over_halt() can return 1 for other > vendors as well? > > > > > > > - 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? > > At least Intel processors since Nehalem have MWAIT C1 support. > For elder ones, need to confirm with Len. > > When no bits set in MWAIT_C1_SUBSTATE_MASK, it means MWAIT C1 is not > available for some reason, let me check if I can make this happen or > not in real life.
I tried an Icelake server and a whiskeylake client, the supported cstates in CPUID(5) edx doesn't change when a specific cstate or all cstates are enabled/disabled in BIOS.
So there are always bits set in MWAIT_C1_SUBSTATE_MASK, and this patch doesn't make any change to these Intel processors that have MWAIT C1 support.
thanks, rui
| |