[PATCH 1/1] Honor state disabling in the cpuidle ladder governor

Discussion:

Carsten Emde

2012-07-17 18:59:15 UTC

Deepthi Dharwar

2012-07-18 06:36:03 UTC

Post by Carsten Emde
There are two cpuidle governors ladder and menu. While the ladder
governor is always available, if CONFIG_CPU_IDLE is selected, the
menu governor additionally requires CONFIG_NO_HZ.
A particular C state can be disabled by writing to the sysfs file
/sys/devices/system/cpu/cpuN/cpuidle/stateN/disable, but this mechanism
is only implemented in the menu governor. Thus, in a system where
CONFIG_NO_HZ is not selected, the ladder governor becomes default and
always will walk through all sleep states - irrespective of whether the
C state was disabled via sysfs or not. The only way to select a specific
C state was to write the related latency to /dev/cpu_dma_latency and
keep the file open as long as this setting was required - not very
practical and not suitable for setting a single core in an SMP system.
With this patch, the ladder governor only will promote to the next
C state, if it has not been disabled, and it will demote, if the
current C state was disabled.

Yes, I agree that currently that disabling a particular C-state
is not reflected in working of ladder governor. This patch is needed
to fix it on ladder too.

Also wanted to clarify on the intended implementation here,
if there are say 5 C-states on a system, disabling 2nd
state would also end by disabling all the remaining 3 deeper states too
as ladder governor enters the lightest state first, and will only move
on to the next deeper state if a idle period was long enough as
per the implementation.
If one is disabling only the deepest state, then it would
work as intended.

Cheers,
Deepthi

