[RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Hanjun Guo]

For now cpuidle governor can be switched via sysfs only when the
boot option "cpuidle_sysfs_switch" is passed, but it's important
to switch the governor to adapt to different workloads, especially
after TEO and haltpoll governor were introduced.

Introduce a CONFIG option to make cpuidle governor switchable to be
the default behaviour, which will not break the boot option behaviour.

Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
---
 drivers/cpuidle/Kconfig | 9 +++++++++
 drivers/cpuidle/sysfs.c | 2 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd..c40cb40 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
 config DT_IDLE_STATES
 	bool
 
+config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
+	bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
+	help
+	  Make the CPU idle governor switchable at runtime, and make it as the
+	  default behaviour even the boot option "cpuidle_sysfs_switch" is not
+	  passed in cmdline.
+
+	  Say N if you unsure about this.
+
 menu "ARM CPU Idle Drivers"
 depends on ARM || ARM64
 source "drivers/cpuidle/Kconfig.arm"
diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
index d3ef1d7..43701da 100644
--- a/drivers/cpuidle/sysfs.c
+++ b/drivers/cpuidle/sysfs.c
@@ -146,7 +146,7 @@ static DEVICE_ATTR(current_governor, 0644, show_current_governor,
  */
 int cpuidle_add_interface(struct device *dev)
 {
-	if (sysfs_switch)
+	if (IS_ENABLED(CONFIG_CPU_IDLE_SWITCH_GOV_IN_DEFAULT) || sysfs_switch)
 		cpuidle_attr_group.attrs = cpuidle_switch_attrs;
 
 	return sysfs_create_group(&dev->kobj, &cpuidle_attr_group);
-- 
1.7.12.4

Re: [RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Daniel Lezcano]

On 27/04/2020 12:17, Hanjun Guo wrote:
> For now cpuidle governor can be switched via sysfs only when the
> boot option "cpuidle_sysfs_switch" is passed, but it's important
> to switch the governor to adapt to different workloads, especially
> after TEO and haltpoll governor were introduced.
> 
> Introduce a CONFIG option to make cpuidle governor switchable to be
> the default behaviour, which will not break the boot option behaviour.
> 
> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> ---
>  drivers/cpuidle/Kconfig | 9 +++++++++
>  drivers/cpuidle/sysfs.c | 2 +-
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> index c0aeedd..c40cb40 100644
> --- a/drivers/cpuidle/Kconfig
> +++ b/drivers/cpuidle/Kconfig
> @@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
>  config DT_IDLE_STATES
>  	bool
>  
> +config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
> +	bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
> +	help
> +	  Make the CPU idle governor switchable at runtime, and make it as the
> +	  default behaviour even the boot option "cpuidle_sysfs_switch" is not
> +	  passed in cmdline.
> +
> +	  Say N if you unsure about this.

Well I wouldn't make this optional but just remove the sysfs_switch.

However, there is the '_ro' suffix when the option is not set. In order
to not break the existing tools, may be let both files co-exist and add
in the ABI/obselete the '_ro' file as candidate for removal ?


>  menu "ARM CPU Idle Drivers"
>  depends on ARM || ARM64
>  source "drivers/cpuidle/Kconfig.arm"
> diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c
> index d3ef1d7..43701da 100644
> --- a/drivers/cpuidle/sysfs.c
> +++ b/drivers/cpuidle/sysfs.c
> @@ -146,7 +146,7 @@ static DEVICE_ATTR(current_governor, 0644, show_current_governor,
>   */
>  int cpuidle_add_interface(struct device *dev)
>  {
> -	if (sysfs_switch)
> +	if (IS_ENABLED(CONFIG_CPU_IDLE_SWITCH_GOV_IN_DEFAULT) || sysfs_switch)
>  		cpuidle_attr_group.attrs = cpuidle_switch_attrs;
>  
>  	return sysfs_create_group(&dev->kobj, &cpuidle_attr_group);
> 


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

RE: [RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Doug Smythies]

I very much support this RFC.
I have been running only with "cpuidle_sysfs_switch" for about 2 years.

Some changes would be required for the documentation files also.

On 2020.04.27 06:37 Daniel Lezcano wrote:
> On 27/04/2020 12:17, Hanjun Guo wrote:
>> For now cpuidle governor can be switched via sysfs only when the
>> boot option "cpuidle_sysfs_switch" is passed, but it's important
>> to switch the governor to adapt to different workloads, especially
>> after TEO and haltpoll governor were introduced.
>> 
>> Introduce a CONFIG option to make cpuidle governor switchable to be
>> the default behaviour, which will not break the boot option behaviour.
>> 
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> ---
>>  drivers/cpuidle/Kconfig | 9 +++++++++
>>  drivers/cpuidle/sysfs.c | 2 +-
>>  2 files changed, 10 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
>> index c0aeedd..c40cb40 100644
>> --- a/drivers/cpuidle/Kconfig
>> +++ b/drivers/cpuidle/Kconfig
>> @@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
>>  config DT_IDLE_STATES
>>  	bool
>>  
>> +config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
>> +	bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
>> +	help
>> +	  Make the CPU idle governor switchable at runtime, and make it as the
>> +	  default behaviour even the boot option "cpuidle_sysfs_switch" is not
>> +	  passed in cmdline.
>> +
>> +	  Say N if you unsure about this.
>
> Well I wouldn't make this optional but just remove the sysfs_switch.

Agree.

> However, there is the '_ro' suffix when the option is not set. In order
> to not break the existing tools, may be let both files co-exist and add
> in the ABI/obselete the '_ro' file as candidate for removal ?

I do not like this _ro thing, and got hit by it with turbostat one time.
Agree it should be made a candidate for removal.

Re: [RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Hanjun Guo]

On 2020/4/27 21:36, Daniel Lezcano wrote:
> On 27/04/2020 12:17, Hanjun Guo wrote:
>> For now cpuidle governor can be switched via sysfs only when the
>> boot option "cpuidle_sysfs_switch" is passed, but it's important
>> to switch the governor to adapt to different workloads, especially
>> after TEO and haltpoll governor were introduced.
>>
>> Introduce a CONFIG option to make cpuidle governor switchable to be
>> the default behaviour, which will not break the boot option behaviour.
>>
>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>> ---
>>   drivers/cpuidle/Kconfig | 9 +++++++++
>>   drivers/cpuidle/sysfs.c | 2 +-
>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
>> index c0aeedd..c40cb40 100644
>> --- a/drivers/cpuidle/Kconfig
>> +++ b/drivers/cpuidle/Kconfig
>> @@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
>>   config DT_IDLE_STATES
>>   	bool
>>   
>> +config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
>> +	bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
>> +	help
>> +	  Make the CPU idle governor switchable at runtime, and make it as the
>> +	  default behaviour even the boot option "cpuidle_sysfs_switch" is not
>> +	  passed in cmdline.
>> +
>> +	  Say N if you unsure about this.
> 
> Well I wouldn't make this optional but just remove the sysfs_switch.

That's my first solution but I send this RFC patch as it's less
aggressive :), I'm happy to get your comments to remove the
sysfs_switch, I will prepare patches for this solution.

> 
> However, there is the '_ro' suffix when the option is not set. In order
> to not break the existing tools, may be let both files co-exist and add
> in the ABI/obselete the '_ro' file as candidate for removal ?

Agreed.

Thanks
Hanjun

Re: [RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Hanjun Guo]

On 2020/4/27 22:41, Doug Smythies wrote:
> I very much support this RFC.
> I have been running only with "cpuidle_sysfs_switch" for about 2 years.

Thanks, glad to hear that switch cpuidle governor at
runtime works for years.

> 
> Some changes would be required for the documentation files also.

I will update them in next version.

> 
> On 2020.04.27 06:37 Daniel Lezcano wrote:
>> On 27/04/2020 12:17, Hanjun Guo wrote:
>>> For now cpuidle governor can be switched via sysfs only when the
>>> boot option "cpuidle_sysfs_switch" is passed, but it's important
>>> to switch the governor to adapt to different workloads, especially
>>> after TEO and haltpoll governor were introduced.
>>>
>>> Introduce a CONFIG option to make cpuidle governor switchable to be
>>> the default behaviour, which will not break the boot option behaviour.
>>>
>>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>>> ---
>>>   drivers/cpuidle/Kconfig | 9 +++++++++
>>>   drivers/cpuidle/sysfs.c | 2 +-
>>>   2 files changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
>>> index c0aeedd..c40cb40 100644
>>> --- a/drivers/cpuidle/Kconfig
>>> +++ b/drivers/cpuidle/Kconfig
>>> @@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
>>>   config DT_IDLE_STATES
>>>   	bool
>>>   
>>> +config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
>>> +	bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
>>> +	help
>>> +	  Make the CPU idle governor switchable at runtime, and make it as the
>>> +	  default behaviour even the boot option "cpuidle_sysfs_switch" is not
>>> +	  passed in cmdline.
>>> +
>>> +	  Say N if you unsure about this.
>>
>> Well I wouldn't make this optional but just remove the sysfs_switch.
> 
> Agree.
> 
>> However, there is the '_ro' suffix when the option is not set. In order
>> to not break the existing tools, may be let both files co-exist and add
>> in the ABI/obselete the '_ro' file as candidate for removal ?
> 
> I do not like this _ro thing, and got hit by it with turbostat one time.
> Agree it should be made a candidate for removal.

OK, I will prepare another RFC patch set to remove sysfs_switch.

Thanks
Hanjun

Re: [RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Rafael J. Wysocki]

On Tue, Apr 28, 2020 at 4:48 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>
> On 2020/4/27 22:41, Doug Smythies wrote:
> > I very much support this RFC.
> > I have been running only with "cpuidle_sysfs_switch" for about 2 years.
>
> Thanks, glad to hear that switch cpuidle governor at
> runtime works for years.
>
> >
> > Some changes would be required for the documentation files also.
>
> I will update them in next version.
>
> >
> > On 2020.04.27 06:37 Daniel Lezcano wrote:
> >> On 27/04/2020 12:17, Hanjun Guo wrote:
> >>> For now cpuidle governor can be switched via sysfs only when the
> >>> boot option "cpuidle_sysfs_switch" is passed, but it's important
> >>> to switch the governor to adapt to different workloads, especially
> >>> after TEO and haltpoll governor were introduced.
> >>>
> >>> Introduce a CONFIG option to make cpuidle governor switchable to be
> >>> the default behaviour, which will not break the boot option behaviour.
> >>>
> >>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
> >>> ---
> >>>   drivers/cpuidle/Kconfig | 9 +++++++++
> >>>   drivers/cpuidle/sysfs.c | 2 +-
> >>>   2 files changed, 10 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
> >>> index c0aeedd..c40cb40 100644
> >>> --- a/drivers/cpuidle/Kconfig
> >>> +++ b/drivers/cpuidle/Kconfig
> >>> @@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
> >>>   config DT_IDLE_STATES
> >>>     bool
> >>>
> >>> +config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
> >>> +   bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
> >>> +   help
> >>> +     Make the CPU idle governor switchable at runtime, and make it as the
> >>> +     default behaviour even the boot option "cpuidle_sysfs_switch" is not
> >>> +     passed in cmdline.
> >>> +
> >>> +     Say N if you unsure about this.
> >>
> >> Well I wouldn't make this optional but just remove the sysfs_switch.
> >
> > Agree.
> >
> >> However, there is the '_ro' suffix when the option is not set. In order
> >> to not break the existing tools, may be let both files co-exist and add
> >> in the ABI/obselete the '_ro' file as candidate for removal ?
> >
> > I do not like this _ro thing, and got hit by it with turbostat one time.
> > Agree it should be made a candidate for removal.
>
> OK, I will prepare another RFC patch set to remove sysfs_switch.

I would just do it in one series, though, as suggested by Daniel.

Also, I wouldn't worry about the _ro thing too much, as the changes
effectively make "cpuidle_sysfs_switch" be the default (and the tools
should be able to cope with "cpuidle_sysfs_switch" anyway) without an
alternative (but who cares?).

Cheers!

Re: [RFC PATCH] cpuidle: Make cpuidle governor switchable to be the default behaviour

[Hanjun Guo]

On 2020/4/29 18:46, Rafael J. Wysocki wrote:
> On Tue, Apr 28, 2020 at 4:48 AM Hanjun Guo <guohanjun@huawei.com> wrote:
>>
>> On 2020/4/27 22:41, Doug Smythies wrote:
>>> I very much support this RFC.
>>> I have been running only with "cpuidle_sysfs_switch" for about 2 years.
>>
>> Thanks, glad to hear that switch cpuidle governor at
>> runtime works for years.
>>
>>>
>>> Some changes would be required for the documentation files also.
>>
>> I will update them in next version.
>>
>>>
>>> On 2020.04.27 06:37 Daniel Lezcano wrote:
>>>> On 27/04/2020 12:17, Hanjun Guo wrote:
>>>>> For now cpuidle governor can be switched via sysfs only when the
>>>>> boot option "cpuidle_sysfs_switch" is passed, but it's important
>>>>> to switch the governor to adapt to different workloads, especially
>>>>> after TEO and haltpoll governor were introduced.
>>>>>
>>>>> Introduce a CONFIG option to make cpuidle governor switchable to be
>>>>> the default behaviour, which will not break the boot option behaviour.
>>>>>
>>>>> Signed-off-by: Hanjun Guo <guohanjun@huawei.com>
>>>>> ---
>>>>>    drivers/cpuidle/Kconfig | 9 +++++++++
>>>>>    drivers/cpuidle/sysfs.c | 2 +-
>>>>>    2 files changed, 10 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
>>>>> index c0aeedd..c40cb40 100644
>>>>> --- a/drivers/cpuidle/Kconfig
>>>>> +++ b/drivers/cpuidle/Kconfig
>>>>> @@ -47,6 +47,15 @@ config CPU_IDLE_GOV_HALTPOLL
>>>>>    config DT_IDLE_STATES
>>>>>      bool
>>>>>
>>>>> +config CPU_IDLE_SWITCH_GOV_IN_DEFAULT
>>>>> +   bool "Switch the CPU idle governor via sysfs at runtime in default behaviour"
>>>>> +   help
>>>>> +     Make the CPU idle governor switchable at runtime, and make it as the
>>>>> +     default behaviour even the boot option "cpuidle_sysfs_switch" is not
>>>>> +     passed in cmdline.
>>>>> +
>>>>> +     Say N if you unsure about this.
>>>>
>>>> Well I wouldn't make this optional but just remove the sysfs_switch.
>>>
>>> Agree.
>>>
>>>> However, there is the '_ro' suffix when the option is not set. In order
>>>> to not break the existing tools, may be let both files co-exist and add
>>>> in the ABI/obselete the '_ro' file as candidate for removal ?
>>>
>>> I do not like this _ro thing, and got hit by it with turbostat one time.
>>> Agree it should be made a candidate for removal.
>>
>> OK, I will prepare another RFC patch set to remove sysfs_switch.
> 
> I would just do it in one series, though, as suggested by Daniel.

OK, will sent out the patchset today.

> 
> Also, I wouldn't worry about the _ro thing too much, as the changes
> effectively make "cpuidle_sysfs_switch" be the default (and the tools
> should be able to cope with "cpuidle_sysfs_switch" anyway) without an
> alternative (but who cares?).

Thanks
Hanjun