Re: [PATCH v3 2/3] x86/smp native_play_dead: Prefer cpuidle_play_dead() over mwait_play_dead()

[Dave Hansen <dave.hansen@intel.com>]

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.