Re: [PATCH 0/3] idle, Honor Hardware Disabled States
From: Prarit Bhargava
Date: Mon Mar 28 2016 - 10:48:50 EST
On 03/24/2016 05:54 PM, Len Brown wrote:
> On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <prarit@xxxxxxxxxx> wrote:
>> On 03/23/2016 04:05 PM, Len Brown wrote:
>>> This patch assumes that if a package state is disabled,
>>> the corresponding core state must be disabled.
>>> That assumption is false.
>>> Indeed, that is a very popular and useful configuration.
>>> But even if that were not the case, this software is not necessary,
>>> since the hardware handles demotion "c-state clipping" automatically.
>>> Yes, there is a case where a certain version of a certain processor
>>> has broken demotion, but this isn't the right fix for that.
>>> The right fix for that is here:
>> Len, should I rebase on top of this? Would that work for you?
> I guess I wasn't clear.
> I don't see the benefit of your patch.
> Please explain it to me.
Your patch does
+ skl_cstates.disabled = 1; /* C8-SKL */
+ skl_cstates.disabled = 1; /* C9-SKL */
and I don't think that is correct for SKY-H.
Your patch does not take into account that the states are explicitly disabled
in MSR_NHM_SNB_PKG_CST_CFG_CTL. That is the problem here and what you've done
is simply hammered a disable into those states.
Additionally, your patch does not show the user the correct state information:
[root@dhcp40-125 ~]# egrep ^ /sys/devices/system/cpu/cpu0/cpuidle/state?/disable
/sys/devices/system/cpu/cpu0/cpuidle/state7/disable:1:0 << should be 1
/sys/devices/system/cpu/cpu0/cpuidle/state8/disable:1:0 << should be 1
The fix is to honour the settings in MSR_NHM_SNB_PKG_CST_CFG_CTL. I cannot say
for certain that ALL SKY-H are impacted (you are admittedly in better position
to say so or not). I can say that on the 2 systems tested here the
MSR_NHM_SNB_PKG_CST_CFG_CTL do have the appropriate disable value set.
/me could be missing some important info -- again, perhaps there are some
SKY-H's out there that do not have states disabled in
MSR_NHM_SNB_PKG_CST_CFG_CTL, and that's why I've proposed rebasing on top of