Re: [PATCH 0/3] idle, Honor Hardware Disabled States

[Prarit Bhargava]

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:
>>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=109081
>>
>> 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.
> 

Len,

Your patch does

+	skl_cstates[5].disabled = 1;	/* C8-SKL */
+	skl_cstates[6].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/state0/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state1/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state2/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state3/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state4/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state5/disable:1:0
    /sys/devices/system/cpu/cpu0/cpuidle/state6/disable:1:0
    /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
your change.

P.


> thanks,
> -Len
>