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: Wed, 25 Jun 2014 16:52:03 +0100 Message-ID: <53AAF023.7040108@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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <53AADEC4020000780001D362@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/25/2014 01:37 PM, Jan Beulich wrote: >>>> On 23.06.14 at 13:09, wrote: >> Allow limiting the max C-state sub-state by appending to the max_cstate >> command-line parameter. E.g. max_cstate=1,0 >> The limit only applies to the highest legal C-state. For example: >> max_cstate = 1, max_csubstate = 0 ==> C0, C1 okay, but not C1E >> max_cstate = 1, max_csubstate = 1 ==> C0, C1 and C1E okay, but not C2 >> max_cstate = 2, max_csubstate = 0 ==> C0, C1, C1E, C2 okay, but not C3 >> max_cstate = 2, max_csubstate = 1 ==> C0, C1, C1E, C2 okay, but not C3 > > While from an abstract perspective this looks okay to me now, I'm > afraid the description, which is also being put into the header file, is > possibly misleading: Neither is the first sub-state of C1 necessarily > C1E, nor is it excluded that C2 and higher also have sub-states (yet > the last of the examples sort of suggests that). The comment was meant to clarify how max_cstate and max_csubstate work by means of an example from a real machine. I don't think it suggests that the C-states used in the example are necessarily what one would find on a real machine. I could make the example more abstract, but I don't think that would be helpful. > >> --- 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. Regards -- Ross Lagerwall