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

From: "Jan Beulich" <JBeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: xen-devel <xen-devel@lists.xenproject.org>,
	Wei Liu <wei.liu2@citrix.com>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: Ping: [PATCH 0/5] x86: more power-efficient CPU parking
Date: Wed, 03 Apr 2019 09:20:44 -0600	[thread overview]
Message-ID: <5CA4CF4C020000780022453D@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <d350614d-e3ec-5962-af7a-2e4acc0682d8@citrix.com>

>>> On 03.04.19 at 16:44, <andrew.cooper3@citrix.com> wrote:
> On 03/04/2019 13:43, Jan Beulich wrote:
>>>>> On 03.04.19 at 13:14, <andrew.cooper3@citrix.com> 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?
> 
> It was an assertion failure when the CPU failed to call into the SMM
> rendezvous.
> 
> LogLibaErrorLogSmmLib.c(276): ((BOOLEAN)(0==1))
> 
> This is a production Dell system IIRC (or maybe Supermicro, but either
> way, a production firmware).
> 
> In retrospect, fully offlining a CPU behind the back of the firmware is
> an extremely antisocial thing to do, and I'm not surprised that the
> firmware doesn't tolerate it.

Oh, I see. I withdraw this patch then.

>> 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.
> 
> I think that would be a good idea, along with a similar speculative
> adjustment.

Will do.

>> 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).
> 
> I really don't think that is a concern.  I don't think you'll be able to
> measure the difference in the noise.

Well, not over any extended period of time. But a single WBINVD
may be quite noticeable to at least the other thread while it
executes.

Jan

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