[PATCH] x86/mwait_idle: Allow setting the max cstate to C1

[PATCH] x86/mwait_idle: Allow setting the max cstate to C1

[Ross Lagerwall]

From: Ross Lagerwall <rosslagerwall@citrix.com>

Following 91413b519631 ("x86/mwait_idle: export both C1 and C1E"), when
setting the max cstate to C1, the C1E cstate is used as well. This is
because MWAIT_HINT2CSTATE returns the same value for C1 and C1E.
Instead, when limiting the cstate, compare max_cstate with the position
in the states array, as the acpi cpu_idle driver does.

Without this patch, there's no way of setting the max cstate to C1 when using
the mwait_idle driver.

Signed-off-by: Ross Lagerwall <rosslagerwall@citrix.com>
---
 xen/arch/x86/cpu/mwait-idle.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/mwait-idle.c b/xen/arch/x86/cpu/mwait-idle.c
index 72a7abf..e4cfc16 100644
--- a/xen/arch/x86/cpu/mwait-idle.c
+++ b/xen/arch/x86/cpu/mwait-idle.c
@@ -328,11 +328,9 @@ static void mwait_idle(void)
 
 	if (max_cstate > 0 && power && !sched_has_urgent_vcpu() &&
 	    (next_state = cpuidle_current_governor->select(power)) > 0) {
-		do {
-			cx = &power->states[next_state];
-		} while (cx->type > max_cstate && --next_state);
-		if (!next_state)
-			cx = NULL;
+		cx = &power->states[next_state];
+		if (cx->idx > max_cstate)
+			cx = &power->states[max_cstate];
 		menu_get_trace_data(&exp, &pred);
 	}
 	if (!cx) {
@@ -585,6 +583,7 @@ static int mwait_idle_cpu_init(struct notifier_block *nfb,
 			setup_clear_cpu_cap(X86_FEATURE_TSC_RELIABLE);
 
 		cx = dev->states + dev->count;
+		cx->idx = dev->count;
 		cx->type = state;
 		cx->address = hint;
 		cx->entry_method = ACPI_CSTATE_EM_FFH;
-- 
1.9.3

Re: [PATCH] x86/mwait_idle: Allow setting the max cstate to C1

[Jan Beulich]

>>> On 02.06.14 at 16:43, <ross.lagerwall@citrix.com> wrote:
> From: Ross Lagerwall <rosslagerwall@citrix.com>
> 
> Following 91413b519631 ("x86/mwait_idle: export both C1 and C1E"), when
> setting the max cstate to C1, the C1E cstate is used as well. This is
> because MWAIT_HINT2CSTATE returns the same value for C1 and C1E.
> Instead, when limiting the cstate, compare max_cstate with the position
> in the states array, as the acpi cpu_idle driver does.
> 
> Without this patch, there's no way of setting the max cstate to C1 when using
> the mwait_idle driver.

But it was intentionally this way from the beginning of the existence of
the mwait idle driver - the other approach makes the value to be passed
really platform dependent (i.e. "max_cstate=2" doesn't universally mean
what one would expect: maximum C-state is C2).

But I recognize the need to disable all possible (intermediate) levels; I
just think this should be done via extending max_cstate= (e.g. by
allowing for a second number, which would then indicate the maximum
sub-state).

Jan

Re: [PATCH] x86/mwait_idle: Allow setting the max cstate to C1

[Ross Lagerwall]

On 06/02/2014 04:46 PM, Jan Beulich wrote:
>>>> On 02.06.14 at 16:43, <ross.lagerwall@citrix.com> wrote:
>> From: Ross Lagerwall <rosslagerwall@citrix.com>
>>
>> Following 91413b519631 ("x86/mwait_idle: export both C1 and C1E"), when
>> setting the max cstate to C1, the C1E cstate is used as well. This is
>> because MWAIT_HINT2CSTATE returns the same value for C1 and C1E.
>> Instead, when limiting the cstate, compare max_cstate with the position
>> in the states array, as the acpi cpu_idle driver does.
>>
>> Without this patch, there's no way of setting the max cstate to C1 when using
>> the mwait_idle driver.
>
> But it was intentionally this way from the beginning of the existence of
> the mwait idle driver - the other approach makes the value to be passed
> really platform dependent (i.e. "max_cstate=2" doesn't universally mean
> what one would expect: maximum C-state is C2).

Except that is how xenpm and xl debugkeys c already display their 
output. E.g:
C0                   : transition [             3431722]
                        residency  [              131101 ms]
C1                   : transition [                 588]
                        residency  [                3514 ms]
C2                   : transition [                 465]
                        residency  [                 497 ms]
C3                   : transition [                 176]
                        residency  [                 299 ms]
C4                   : transition [                  15]
                        residency  [                   5 ms]
C5                   : transition [             3430478]
                        residency  [            57685073 ms]

In the above, C1 == hardware C1 and C2 == hardware C1E.  Changing it so 
that "xenpm set-max-cstate 1" sets it to xenpm's notion of C1 rather 
than actual C1 (as the patch does) seems consistent to me.

The alternative would be to fix up xenpm, xl debugkeys c, and any other 
consumers of C-states to correctly display substates and take a new 
substate parameter.  IMHO, there's little gain in doing this as C-states 
are really platform dependent anyway.  If the next Intel processor has a 
selectable sub-sub-C-state, do we really want to have to change 
everything again?

Cheers
-- 
Ross Lagerwall

Re: [PATCH] x86/mwait_idle: Allow setting the max cstate to C1

[Jan Beulich]

>>> Ross Lagerwall <ross.lagerwall@citrix.com> 06/18/14 10:13 AM >>>
>On 06/02/2014 04:46 PM, Jan Beulich wrote:
>>>>> On 02.06.14 at 16:43, <ross.lagerwall@citrix.com> wrote:
>>> From: Ross Lagerwall <rosslagerwall@citrix.com>
>>>
>>> Following 91413b519631 ("x86/mwait_idle: export both C1 and C1E"), when
>>> setting the max cstate to C1, the C1E cstate is used as well. This is
>>> because MWAIT_HINT2CSTATE returns the same value for C1 and C1E.
>>> Instead, when limiting the cstate, compare max_cstate with the position
>>> in the states array, as the acpi cpu_idle driver does.
>>>
>>> Without this patch, there's no way of setting the max cstate to C1 when using
>>> the mwait_idle driver.
>>
>> But it was intentionally this way from the beginning of the existence of
>> the mwait idle driver - the other approach makes the value to be passed
>> really platform dependent (i.e. "max_cstate=2" doesn't universally mean
>> what one would expect: maximum C-state is C2).
>
>Except that is how xenpm and xl debugkeys c already display their 
>output. E.g:
>C0                   : transition [             3431722]
>residency  [              131101 ms]
>C1                   : transition [                 588]
>residency  [                3514 ms]
>C2                   : transition [                 465]
>residency  [                 497 ms]
>C3                   : transition [                 176]
>residency  [                 299 ms]
>C4                   : transition [                  15]
>residency  [                   5 ms]
>C5                   : transition [             3430478]
>residency  [            57685073 ms]
>
>In the above, C1 == hardware C1 and C2 == hardware C1E.  Changing it so 
>that "xenpm set-max-cstate 1" sets it to xenpm's notion of C1 rather 
>than actual C1 (as the patch does) seems consistent to me.

And I always considered it wrong for it to confuse things like this. Nevertheless
I agree that the utility's output and accepted input ought to be the same. Yet I'm
getting the impression that you forget that there's also a hypervisor command
line option to consider here, and that definitely ought to reflect reality, not some
tool's questionable view of the world.

>The alternative would be to fix up xenpm, xl debugkeys c, and any other 
>consumers of C-states to correctly display substates and take a new 
>substate parameter.  IMHO, there's little gain in doing this as C-states 
>are really platform dependent anyway.  If the next Intel processor has a 
>selectable sub-sub-C-state, do we really want to have to change 
>everything again?

Yes, I think this would be the right direction - far better than using confusing naming.

Jan