cpuidle: improve governor Kconfig options

Message ID 1369688458-9114-1-git-send-email-daniel.lezcano@linaro.org
State New
Headers show

Commit Message

Daniel Lezcano May 27, 2013, 9 p.m.
Each governor is suitable for different kernel configurations: the menu
governor suits better for a tickless system, while the ladder governor fits
better for a periodic timer tick system.

The Kconfig does not allow to [un]select a governor, thus both are compiled in
the kernel but the init order makes the menu governor to be the last one to be
registered, so becoming the default. The only way to switch back to the ladder
governor is to enable the sysfs governor switch in the kernel command line.

Because it seems nobody complained about this, the menu governor is used by
default most of the time on the system, having both governors is not really
necessary on a tickless system but there isn't a config option to disable one
or another governor.

Create a submenu for cpuidle and add a label for each governor, so we can see
the option in the menu config and enable/disable it.

The governors will be enabled depending on the CONFIG_NO_HZ option:
 - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
   governor is optional, defaulting to 'no'
 - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
   menu governor is optional, defaulting to 'no'

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/Kconfig |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Rafael J. Wysocki May 28, 2013, 11:46 a.m. | #1
On Monday, May 27, 2013 11:00:58 PM Daniel Lezcano wrote:
> Each governor is suitable for different kernel configurations: the menu
> governor suits better for a tickless system, while the ladder governor fits
> better for a periodic timer tick system.
> 
> The Kconfig does not allow to [un]select a governor, thus both are compiled in
> the kernel but the init order makes the menu governor to be the last one to be
> registered, so becoming the default. The only way to switch back to the ladder
> governor is to enable the sysfs governor switch in the kernel command line.
> 
> Because it seems nobody complained about this, the menu governor is used by
> default most of the time on the system, having both governors is not really
> necessary on a tickless system but there isn't a config option to disable one
> or another governor.
> 
> Create a submenu for cpuidle and add a label for each governor, so we can see
> the option in the menu config and enable/disable it.
> 
> The governors will be enabled depending on the CONFIG_NO_HZ option:
>  - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
>    governor is optional, defaulting to 'no'

Why not defaulting to 'yes'?  That'd be backwards compatible, wouldn't it?

>  - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
>    menu governor is optional, defaulting to 'no'

Are you sure this is not going to introduce regressions with CONFIG_NO_HZ
unset?

Rafael


> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/Kconfig |   20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index c4cc27e..90c2f39 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -1,7 +1,9 @@
>  
> -config CPU_IDLE
> +menuconfig CPU_IDLE
>  	bool "CPU idle PM support"
>  	default y if ACPI || PPC_PSERIES
> +	select CPU_IDLE_GOV_LADDER if (!NO_HZ && !NO_HZ_IDLE)
> +	select CPU_IDLE_GOV_MENU if (NO_HZ || NO_HZ_IDLE)
>  	help
>  	  CPU idle is a generic framework for supporting software-controlled
>  	  idle processor power management.  It includes modular cross-platform
> @@ -9,9 +11,10 @@ config CPU_IDLE
>  
>  	  If you're using an ACPI-enabled platform, you should say Y here.
>  
> +if CPU_IDLE
> +
>  config CPU_IDLE_MULTIPLE_DRIVERS
>          bool "Support multiple cpuidle drivers"
> -        depends on CPU_IDLE
>          default n
>          help
>           Allows the cpuidle framework to use different drivers for each CPU.
> @@ -19,24 +22,19 @@ config CPU_IDLE_MULTIPLE_DRIVERS
>           states. If unsure say N.
>  
>  config CPU_IDLE_GOV_LADDER
> -	bool
> -	depends on CPU_IDLE
> -	default y
> +	bool "Ladder governor (for periodic timer tick)"
> +	default n if (NO_HZ || NO_HZ_IDLE)
>  
>  config CPU_IDLE_GOV_MENU
> -	bool
> -	depends on CPU_IDLE && NO_HZ
> -	default y
> +	bool "Menu governor (for tickless system)"
> +	default n if (!NO_HZ && !NO_HZ_IDLE)
>  
>  config ARCH_NEEDS_CPU_IDLE_COUPLED
>  	def_bool n
>  
> -if CPU_IDLE
> -
>  config CPU_IDLE_CALXEDA
>  	bool "CPU Idle Driver for Calxeda processors"
>  	depends on ARCH_HIGHBANK
>  	help
>  	  Select this to enable cpuidle on Calxeda processors.
> -
>  endif
>
Daniel Lezcano May 28, 2013, 12:23 p.m. | #2
On 05/28/2013 01:46 PM, Rafael J. Wysocki wrote:
> On Monday, May 27, 2013 11:00:58 PM Daniel Lezcano wrote:
>> Each governor is suitable for different kernel configurations: the menu
>> governor suits better for a tickless system, while the ladder governor fits
>> better for a periodic timer tick system.
>>
>> The Kconfig does not allow to [un]select a governor, thus both are compiled in
>> the kernel but the init order makes the menu governor to be the last one to be
>> registered, so becoming the default. The only way to switch back to the ladder
>> governor is to enable the sysfs governor switch in the kernel command line.
>>
>> Because it seems nobody complained about this, the menu governor is used by
>> default most of the time on the system, having both governors is not really
>> necessary on a tickless system but there isn't a config option to disable one
>> or another governor.
>>
>> Create a submenu for cpuidle and add a label for each governor, so we can see
>> the option in the menu config and enable/disable it.
>>
>> The governors will be enabled depending on the CONFIG_NO_HZ option:
>>  - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
>>    governor is optional, defaulting to 'no'
> 
> Why not defaulting to 'yes'?  That'd be backwards compatible, wouldn't it?

