From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ross Lagerwall Subject: Re: [PATCH v3 1/3] x86: Allow limiting the max C-state sub-state Date: Mon, 7 Jul 2014 16:14:05 +0100 Message-ID: <53BAB93D.7030805@citrix.com> References: <1403521791-28859-1-git-send-email-ross.lagerwall@citrix.com> <1403521791-28859-2-git-send-email-ross.lagerwall@citrix.com> <53AADEC4020000780001D362@mail.emea.novell.com> <53AAF023.7040108@citrix.com> <53AC3E76020000780001DB42@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53AC3E76020000780001DB42@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Liu Jinsong , Xen-devel List-Id: xen-devel@lists.xenproject.org On 06/26/2014 02:38 PM, Jan Beulich wrote: >>>> On 25.06.14 at 17:52, wrote: >> On 06/25/2014 01:37 PM, Jan Beulich wrote: >>>>>> On 23.06.14 at 13:09, wrote: >>>> --- a/xen/arch/x86/cpu/mwait-idle.c >>>> +++ b/xen/arch/x86/cpu/mwait-idle.c >>>> @@ -330,7 +330,9 @@ static void mwait_idle(void) >>>> (next_state = cpuidle_current_governor->select(power)) > 0) { >>>> do { >>>> cx = &power->states[next_state]; >>>> - } while (cx->type > max_cstate && --next_state); >>>> + } while ((cx->type > max_cstate || (cx->type == max_cstate && >>>> + MWAIT_HINT2SUBSTATE(cx->address) > max_csubstate)) && >>>> + --next_state); >>> >>> In the context of the above comment it then is questionable >>> whether here (and similarly in acpi_processor_idle()) using the >>> MWAIT parameter value for the comparison here is really >>> suitable: If you look at hsw_cstates[] and atom_cstates[] you'll >>> see that there we have states with just a single non-zero sub- >>> state (which the logic here would exclude in certain cases when >>> one would expect it to be permitted). >>> >> >> When would one expect them to be permitted that this logic would exclude? >> >> C7s-HSW has a C-state of 4 and a sub-state of 2. If you set max_cstate >> = 4, then no C-state > 4 will be selected. >> Similarly, if you select max_csubstate = 2, then no sub C-state > 2 will >> be selected (if max_cstate = 4). This seems congruous to me. > > Actually I think I got it the wrong way round: Taking your example, if > one sets max_csubstate = 1, one may expect the 1st (not necessarily > the one numbered "1") to still be permitted (much like we also don't tie > max_cstate to actual values to be passed to MWAIT). Just look at how > much more interesting the sub-states get with the ports from recent > Linux that I posted earlier today. > AFAICT, max_cstate _is_ tied to the actual values passed to MWAIT. For example, take Haswell's C9-HSW state: it has an MWAIT flag value of 0x50 which gets assigned as cx->type = 6 and subsequently compared with max_cstate. max_csubstate is implemented in the same way as max_cstate. However, given the states in the Bay Trail patch (specifically C6N-BYT and C6S-BYT), this is clearly insufficient. How do you think max_csubstate should limit the substate? Should it be something like a max_csubstate = 2 permits the first and second substates? Regards -- Ross Lagerwall