Messages in this thread | | | Date | Mon, 11 Apr 2016 07:37:34 -0400 | From | Prarit Bhargava <> | Subject | Re: [PATCH 0/3] idle, Honor Hardware Disabled States |
| |
On 03/31/2016 12:59 AM, Len Brown wrote: >> 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. > > For https://bugzilla.kernel.org/show_bug.cgi?id=109081 > it is correct. > >> 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. > > ENOPARSE. > Are we talking about the failure in > https://bugzilla.kernel.org/show_bug.cgi?id=109081 > or a different problem? >
I see now. There are two bugs. The first is your case where the deep c-states should be disabled but are not disabled, and the my case where the states *are* disabled but not indicated as such in sysfs.
>> >> 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 'disabled' attribute you see in sysfs is not > struct cpuidle_state.disabled > it is > struct cpuidle_state_usage.disabled
Yes. I know that. But the problem is that your patch should take that into account.
> >> 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. > > Do you see this debug message when you run current upstream on this hardware? > > /* if state marked as disabled, skip it */ > if (cpuidle_state_table[cstate].disabled != 0) { > pr_debug(PREFIX "state %s is disabled", > cpuidle_state_table[cstate].name); > continue; > } > > > If no, then my patch is not disabling C8/C9 on your system. > > Also, if it were, the code above causes the states to not appear > at all in sysfs, because they are not registered. > > Re: MSR_NHM_SNB_PKG_CST_CFG_CTL > > if PC10 is disabled there, then functionally, it doesn't matter what we do, > which is why my patch does nothing when PC10 is disabled. > > In such a scenario, pc10 presence in sysfs (and cpufreq) > is cosmetic. The hardware knows what to do. > > Do you think that cosmetic issue is worth dealing with?
The bug reported is that the system will not go into the deep c-states. This is because the c-states are disabled by hardware. This means that turbostat shows only transitions to C7, while sysfs (and the monitoring software) indicates deeper c-states are available.
So yes, I do think that it is worth dealing with.
> Note that the decoding of that MSR changes with every CPU,
:( Ugh ... I know :(. I just decoded it.
> so to get it right (like turbostat does), we'd need a table. > Also, it would be useful only for states which are PC states only. > ie. we can't disable CC7 just because PC7 is disabled. etc. > So you could remove PC8, PC9, PC10 from sysfs on SKL > when they are disabled, but that is all. >
P.
> thanks, > Len Brown, Intel Open Source Technology Center >
| |