| Message ID | 20241108122909.763663-3-patryk.wlazlyn@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | SRF: Fix offline CPU preventing pc6 entry | expand |
On 11/8/24 04:29, Patryk Wlazlyn wrote: > The generic implementation, based on cpuid leaf 0x5, for looking up the > mwait hint for the deepest cstate, depends on them to be continuous in > range [0, NUM_SUBSTATES-1]. While that is correct on most Intel x86 > platforms, it is not architectural and may not result in reaching the > most optimized idle state on some of them. > > Prefer cpuidle_play_dead() over the generic mwait_play_dead() loop and > fallback to the later in case of missing enter_dead() handler. I don't think the bug has anything to do with this patch, really. There's no need to rehash it here. The issue here is that the only way to call mwait today is via mwait_play_dead() directly, using its internally-calculated hint. What you want is for a cpuidle-driver-calculated hint to be used. So, you're using the new hint function via the cpuidle driver. But you just need the cpuidle driver to be called first, not the old mwait_play_dead()-calculated hint. The new code will still do mwait, just via a different path and with a different hint. The thing this doesn't mention is what the impact on everyone else is. I _think_ the ACPI cpuidle driver is the only worry. Today, if there's a system that supports mwait and ACPI cpuidle, it'll use mwait. After this patch, it'll use ACPI cpuidle. The changelog doesn't mention that behavior change or why that's OK. Also, looking at this: > - mwait_play_dead(); > if (cpuidle_play_dead()) > - hlt_play_dead(); > + mwait_play_dead(); > + hlt_play_dead(); None of those return on success, right? Is there any reason this couldn't just be: /* The first successful play_dead() will not return: */ cpuidle_play_dead(); mwait_play_dead(); hlt_play_dead(); That has the added bonus of not needing to return anything from mwait_play_dead() and the resulting churn from the last patch.
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c index 44c40781bad6..721bb931181c 100644 --- a/arch/x86/kernel/smpboot.c +++ b/arch/x86/kernel/smpboot.c @@ -1416,9 +1416,9 @@ void native_play_dead(void) play_dead_common(); tboot_shutdown(TB_SHUTDOWN_WFS); - mwait_play_dead(); if (cpuidle_play_dead()) - hlt_play_dead(); + mwait_play_dead(); + hlt_play_dead(); } #else /* ... !CONFIG_HOTPLUG_CPU */
The generic implementation, based on cpuid leaf 0x5, for looking up the mwait hint for the deepest cstate, depends on them to be continuous in range [0, NUM_SUBSTATES-1]. While that is correct on most Intel x86 platforms, it is not architectural and may not result in reaching the most optimized idle state on some of them. Prefer cpuidle_play_dead() over the generic mwait_play_dead() loop and fallback to the later in case of missing enter_dead() handler. Signed-off-by: Patryk Wlazlyn <patryk.wlazlyn@linux.intel.com> --- arch/x86/kernel/smpboot.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)