Re: [PATCH v2 2/3] x86/smp: Allow forcing the mwait hint for play dead loop

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

On 10/30/24 06:33, Patryk Wlazlyn wrote:
>>> +void smp_set_mwait_play_dead_hint(unsigned int hint)
>>> +{
>>> +    WRITE_ONCE(play_dead_mwait_hint, hint);
>>> +}
>>
>> This all feels a bit hacky and unstructured to me.
>>
>> Could we at least set up a few rules here?  Like, say what the hints
>> are, what values can they have?  Where do they come from?  Can this get
>> called more than once?  Does it _need_ to be set?  What's the behavior
>> when it is not set?  Who is responsible for calling this?
> 
> The other idea is to first check if currently loaded idle driver provides
> enter_dead() callback first and leave the current, deepest mwait hint
> computation code as a fallback.
> 
> Does that sound less hacky?

Yes.

> Unfortunately, it comes with a little problem. In case of kexec, we need to
> have a way to exit from the mwait loop and enter hlt to prevent offlined CPU
> from crashing when the old memory is being overwritten.

Why is this a problem?  Like I mentioning to Rafael, just *call*
mwait_play_dead() from the idle driver.

mwait_play_dead() could probably also use some refactoring because it
has 3 pieces:

	1. Should the code run at all or defer to another play dead
	   implementation?
	2. Calculating the hint (obviously not needed in your new case)
	3. Actually running mwait (including the kexec hack)

The "should the code run?" bit is superfluous but harmless if called
from the idle driver.  Ditto on the hint calculation, but you already
factored it out.  That leaves the "actually run mwait" bit which you
100% need.

> I think, we can solve it by bringing the CPU back online before we proceed
> with kexec, but I would appreciate some feedback from someone who is more
> familiar with kexec, before merging that.
> 
> We may also signal that by touching the resched flag on which enter_dead()
> code will monitor in case of mwait and enter hlt right after, but that's a
> bit hackier IMO.

That route is also fine with me, but I'm not sure it's necessary.

>> What good does the smp_ prefix do?  I don't think _callers_ care whether
>> this is getting optimized out or not.
> 
> The prefix makes it a little bit cleaner by not exporting new global symbol
> with "set_mwait_play_dead_hint" name.

I'm not following.