Re: [Xen-devel] Ping: [PATCH 0/5] x86: more power-efficient CPU parking

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 03.04.19 at 13:14, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 03/04/2019 11:12, Jan Beulich wrote:
>>>>> On 01.08.18 at 16:22,  wrote:
>>> When putting CPUs to sleep permanently, we should try to put them into
>>> the most power conserving state possible. For now it is unclear whether,
>>> especially in a deep C-state, the P-state also matters, so this series only
>>> arranges for the C-state side of things (plus some cleanup).
>>>
>>> 1: x86/cpuidle: replace a pointless NULL check
>>> 2: x86/idle: re-arrange dead-idle handling
>>> 3: x86/cpuidle: push parked CPUs into deeper sleep states when possible
>>> 4: x86/cpuidle: clean up Cx dumping
>>> 5: x86: place non-parked CPUs into wait-for-SIPI state after offlining
>> So patch 5 is understandably controversial, and I'm explicitly
>> excluding it from the ping.
> 
> Considering that it causes EFI firmware to explode in several
> interesting ways, I'm afraid it is a complete nonstarter.

I didn't know this - neither of my two EFI boxes have exploded in
any way during the last half year. Care to share details?

>> Patch 1 has gone in long ago and
>> patch 4 has been acked. While there was some feedback on
>> 2 (albeit stalled then after my reply), I don't think I've had any
>> substantial feedback on 3 though, yet _they_ are supposed to
>> provide the main improvement here. Patch 5 really was meant
>> to be more optional than I had expressed, hence its placement
>> even after the pure cleanup patch 4.
> 
> Some of the patches have cycled out of my inbox, so I'm reviewing from
> the mail archive.
> 
> 2) Now that default_dead_idle() isn't a terminal function, you must use
> spec_ctrl_enter_idle() on the way back out for safety.

Ah, yes, perhaps better to add it, than to implicitly rely on callers
(there's really exactly one) to be terminal. (I take it you mean
spec_ctrl_exit_idle().)

> I'm still going to insist on something about #MC, even if it is just a
> note in the commit message saying "this doesn't yet make #MC any safer
> for parked CPUs".

I can add something like this, but what is it that's known unsafe
for parked CPUs? They in particular retain their per-CPU data. If
anything I see a problem with fully offline CPUs. For now I'll add
a similar sentence, but with "parked" replaced.

One other question (I apparently forgot about this aspect
between putting together the series and posting it):
acpi_dead_idle() has built-in loops as well. While it's not
expected for a CPU to need waking from there (as no "even
better" dead-idle handler could get installed) I wonder whether
for consistency we wouldn't better drop the loops there too.
The downside of doing so would be added overhead in case
of spurious wakeups (which ought to have a small chance of
being possible in particular in the MWAIT case).

> 3) Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Thanks!

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel