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

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

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