Yes, that wouldn't be backward compatible but who wants the ladder
governor which is less efficient with a tickless system ?

>>  - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
>>    menu governor is optional, defaulting to 'no'
> 
> Are you sure this is not going to introduce regressions with CONFIG_NO_HZ
> unset?

Yes, a system configured with a periodic tick will use the ladder
instead of the menu governor (which is less efficient on this
configuration).

Currently the default is the menu governor with a periodic tick system
because both governors are compiled in and the init order makes the menu
governor to be enabled instead of the 'ladder'.

We can set both to 'yes' by default but in this case, I suggest to add
in the code a 'preferred' governor depending on tickless or not, so the
right governor is chosen at boot time depending on CONFIG_NO_HZ.

Otherwise, I am not sure people running servers (with a periodic tick)
will change their kernel command line option to enable the sysfs
governor switch to change the governor to ladder, neither they are aware
ladder is better for their system.

I am open to any suggestion.

Thanks
  -- Daniel
Rafael J. Wysocki May 28, 2013, 12:42 p.m. | #3
On Tuesday, May 28, 2013 02:23:00 PM Daniel Lezcano wrote:
> On 05/28/2013 01:46 PM, Rafael J. Wysocki wrote:
> > On Monday, May 27, 2013 11:00:58 PM Daniel Lezcano wrote:
> >> Each governor is suitable for different kernel configurations: the menu
> >> governor suits better for a tickless system, while the ladder governor fits
> >> better for a periodic timer tick system.
> >>
> >> The Kconfig does not allow to [un]select a governor, thus both are compiled in
> >> the kernel but the init order makes the menu governor to be the last one to be
> >> registered, so becoming the default. The only way to switch back to the ladder
> >> governor is to enable the sysfs governor switch in the kernel command line.
> >>
> >> Because it seems nobody complained about this, the menu governor is used by
> >> default most of the time on the system, having both governors is not really
> >> necessary on a tickless system but there isn't a config option to disable one
> >> or another governor.
> >>
> >> Create a submenu for cpuidle and add a label for each governor, so we can see
> >> the option in the menu config and enable/disable it.
> >>
> >> The governors will be enabled depending on the CONFIG_NO_HZ option:
> >>  - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
> >>    governor is optional, defaulting to 'no'
> > 
> > Why not defaulting to 'yes'?  That'd be backwards compatible, wouldn't it?
> 
> Yes, that wouldn't be backward compatible but who wants the ladder
> governor which is less efficient with a tickless system ?

I don't know and this isn't the right question to ask I think.

