Re: [PATCH v3 1/3] x86: Allow limiting the max C-state sub-state

[Ross Lagerwall <ross.lagerwall@citrix.com>]

On 06/25/2014 01:37 PM, Jan Beulich wrote:
>>>> On 23.06.14 at 13:09, <ross.lagerwall@citrix.com> 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