[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <570B8C7E.2030504@redhat.com>
Date: Mon, 11 Apr 2016 07:37:34 -0400
From: Prarit Bhargava <prarit@...hat.com>
To: Len Brown <lenb@...nel.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
X86 ML <x86@...nel.org>,
"Peter Zijlstra (Intel)" <peterz@...radead.org>,
Borislav Petkov <bp@...e.de>, Andi Kleen <ak@...ux.intel.com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Dasaratharaman Chandramouli
<dasaratharaman.chandramouli@...el.com>,
Linux PM list <linux-pm@...r.kernel.org>
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@...p40-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
>
Powered by blists - more mailing lists