| Message ID | 1587982637-33618-1-git-send-email-guohanjun@huawei.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
| Series | [RFC] cpuidle: Make cpuidle governor switchable to be the default behaviour | expand |
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); >
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.
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
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
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!
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
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);
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(-)