Post by Carsten Emde
---
drivers/cpuidle/governors/ladder.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
Index: linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/drivers/cpuidle/governors/ladder.c
+++ linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
@@ -88,6 +88,7 @@ static int ladder_select_state(struct cp
/* consider promotion */
if (last_idx < drv->state_count - 1 &&
+ !drv->states[last_idx + 1].disable &&
last_residency > last_state->threshold.promotion_time &&
drv->states[last_idx + 1].exit_latency <= latency_req) {
last_state->stats.promotion_count++;
@@ -100,7 +101,8 @@ static int ladder_select_state(struct cp
/* consider demotion */
if (last_idx > CPUIDLE_DRIVER_STATE_START &&
- drv->states[last_idx].exit_latency > latency_req) {
+ (drv->states[last_idx].disable ||
+ drv->states[last_idx].exit_latency > latency_req)) {
int i;
for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) {

Deepthi Dharwar

2012-07-19 11:14:24 UTC

Permalink

Post by Deepthi Dharwar

Yes, I agree that currently that disabling a particular C-state
is not reflected in working of ladder governor. This patch is needed
to fix it on ladder too.
Also wanted to clarify on the intended implementation here,
if there are say 5 C-states on a system, disabling 2nd
state would also end by disabling all the remaining 3 deeper states too
as ladder governor enters the lightest state first, and will only move
on to the next deeper state if a idle period was long enough as
per the implementation.
If one is disabling only the deepest state, then it would
work as intended.

Yes, the patch does not make the setting of the sysfs variable
"disable" coherent, i.e. if one is disabling a light state, then all
deeper states are disabled as well, but the "disable" variable does not
reflect it. Likewise, if one enables a deep state but a lighter state
still is disabled, then this has no effect.

Agree, as per the ladder design.

I could implement a sanitize mechanism of the ladder governor that
takes care the "disable" variables of all deeper states are set to 1,
if a state is disabled, and those of all lighter states are set to 0,
if a state is enabled. Do you wish me to do that?

No, I dont think thats necessary, current code suffices it.
The disable flag is knob we are giving to the user . So may be just
document the intended use of disable flag working
alongside design of ladder governor.

It's not necessary - but maybe better. Here comes v3 with a sanitizer.
Is this too ugly?

The v2, with the documentation in place seems sufficient.
Yup, this adds unnecessary fields which are not much use
coz the same can be achieved with just disable flag check.

Also, any reason why the patch is being sent as an attachment ?
Sending patches as an attachment is not a recommended practice.

Cheers,
Deepthi

-Carsten.

Carsten Emde

2012-07-19 11:39:31 UTC

Permalink

Deepthi,

Post by Deepthi Dharwar

[..]

It's not necessary - but maybe better. Here comes v3 with a sanitizer.
Is this too ugly?

The v2, with the documentation in place seems sufficient.
Yup, this adds unnecessary fields which are not much use
coz the same can be achieved with just disable flag check.

ok, let's take v2.

Post by Deepthi Dharwar
Also, any reason why the patch is being sent as an attachment ?
Sending patches as an attachment is not a recommended practice.

Sorry.

-Carsten.

Rafael J. Wysocki

2012-07-19 18:42:57 UTC

Permalink

Hi,

Post by Carsten Emde
Deepthi,

Post by Deepthi Dharwar

[..]

It's not necessary - but maybe better. Here comes v3 with a sanitizer.
Is this too ugly?

The v2, with the documentation in place seems sufficient.
Yup, this adds unnecessary fields which are not much use
coz the same can be achieved with just disable flag check.

ok, let's take v2.

Post by Deepthi Dharwar
Also, any reason why the patch is being sent as an attachment ?
Sending patches as an attachment is not a recommended practice.

Sorry.

Can you please resend the version regarded as the current one?

And please send it inline this time.

Thanks,
Rafael

Carsten Emde

2012-07-19 18:52:12 UTC

Permalink

Rafael,

Post by Rafael J. Wysocki

Post by Carsten Emde

Post by Deepthi Dharwar

[..]
I could implement a sanitize mechanism of the ladder governor that
takes care the "disable" variables of all deeper states are set to 1,
if a state is disabled, and those of all lighter states are set to 0,
if a state is enabled. Do you wish me to do that?

It's not necessary - but maybe better. Here comes v3 with a sanitizer.
Is this too ugly?

The v2, with the documentation in place seems sufficient.
Yup, this adds unnecessary fields which are not much use
coz the same can be achieved with just disable flag check.

ok, let's take v2.

Can you please resend the version regarded as the current one?

This is the version that was regarded as the current one (v2).

Changes in v2: A note in the documentation explains why the sysfs
variable "disable" may not always reflect the current situation
and why modifying it may not always work as expected.

-Carsten.

Carsten Emde

2012-07-19 18:52:13 UTC

Permalink

There are two cpuidle governors ladder and menu. While the ladder
governor is always available, if CONFIG_CPU_IDLE is selected, the
menu governor additionally requires CONFIG_NO_HZ.

A particular C state can be disabled by writing to the sysfs file
/sys/devices/system/cpu/cpuN/cpuidle/stateN/disable, but this mechanism
is only implemented in the menu governor. Thus, in a system where
CONFIG_NO_HZ is not selected, the ladder governor becomes default and
always will walk through all sleep states - irrespective of whether the
C state was disabled via sysfs or not. The only way to select a specific
C state was to write the related latency to /dev/cpu_dma_latency and
keep the file open as long as this setting was required - not very
practical and not suitable for setting a single core in an SMP system.

With this patch, the ladder governor only will promote to the next
C state, if it has not been disabled, and it will demote, if the
current C state was disabled.

Note that the patch does not make the setting of the sysfs variable
"disable" coherent, i.e. if one is disabling a light state, then all
deeper states are disabled as well, but the "disable" variable does not
reflect it. Likewise, if one enables a deep state but a lighter state
still is disabled, then this has no effect. A related section has been
added to the documentation.

Signed-off-by: Carsten Emde <***@osadl.org>

---
Documentation/cpuidle/sysfs.txt | 10 +++++++++-
drivers/cpuidle/governors/ladder.c | 4 +++-
2 files changed, 12 insertions(+), 2 deletions(-)

Index: linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/Documentation/cpuidle/sysfs.txt
+++ linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
@@ -76,9 +76,17 @@ total 0

* desc : Small description about the idle state (string)
-* disable : Option to disable this idle state (bool)
+* disable : Option to disable this idle state (bool) -> see note below
* latency : Latency to exit out of this idle state (in microseconds)
* name : Name of the idle state (string)
* power : Power consumed while in this idle state (in milliwatts)
* time : Total time spent in this idle state (in microseconds)
* usage : Number of times this state was entered (count)
+
+Note:
+The behavior and the effect of the disable variable depends on the
+implementation of a particular governor. In the ladder governor, for
+example, it is not coherent, i.e. if one is disabling a light state,
+then all deeper states are disabled as well, but the disable variable
+does not reflect it. Likewise, if one enables a deep state but a lighter
+state still is disabled, then this has no effect.
Index: linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/drivers/cpuidle/governors/ladder.c
+++ linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
@@ -88,6 +88,7 @@ static int ladder_select_state(struct cp

/* consider promotion */
if (last_idx < drv->state_count - 1 &&
+ !drv->states[last_idx + 1].disable &&
last_residency > last_state->threshold.promotion_time &&
drv->states[last_idx + 1].exit_latency <= latency_req) {
last_state->stats.promotion_count++;
@@ -100,7 +101,8 @@ static int ladder_select_state(struct cp

/* consider demotion */
if (last_idx > CPUIDLE_DRIVER_STATE_START &&
- drv->states[last_idx].exit_latency > latency_req) {
+ (drv->states[last_idx].disable ||
+ drv->states[last_idx].exit_latency > latency_req)) {
int i;

for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) {

Rafael J. Wysocki

2012-07-19 19:30:20 UTC

Permalink

Your patch doesn't seem to take this linux-next commit:

http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=commit;h=dc7fd275ae60ef8edf952aff2a62462f5d892fd4

into account, does it?

Rafael

Post by Carsten Emde
---
Documentation/cpuidle/sysfs.txt | 10 +++++++++-
drivers/cpuidle/governors/ladder.c | 4 +++-
2 files changed, 12 insertions(+), 2 deletions(-)
Index: linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/Documentation/cpuidle/sysfs.txt
+++ linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
@@ -76,9 +76,17 @@ total 0
* desc : Small description about the idle state (string)
-* disable : Option to disable this idle state (bool)
+* disable : Option to disable this idle state (bool) -> see note below
* latency : Latency to exit out of this idle state (in microseconds)
* name : Name of the idle state (string)
* power : Power consumed while in this idle state (in milliwatts)
* time : Total time spent in this idle state (in microseconds)
* usage : Number of times this state was entered (count)
+
+The behavior and the effect of the disable variable depends on the
+implementation of a particular governor. In the ladder governor, for
+example, it is not coherent, i.e. if one is disabling a light state,
+then all deeper states are disabled as well, but the disable variable
+does not reflect it. Likewise, if one enables a deep state but a lighter
+state still is disabled, then this has no effect.
Index: linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/drivers/cpuidle/governors/ladder.c
+++ linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
@@ -88,6 +88,7 @@ static int ladder_select_state(struct cp
/* consider promotion */
if (last_idx < drv->state_count - 1 &&
+ !drv->states[last_idx + 1].disable &&
last_residency > last_state->threshold.promotion_time &&
drv->states[last_idx + 1].exit_latency <= latency_req) {
last_state->stats.promotion_count++;
@@ -100,7 +101,8 @@ static int ladder_select_state(struct cp
/* consider demotion */
if (last_idx > CPUIDLE_DRIVER_STATE_START &&
- drv->states[last_idx].exit_latency > latency_req) {
+ (drv->states[last_idx].disable ||
+ drv->states[last_idx].exit_latency > latency_req)) {
int i;
for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) {

Carsten Emde

2012-07-19 20:34:10 UTC

Permalink

There are two cpuidle governors ladder and menu. While the ladder
governor is always available, if CONFIG_CPU_IDLE is selected, the
menu governor additionally requires CONFIG_NO_HZ.

A particular C state can be disabled by writing to the sysfs file
/sys/devices/system/cpu/cpuN/cpuidle/stateN/disable, but this mechanism
is only implemented in the menu governor. Thus, in a system where
CONFIG_NO_HZ is not selected, the ladder governor becomes default and
always will walk through all sleep states - irrespective of whether the
C state was disabled via sysfs or not. The only way to select a specific
C state was to write the related latency to /dev/cpu_dma_latency and
keep the file open as long as this setting was required - not very
practical and not suitable for setting a single core in an SMP system.

With this patch, the ladder governor only will promote to the next
C state, if it has not been disabled, and it will demote, if the
current C state was disabled.

Note that the patch does not make the setting of the sysfs variable
"disable" coherent, i.e. if one is disabling a light state, then all
deeper states are disabled as well, but the "disable" variable does not
reflect it. Likewise, if one enables a deep state but a lighter state
still is disabled, then this has no effect. A related section has been
added to the documentation.

Signed-off-by: Carsten Emde <***@osadl.org>

---
Documentation/cpuidle/sysfs.txt | 10 +++++++++-
drivers/cpuidle/governors/ladder.c | 4 +++-
2 files changed, 12 insertions(+), 2 deletions(-)

Index: linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/Documentation/cpuidle/sysfs.txt
+++ linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
@@ -76,9 +76,17 @@ total 0

* desc : Small description about the idle state (string)
-* disable : Option to disable this idle state (bool)
+* disable : Option to disable this idle state (bool) -> see note below
* latency : Latency to exit out of this idle state (in microseconds)
* name : Name of the idle state (string)
* power : Power consumed while in this idle state (in milliwatts)
* time : Total time spent in this idle state (in microseconds)
* usage : Number of times this state was entered (count)
+
+Note:
+The behavior and the effect of the disable variable depends on the
+implementation of a particular governor. In the ladder governor, for
+example, it is not coherent, i.e. if one is disabling a light state,
+then all deeper states are disabled as well, but the disable variable
+does not reflect it. Likewise, if one enables a deep state but a lighter
+state still is disabled, then this has no effect.
Index: linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/drivers/cpuidle/governors/ladder.c
+++ linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
@@ -88,6 +88,7 @@ static int ladder_select_state(struct cp

/* consider promotion */
if (last_idx < drv->state_count - 1 &&
+ !dev->states_usage[last_idx + 1].disable &&
last_residency > last_state->threshold.promotion_time &&
drv->states[last_idx + 1].exit_latency <= latency_req) {
last_state->stats.promotion_count++;
@@ -100,7 +101,8 @@ static int ladder_select_state(struct cp

/* consider demotion */
if (last_idx > CPUIDLE_DRIVER_STATE_START &&
- drv->states[last_idx].exit_latency > latency_req) {
+ (dev->states_usage[last_idx].disable ||
+ drv->states[last_idx].exit_latency > latency_req)) {
int i;

for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) {

Rafael J. Wysocki

2012-07-19 21:48:05 UTC

Permalink

This looks fine to me, but it's too late for v3.6. I can queue it up
for v3.7, though.

Thanks,
Rafael

Post by Carsten Emde
---
Documentation/cpuidle/sysfs.txt | 10 +++++++++-
drivers/cpuidle/governors/ladder.c | 4 +++-
2 files changed, 12 insertions(+), 2 deletions(-)
Index: linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/Documentation/cpuidle/sysfs.txt
+++ linux-3.4.4-rt14-rc2-64/Documentation/cpuidle/sysfs.txt
@@ -76,9 +76,17 @@ total 0
* desc : Small description about the idle state (string)
-* disable : Option to disable this idle state (bool)
+* disable : Option to disable this idle state (bool) -> see note below
* latency : Latency to exit out of this idle state (in microseconds)
* name : Name of the idle state (string)
* power : Power consumed while in this idle state (in milliwatts)
* time : Total time spent in this idle state (in microseconds)
* usage : Number of times this state was entered (count)
+
+The behavior and the effect of the disable variable depends on the
+implementation of a particular governor. In the ladder governor, for
+example, it is not coherent, i.e. if one is disabling a light state,
+then all deeper states are disabled as well, but the disable variable
+does not reflect it. Likewise, if one enables a deep state but a lighter
+state still is disabled, then this has no effect.
Index: linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
===================================================================
--- linux-3.4.4-rt14-rc2-64.orig/drivers/cpuidle/governors/ladder.c
+++ linux-3.4.4-rt14-rc2-64/drivers/cpuidle/governors/ladder.c
@@ -88,6 +88,7 @@ static int ladder_select_state(struct cp
/* consider promotion */
if (last_idx < drv->state_count - 1 &&
+ !dev->states_usage[last_idx + 1].disable &&
last_residency > last_state->threshold.promotion_time &&
drv->states[last_idx + 1].exit_latency <= latency_req) {
last_state->stats.promotion_count++;
@@ -100,7 +101,8 @@ static int ladder_select_state(struct cp
/* consider demotion */
if (last_idx > CPUIDLE_DRIVER_STATE_START &&
- drv->states[last_idx].exit_latency > latency_req) {
+ (dev->states_usage[last_idx].disable ||
+ drv->states[last_idx].exit_latency > latency_req)) {
int i;
for (i = last_idx - 1; i > CPUIDLE_DRIVER_STATE_START; i--) {

Carsten Emde

2012-07-19 22:22:30 UTC

Permalink

Post by Rafael J. Wysocki

This looks fine to me, but it's too late for v3.6. I can queue it up
for v3.7, though.

Yes, please.

Thanks,
-Carsten.

Carsten Emde

2012-07-19 20:34:09 UTC

Permalink

Rafael,

Post by Rafael J. Wysocki
http://git.kernel.org/?p=linux/kernel/git/rafael/linux-pm.git;a=commit;h=dc7fd275ae60ef8edf952aff2a62462f5d892fd4
into account, does it?

Hmm, oops, you're right. This one came in after I checked it last time.

Changes in v2: A note in the documentation explains why the sysfs
variable "disable" may not always reflect the current situation
and why modifying it may not always work as expected.

Changes in v3: The patch now applies to the current linux-next and
uses the per-cpu "disable" field.

-Carsten.