You need to ask who uses the ladder governor with CONFIG_NO_HZ and you need to
avoid making changes that aren't backwards compatible if you don't know the
answer (which I'm pretty sure is the case).

I'd prefer to default to 'yes' to start with and then to change the
default to 'no' after a couple of cycles, possibly.

> >>  - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
> >>    menu governor is optional, defaulting to 'no'
> > 
> > Are you sure this is not going to introduce regressions with CONFIG_NO_HZ
> > unset?
> 
> Yes, a system configured with a periodic tick will use the ladder
> instead of the menu governor (which is less efficient on this
> configuration).

Well, "less efficient" meaning what exactly?  Less power-efficient or less
efficient performance-wise?

> Currently the default is the menu governor with a periodic tick system
> because both governors are compiled in and the init order makes the menu
> governor to be enabled instead of the 'ladder'.

I understand that.

> We can set both to 'yes' by default but in this case, I suggest to add
> in the code a 'preferred' governor depending on tickless or not, so the
> right governor is chosen at boot time depending on CONFIG_NO_HZ.
> 
> Otherwise, I am not sure people running servers (with a periodic tick)
> will change their kernel command line option to enable the sysfs
> governor switch to change the governor to ladder, neither they are aware
> ladder is better for their system.
> 
> I am open to any suggestion.

I think the change to make the governor preference depend on CONFIG_NO_HZ
kind of makes sense.  However, I'd default to 'yes' for both governors in both
configurations to start with (which basically means that the ordering will now
depend on CONFIG_NO_HZ).

Thanks,
Rafael
Daniel Lezcano May 28, 2013, 1:23 p.m. | #4
On 05/28/2013 02:42 PM, Rafael J. Wysocki wrote:
> On Tuesday, May 28, 2013 02:23:00 PM Daniel Lezcano wrote:
>> On 05/28/2013 01:46 PM, Rafael J. Wysocki wrote:
>>> On Monday, May 27, 2013 11:00:58 PM Daniel Lezcano wrote:
>>>> Each governor is suitable for different kernel configurations: the menu
>>>> governor suits better for a tickless system, while the ladder governor fits
>>>> better for a periodic timer tick system.
>>>>
>>>> The Kconfig does not allow to [un]select a governor, thus both are compiled in
>>>> the kernel but the init order makes the menu governor to be the last one to be
>>>> registered, so becoming the default. The only way to switch back to the ladder
>>>> governor is to enable the sysfs governor switch in the kernel command line.
>>>>
>>>> Because it seems nobody complained about this, the menu governor is used by
>>>> default most of the time on the system, having both governors is not really
>>>> necessary on a tickless system but there isn't a config option to disable one
>>>> or another governor.
>>>>
>>>> Create a submenu for cpuidle and add a label for each governor, so we can see
>>>> the option in the menu config and enable/disable it.
>>>>
>>>> The governors will be enabled depending on the CONFIG_NO_HZ option:
>>>>  - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
>>>>    governor is optional, defaulting to 'no'
>>>
>>> Why not defaulting to 'yes'?  That'd be backwards compatible, wouldn't it?
>>
>> Yes, that wouldn't be backward compatible but who wants the ladder
>> governor which is less efficient with a tickless system ?
> 
> I don't know and this isn't the right question to ask I think.
> 
> You need to ask who uses the ladder governor with CONFIG_NO_HZ and you need to
> avoid making changes that aren't backwards compatible if you don't know the
> answer (which I'm pretty sure is the case).
> 
> I'd prefer to default to 'yes' to start with and then to change the
> default to 'no' after a couple of cycles, possibly.
> 
>>>>  - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
>>>>    menu governor is optional, defaulting to 'no'
>>>
>>> Are you sure this is not going to introduce regressions with CONFIG_NO_HZ
>>> unset?
>>
>> Yes, a system configured with a periodic tick will use the ladder
>> instead of the menu governor (which is less efficient on this
>> configuration).
> 
> Well, "less efficient" meaning what exactly?  Less power-efficient or less
> efficient performance-wise?

less power efficient.

>> Currently the default is the menu governor with a periodic tick system
>> because both governors are compiled in and the init order makes the menu
>> governor to be enabled instead of the 'ladder'.
> 
> I understand that.
> 
>> We can set both to 'yes' by default but in this case, I suggest to add
>> in the code a 'preferred' governor depending on tickless or not, so the
>> right governor is chosen at boot time depending on CONFIG_NO_HZ.
>>
>> Otherwise, I am not sure people running servers (with a periodic tick)
>> will change their kernel command line option to enable the sysfs
>> governor switch to change the governor to ladder, neither they are aware
>> ladder is better for their system.
>>
>> I am open to any suggestion.
> 
> I think the change to make the governor preference depend on CONFIG_NO_HZ
> kind of makes sense.  However, I'd default to 'yes' for both governors in both
> configurations to start with (which basically means that the ordering will now
> depend on CONFIG_NO_HZ).

Ok, no problem. I will respin the patch.

Thanks for the review

  -- Daniel
Rafael J. Wysocki May 28, 2013, 9:26 p.m. | #5
On Tuesday, May 28, 2013 03:23:37 PM Daniel Lezcano wrote:
> On 05/28/2013 02:42 PM, Rafael J. Wysocki wrote:
> > On Tuesday, May 28, 2013 02:23:00 PM Daniel Lezcano wrote:
> >> On 05/28/2013 01:46 PM, Rafael J. Wysocki wrote:
> >>> On Monday, May 27, 2013 11:00:58 PM Daniel Lezcano wrote:
> >>>> Each governor is suitable for different kernel configurations: the menu
> >>>> governor suits better for a tickless system, while the ladder governor fits
> >>>> better for a periodic timer tick system.
> >>>>
> >>>> The Kconfig does not allow to [un]select a governor, thus both are compiled in
> >>>> the kernel but the init order makes the menu governor to be the last one to be
> >>>> registered, so becoming the default. The only way to switch back to the ladder
> >>>> governor is to enable the sysfs governor switch in the kernel command line.
> >>>>
> >>>> Because it seems nobody complained about this, the menu governor is used by
> >>>> default most of the time on the system, having both governors is not really
> >>>> necessary on a tickless system but there isn't a config option to disable one
> >>>> or another governor.
> >>>>
> >>>> Create a submenu for cpuidle and add a label for each governor, so we can see
> >>>> the option in the menu config and enable/disable it.
> >>>>
> >>>> The governors will be enabled depending on the CONFIG_NO_HZ option:
> >>>>  - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
> >>>>    governor is optional, defaulting to 'no'
> >>>
> >>> Why not defaulting to 'yes'?  That'd be backwards compatible, wouldn't it?
> >>
> >> Yes, that wouldn't be backward compatible but who wants the ladder
> >> governor which is less efficient with a tickless system ?
> > 
> > I don't know and this isn't the right question to ask I think.
> > 
> > You need to ask who uses the ladder governor with CONFIG_NO_HZ and you need to
> > avoid making changes that aren't backwards compatible if you don't know the
> > answer (which I'm pretty sure is the case).
> > 
> > I'd prefer to default to 'yes' to start with and then to change the
> > default to 'no' after a couple of cycles, possibly.
> > 
> >>>>  - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
> >>>>    menu governor is optional, defaulting to 'no'
> >>>
> >>> Are you sure this is not going to introduce regressions with CONFIG_NO_HZ
> >>> unset?
> >>
> >> Yes, a system configured with a periodic tick will use the ladder
> >> instead of the menu governor (which is less efficient on this
> >> configuration).
> > 
> > Well, "less efficient" meaning what exactly?  Less power-efficient or less
> > efficient performance-wise?
> 
> less power efficient.

I thought so, but that's really important, because some people *do* care about
performance.  In the context of changes like this one we always need to talk
about both power and performance.  For example, when you're saying that
changing something may lead to better energy saving behavior, you should also
say what the impact of that change on performance is going to be.

Thanks,
Rafael
Daniel Lezcano May 28, 2013, 9:39 p.m. | #6
On 05/28/2013 11:26 PM, Rafael J. Wysocki wrote:
> On Tuesday, May 28, 2013 03:23:37 PM Daniel Lezcano wrote:
>> On 05/28/2013 02:42 PM, Rafael J. Wysocki wrote:
>>> On Tuesday, May 28, 2013 02:23:00 PM Daniel Lezcano wrote:
>>>> On 05/28/2013 01:46 PM, Rafael J. Wysocki wrote:
>>>>> On Monday, May 27, 2013 11:00:58 PM Daniel Lezcano wrote:
>>>>>> Each governor is suitable for different kernel configurations: the menu
>>>>>> governor suits better for a tickless system, while the ladder governor fits
>>>>>> better for a periodic timer tick system.
>>>>>>
>>>>>> The Kconfig does not allow to [un]select a governor, thus both are compiled in
>>>>>> the kernel but the init order makes the menu governor to be the last one to be
>>>>>> registered, so becoming the default. The only way to switch back to the ladder
>>>>>> governor is to enable the sysfs governor switch in the kernel command line.
>>>>>>
>>>>>> Because it seems nobody complained about this, the menu governor is used by
>>>>>> default most of the time on the system, having both governors is not really
>>>>>> necessary on a tickless system but there isn't a config option to disable one
>>>>>> or another governor.
>>>>>>
>>>>>> Create a submenu for cpuidle and add a label for each governor, so we can see
>>>>>> the option in the menu config and enable/disable it.
>>>>>>
>>>>>> The governors will be enabled depending on the CONFIG_NO_HZ option:
>>>>>>  - If CONFIG_NO_HZ is set, then the menu governor is selected and the ladder
>>>>>>    governor is optional, defaulting to 'no'
>>>>>
>>>>> Why not defaulting to 'yes'?  That'd be backwards compatible, wouldn't it?
>>>>
>>>> Yes, that wouldn't be backward compatible but who wants the ladder
>>>> governor which is less efficient with a tickless system ?
>>>
>>> I don't know and this isn't the right question to ask I think.
>>>
>>> You need to ask who uses the ladder governor with CONFIG_NO_HZ and you need to
>>> avoid making changes that aren't backwards compatible if you don't know the
>>> answer (which I'm pretty sure is the case).
>>>
>>> I'd prefer to default to 'yes' to start with and then to change the
>>> default to 'no' after a couple of cycles, possibly.
>>>
>>>>>>  - If CONFIG_NO_HZ is not set, then the ladder governor is selected and the
>>>>>>    menu governor is optional, defaulting to 'no'
>>>>>
>>>>> Are you sure this is not going to introduce regressions with CONFIG_NO_HZ
>>>>> unset?
>>>>
>>>> Yes, a system configured with a periodic tick will use the ladder
>>>> instead of the menu governor (which is less efficient on this
>>>> configuration).
>>>
>>> Well, "less efficient" meaning what exactly?  Less power-efficient or less
>>> efficient performance-wise?
>>
>> less power efficient.
> 
> I thought so, but that's really important, because some people *do* care about
> performance.  In the context of changes like this one we always need to talk
> about both power and performance.  For example, when you're saying that
> changing something may lead to better energy saving behavior, you should also
> say what the impact of that change on performance is going to be

I fully agree with you but with this very specific case, when the
cpuidle is enabled in the system, it is to have the best trade-off
between performance and power saving. Using the ladder governor on a
tickless system is not power efficient and the performance is not
significantly improved. If the governor is too agressive in power saving
and impacts the performance the pm_qos will guarantee the needed
performance.

Desktop will use tickless system because they want power saving, thus
cpuidle and the menu governor is the best trade-off.

Server will want more responsiveness and will use a periodic timer tick
system, the cpuidle and the ladder suits better.

The pm_qos will ensure a latency for both governors.

But again I agree 100% with you and keeping the old behavior makes sense
until we sort this out clearly in the next cycles.

Thanks
  -- Daniel

Patch

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c4cc27e..90c2f39 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -1,7 +1,9 @@ 
 
-config CPU_IDLE
+menuconfig CPU_IDLE
 	bool "CPU idle PM support"
 	default y if ACPI || PPC_PSERIES
+	select CPU_IDLE_GOV_LADDER if (!NO_HZ && !NO_HZ_IDLE)
+	select CPU_IDLE_GOV_MENU if (NO_HZ || NO_HZ_IDLE)
 	help
 	  CPU idle is a generic framework for supporting software-controlled
 	  idle processor power management.  It includes modular cross-platform
@@ -9,9 +11,10 @@  config CPU_IDLE
 
 	  If you're using an ACPI-enabled platform, you should say Y here.
 
+if CPU_IDLE
+
 config CPU_IDLE_MULTIPLE_DRIVERS
         bool "Support multiple cpuidle drivers"
-        depends on CPU_IDLE
         default n
         help
          Allows the cpuidle framework to use different drivers for each CPU.
@@ -19,24 +22,19 @@  config CPU_IDLE_MULTIPLE_DRIVERS
          states. If unsure say N.
 
 config CPU_IDLE_GOV_LADDER
-	bool
-	depends on CPU_IDLE
-	default y
+	bool "Ladder governor (for periodic timer tick)"
+	default n if (NO_HZ || NO_HZ_IDLE)
 
 config CPU_IDLE_GOV_MENU
-	bool
-	depends on CPU_IDLE && NO_HZ
-	default y
+	bool "Menu governor (for tickless system)"
+	default n if (!NO_HZ && !NO_HZ_IDLE)
 
 config ARCH_NEEDS_CPU_IDLE_COUPLED
 	def_bool n
 
-if CPU_IDLE
-
 config CPU_IDLE_CALXEDA
 	bool "CPU Idle Driver for Calxeda processors"
 	depends on ARCH_HIGHBANK
 	help
 	  Select this to enable cpuidle on Calxeda processors.
-
 endif