On 03/24/2016 05:54 PM, Len Brown wrote: > On Wed, Mar 23, 2016 at 7:50 PM, Prarit Bhargava <pra...@redhat.com> 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 >