* [PATCH 0/1] cpuidle: allow to disable C states of the ladder governor @ 2012-07-17 18:59 Carsten Emde 2012-07-17 18:59 ` [PATCH 1/1] Honor state disabling in the cpuidle " Carsten Emde 0 siblings, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-17 18:59 UTC (permalink / raw) To: Len Brown Cc: Kevin Hilman, Deepthi Dharwar, Thomas Gleixner, LKML, Carsten Emde Hi, when trying to disable a specific C state in order to improve the idle responsiveness of a system but still enjoy some power saving, we noted that writing to the related sysfs variable had no effect. This patch allows to disable C states of the ladder governor. -Carsten. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/1] Honor state disabling in the cpuidle ladder governor 2012-07-17 18:59 [PATCH 0/1] cpuidle: allow to disable C states of the ladder governor Carsten Emde @ 2012-07-17 18:59 ` Carsten Emde 2012-07-18 6:36 ` Deepthi Dharwar 0 siblings, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-17 18:59 UTC (permalink / raw) To: Len Brown Cc: Kevin Hilman, Deepthi Dharwar, Thomas Gleixner, LKML, Carsten Emde [-- Attachment #1: drivers-cpuidle-ladder-honor-disabling.patch --] [-- Type: text/plain, Size: 2097 bytes --] 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. Signed-off-by: Carsten Emde <C.Emde@osadl.org> --- 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--) { ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Honor state disabling in the cpuidle ladder governor 2012-07-17 18:59 ` [PATCH 1/1] Honor state disabling in the cpuidle " Carsten Emde @ 2012-07-18 6:36 ` Deepthi Dharwar 2012-07-18 11:02 ` Carsten Emde 0 siblings, 1 reply; 17+ messages in thread From: Deepthi Dharwar @ 2012-07-18 6:36 UTC (permalink / raw) To: Carsten Emde; +Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML On 07/18/2012 12:29 AM, Carsten Emde wrote: > 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 > > Signed-off-by: Carsten Emde <C.Emde@osadl.org> > > --- > 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--) { > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Honor state disabling in the cpuidle ladder governor 2012-07-18 6:36 ` Deepthi Dharwar @ 2012-07-18 11:02 ` Carsten Emde 2012-07-18 11:48 ` Deepthi Dharwar 0 siblings, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-18 11:02 UTC (permalink / raw) To: Deepthi Dharwar; +Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML On 07/18/2012 08:36 AM, Deepthi Dharwar wrote: > On 07/18/2012 12:29 AM, Carsten Emde wrote: > >> 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. 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. 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? -Carsten. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1] Honor state disabling in the cpuidle ladder governor 2012-07-18 11:02 ` Carsten Emde @ 2012-07-18 11:48 ` Deepthi Dharwar 2012-07-18 14:09 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle ladder governor - documented Carsten Emde 2012-07-18 14:38 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer Carsten Emde 0 siblings, 2 replies; 17+ messages in thread From: Deepthi Dharwar @ 2012-07-18 11:48 UTC (permalink / raw) To: Carsten Emde Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list On 07/18/2012 04:32 PM, Carsten Emde wrote: > On 07/18/2012 08:36 AM, Deepthi Dharwar wrote: >> On 07/18/2012 12:29 AM, Carsten Emde wrote: >> >>> 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. > 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. Cheers Deepthi > -Carsten. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/1 v2] Honor state disabling in the cpuidle ladder governor - documented 2012-07-18 11:48 ` Deepthi Dharwar @ 2012-07-18 14:09 ` Carsten Emde 2012-07-18 14:38 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer Carsten Emde 1 sibling, 0 replies; 17+ messages in thread From: Carsten Emde @ 2012-07-18 14:09 UTC (permalink / raw) To: Deepthi Dharwar Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list [-- Attachment #1: Type: text/plain, Size: 2823 bytes --] On 07/18/2012 01:48 PM, Deepthi Dharwar wrote: > On 07/18/2012 04:32 PM, Carsten Emde wrote: >> On 07/18/2012 08:36 AM, Deepthi Dharwar wrote: >>> On 07/18/2012 12:29 AM, Carsten Emde wrote: >>>> 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. >> 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. Here comes v2 with a related section added to the documentation. -Carsten. [-- Attachment #2: drivers-cpuidle-ladder-honor-disabling-with-doc.patch --] [-- Type: text/x-patch, Size: 3857 bytes --] Subject: Honor state disabling in the cpuidle ladder governor From: Carsten Emde <C.Emde@osadl.org> Date: Tue, 17 Jul 2012 19:50:13 +0100 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 addded to the documentation. Signed-off-by: Carsten Emde <C.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--) { ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer 2012-07-18 11:48 ` Deepthi Dharwar 2012-07-18 14:09 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle ladder governor - documented Carsten Emde @ 2012-07-18 14:38 ` Carsten Emde 2012-07-19 11:14 ` Deepthi Dharwar 1 sibling, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-18 14:38 UTC (permalink / raw) To: Deepthi Dharwar Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list [-- Attachment #1: Type: text/plain, Size: 2848 bytes --] On 07/18/2012 01:48 PM, Deepthi Dharwar wrote: > On 07/18/2012 04:32 PM, Carsten Emde wrote: >> On 07/18/2012 08:36 AM, Deepthi Dharwar wrote: >>> On 07/18/2012 12:29 AM, Carsten Emde wrote: >>>> 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. >> 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? -Carsten. [-- Attachment #2: drivers-cpuidle-ladder-honor-disabling-with-sanitizer.patch --] [-- Type: text/x-patch, Size: 4928 bytes --] From: Carsten Emde <C.Emde@osadl.org> Subject: Honor state disabling in the cpuidle ladder governor 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. A sanitize mechanism 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. Signed-off-by: Carsten Emde <C.Emde@osadl.org> --- drivers/cpuidle/governors/ladder.c | 25 ++++++++++++++++++++++++- drivers/cpuidle/sysfs.c | 6 ++++++ include/linux/cpuidle.h | 4 ++++ 3 files changed, 34 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 @@ -58,6 +58,25 @@ static inline void ladder_do_selection(s ldev->last_state_idx = new_idx; } +void notify_state_modified(struct cpuidle_state *state) +{ + if (state->disable) { + /* synchronize all deeper states, if any */ + int states = state->state_count - 1 - state->state_no; + struct cpuidle_state *last = state + states; + + while (++state <= last) + state->disable = 1; + } else { + /* synchronize all lighter states, if any */ + int states = state->state_no; + struct cpuidle_state *first = state - states; + + while (--state >= first) + state->disable = 0; + } +} + /** * ladder_select_state - selects the next state to enter * @drv: cpuidle driver @@ -88,6 +107,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 +120,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--) { @@ -154,6 +175,8 @@ static int ladder_enable_device(struct c lstate->threshold.promotion_time = state->exit_latency; if (i > 0) lstate->threshold.demotion_time = state->exit_latency; + + state->notify = notify_state_modified; } return 0; Index: linux-3.4.4-rt14-rc2-64/drivers/cpuidle/sysfs.c =================================================================== --- linux-3.4.4-rt14-rc2-64.orig/drivers/cpuidle/sysfs.c +++ linux-3.4.4-rt14-rc2-64/drivers/cpuidle/sysfs.c @@ -239,15 +239,19 @@ static ssize_t store_state_##_name(struc { \ long value; \ int err; \ + unsigned int old; \ if (!capable(CAP_SYS_ADMIN)) \ return -EPERM; \ err = kstrtol(buf, 0, &value); \ if (err) \ return err; \ + old = state->disable; \ if (value) \ state->disable = 1; \ else \ state->disable = 0; \ + if (state->notify && state->disable != old) \ + state->notify(state); \ return size; \ } @@ -377,6 +381,8 @@ int cpuidle_add_state_sysfs(struct cpuid kfree(kobj); goto error_state; } + drv->states[i].state_no = i; + drv->states[i].state_count = device->state_count; kobject_uevent(&kobj->kobj, KOBJ_ADD); device->kobjs[i] = kobj; } Index: linux-3.4.4-rt14-rc2-64/include/linux/cpuidle.h =================================================================== --- linux-3.4.4-rt14-rc2-64.orig/include/linux/cpuidle.h +++ linux-3.4.4-rt14-rc2-64/include/linux/cpuidle.h @@ -47,6 +47,10 @@ struct cpuidle_state { int power_usage; /* in mW */ unsigned int target_residency; /* in US */ unsigned int disable; + unsigned int state_no; + unsigned int state_count; + + void (*notify) (struct cpuidle_state *state); int (*enter) (struct cpuidle_device *dev, struct cpuidle_driver *drv, ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer 2012-07-18 14:38 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer Carsten Emde @ 2012-07-19 11:14 ` Deepthi Dharwar 2012-07-19 11:39 ` Carsten Emde 0 siblings, 1 reply; 17+ messages in thread From: Deepthi Dharwar @ 2012-07-19 11:14 UTC (permalink / raw) To: Carsten Emde Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list On 07/18/2012 08:08 PM, Carsten Emde wrote: > On 07/18/2012 01:48 PM, Deepthi Dharwar wrote: >> On 07/18/2012 04:32 PM, Carsten Emde wrote: >>> On 07/18/2012 08:36 AM, Deepthi Dharwar wrote: >>>> On 07/18/2012 12:29 AM, Carsten Emde wrote: >>>>> 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. >>> 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. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer 2012-07-19 11:14 ` Deepthi Dharwar @ 2012-07-19 11:39 ` Carsten Emde 2012-07-19 18:42 ` Rafael J. Wysocki 2012-07-19 18:52 ` [PATCH 0/1 v2] cpuidle: allow to disable C states of the ladder governor Carsten Emde 0 siblings, 2 replies; 17+ messages in thread From: Carsten Emde @ 2012-07-19 11:39 UTC (permalink / raw) To: Deepthi Dharwar Cc: Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list Deepthi, >>> [..] >>>> 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. ok, let's take v2. > Also, any reason why the patch is being sent as an attachment ? > Sending patches as an attachment is not a recommended practice. Sorry. -Carsten. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer 2012-07-19 11:39 ` Carsten Emde @ 2012-07-19 18:42 ` Rafael J. Wysocki 2012-07-19 18:52 ` [PATCH 0/1 v2] cpuidle: allow to disable C states of the ladder governor Carsten Emde 1 sibling, 0 replies; 17+ messages in thread From: Rafael J. Wysocki @ 2012-07-19 18:42 UTC (permalink / raw) To: Carsten Emde Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list Hi, On Thursday, July 19, 2012, Carsten Emde wrote: > Deepthi, > > >>> [..] > >>>> 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. > ok, let's take v2. > > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/1 v2] cpuidle: allow to disable C states of the ladder governor 2012-07-19 11:39 ` Carsten Emde 2012-07-19 18:42 ` Rafael J. Wysocki @ 2012-07-19 18:52 ` Carsten Emde 2012-07-19 18:52 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle " Carsten Emde 1 sibling, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-19 18:52 UTC (permalink / raw) To: Rafael Wysocki Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list, Carsten Emde Rafael, >>>>>> [..] >>>>>> 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. >> 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/1 v2] Honor state disabling in the cpuidle ladder governor 2012-07-19 18:52 ` [PATCH 0/1 v2] cpuidle: allow to disable C states of the ladder governor Carsten Emde @ 2012-07-19 18:52 ` Carsten Emde 2012-07-19 19:30 ` Rafael J. Wysocki 2012-07-19 20:34 ` [PATCH 0/1 v3] cpuidle: allow to disable C states of the " Carsten Emde 0 siblings, 2 replies; 17+ messages in thread From: Carsten Emde @ 2012-07-19 18:52 UTC (permalink / raw) To: Rafael Wysocki Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list, Carsten Emde [-- Attachment #1: drivers-cpuidle-ladder-honor-disabling-with-doc.patch --] [-- Type: text/plain, Size: 3718 bytes --] 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 <C.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--) { ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1 v2] Honor state disabling in the cpuidle ladder governor 2012-07-19 18:52 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle " Carsten Emde @ 2012-07-19 19:30 ` Rafael J. Wysocki 2012-07-19 20:34 ` [PATCH 0/1 v3] cpuidle: allow to disable C states of the " Carsten Emde 1 sibling, 0 replies; 17+ messages in thread From: Rafael J. Wysocki @ 2012-07-19 19:30 UTC (permalink / raw) To: Carsten Emde Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list On Thursday, July 19, 2012, Carsten Emde wrote: > 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 <C.Emde@osadl.org> 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 > --- > 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--) { > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 0/1 v3] cpuidle: allow to disable C states of the ladder governor 2012-07-19 18:52 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle " Carsten Emde 2012-07-19 19:30 ` Rafael J. Wysocki @ 2012-07-19 20:34 ` Carsten Emde 2012-07-19 20:34 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle " Carsten Emde 1 sibling, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-19 20:34 UTC (permalink / raw) To: Rafael Wysocki Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list, Carsten Emde Rafael, > 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? 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor 2012-07-19 20:34 ` [PATCH 0/1 v3] cpuidle: allow to disable C states of the " Carsten Emde @ 2012-07-19 20:34 ` Carsten Emde 2012-07-19 21:48 ` Rafael J. Wysocki 0 siblings, 1 reply; 17+ messages in thread From: Carsten Emde @ 2012-07-19 20:34 UTC (permalink / raw) To: Rafael Wysocki Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list, Carsten Emde [-- Attachment #1: drivers-cpuidle-ladder-honor-disabling-with-doc.patch --] [-- Type: text/plain, Size: 3730 bytes --] 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 <C.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--) { ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor 2012-07-19 20:34 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle " Carsten Emde @ 2012-07-19 21:48 ` Rafael J. Wysocki 2012-07-19 22:22 ` Carsten Emde 0 siblings, 1 reply; 17+ messages in thread From: Rafael J. Wysocki @ 2012-07-19 21:48 UTC (permalink / raw) To: Carsten Emde Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list On Thursday, July 19, 2012, Carsten Emde wrote: > 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 <C.Emde@osadl.org> This looks fine to me, but it's too late for v3.6. I can queue it up for v3.7, though. Thanks, Rafael > --- > 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--) { > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor 2012-07-19 21:48 ` Rafael J. Wysocki @ 2012-07-19 22:22 ` Carsten Emde 0 siblings, 0 replies; 17+ messages in thread From: Carsten Emde @ 2012-07-19 22:22 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Deepthi Dharwar, Len Brown, Kevin Hilman, Thomas Gleixner, LKML, Linux PM mailing list On 07/19/2012 11:48 PM, Rafael J. Wysocki wrote: > On Thursday, July 19, 2012, Carsten Emde wrote: >> 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<C.Emde@osadl.org> > > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2012-07-19 22:36 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-07-17 18:59 [PATCH 0/1] cpuidle: allow to disable C states of the ladder governor Carsten Emde 2012-07-17 18:59 ` [PATCH 1/1] Honor state disabling in the cpuidle " Carsten Emde 2012-07-18 6:36 ` Deepthi Dharwar 2012-07-18 11:02 ` Carsten Emde 2012-07-18 11:48 ` Deepthi Dharwar 2012-07-18 14:09 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle ladder governor - documented Carsten Emde 2012-07-18 14:38 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle ladder governor - with sanitizer Carsten Emde 2012-07-19 11:14 ` Deepthi Dharwar 2012-07-19 11:39 ` Carsten Emde 2012-07-19 18:42 ` Rafael J. Wysocki 2012-07-19 18:52 ` [PATCH 0/1 v2] cpuidle: allow to disable C states of the ladder governor Carsten Emde 2012-07-19 18:52 ` [PATCH 1/1 v2] Honor state disabling in the cpuidle " Carsten Emde 2012-07-19 19:30 ` Rafael J. Wysocki 2012-07-19 20:34 ` [PATCH 0/1 v3] cpuidle: allow to disable C states of the " Carsten Emde 2012-07-19 20:34 ` [PATCH 1/1 v3] Honor state disabling in the cpuidle " Carsten Emde 2012-07-19 21:48 ` Rafael J. Wysocki 2012-07-19 22:22 ` Carsten Emde
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.