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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel