[PATCH V4 0/3] cpufreq: Allow default governor on cmdline and fix locking issues

[PATCH V4 0/3] cpufreq: Allow default governor on cmdline and fix locking issues

[Viresh Kumar]

Hi,

I have picked Quentin's series over my patch, modified both and tested.

V3->V4:
- Do __module_get() for cpufreq_default_governor() case as well and get
  rid of an extra variable.
- Use a single character array, default_governor, instead of two of them.

V2->V3:
- default_governor is a string now and we don't set it on governor
  registration or unregistration anymore.
- Fixed locking issues in cpufreq_init_policy().

--
Viresh

Original cover letter fro Quentin:

This series enables users of prebuilt kernels (e.g. distro kernels) to
specify their CPUfreq governor of choice using the kernel command line,
instead of having to wait for the system to fully boot to userspace to
switch using the sysfs interface. This is helpful for 2 reasons:
  1. users get to choose the governor that runs during the actual boot;
  2. it simplifies the userspace boot procedure a bit (one less thing to
     worry about).

To enable this, the first patch moves all governor init calls to
core_initcall, to make sure they are registered by the time the drivers
probe. This should be relatively low impact as registering a governor
is a simple procedure (it gets added to a llist), and all governors
already load at core_initcall anyway when they're set as the default
in Kconfig. This also allows to clean-up the governors' init/exit code,
and reduces boilerplate.

The second patch introduces the new command line parameter, inspired by
its cpuidle counterpart. More details can be found in the respective
patch headers.

Changes in v2:
 - added Viresh's ack to patch 01
 - moved the assignment of 'default_governor' in patch 02 to the governor
   registration path instead of the driver registration (Viresh)

Quentin Perret (2):
  cpufreq: Register governors at core_initcall
  cpufreq: Specify default governor on command line

Viresh Kumar (1):
  cpufreq: Fix locking issues with governors

 .../admin-guide/kernel-parameters.txt         |  5 ++
 Documentation/admin-guide/pm/cpufreq.rst      |  6 +-
 .../platforms/cell/cpufreq_spudemand.c        | 26 +-----
 drivers/cpufreq/cpufreq.c                     | 87 ++++++++++++-------
 drivers/cpufreq/cpufreq_conservative.c        | 22 ++---
 drivers/cpufreq/cpufreq_ondemand.c            | 24 ++---
 drivers/cpufreq/cpufreq_performance.c         | 14 +--
 drivers/cpufreq/cpufreq_powersave.c           | 18 +---
 drivers/cpufreq/cpufreq_userspace.c           | 18 +---
 include/linux/cpufreq.h                       | 14 +++
 kernel/sched/cpufreq_schedutil.c              |  6 +-
 11 files changed, 100 insertions(+), 140 deletions(-)

-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V4 3/3] cpufreq: Specify default governor on command line

[Viresh Kumar]

From: Quentin Perret <qperret@google.com>

Currently, the only way to specify the default CPUfreq governor is via
Kconfig options, which suits users who can build the kernel themselves
perfectly.

However, for those who use a distro-like kernel (such as Android, with
the Generic Kernel Image project), the only way to use a different
default is to boot to userspace, and to then switch using the sysfs
interface. Being able to specify the default governor on the command
line, like is the case for cpuidle, would enable those users to specify
their governor of choice earlier on, and to simplify slighlty the
userspace boot procedure.

To support this use-case, add a kernel command line parameter enabling
to specify a default governor for CPUfreq, which takes precedence over
the builtin default.

This implementation has one notable limitation: the default governor
must be registered before the driver. This is solved for builtin
governors and drivers using appropriate *_initcall() functions. And in
the modular case, this must be reflected as a constraint on the module
loading order.

Signed-off-by: Quentin Perret <qperret@google.com>
[ Viresh: Converted 'default_governor' to a string and parsing it only
	  at initcall level, and several updates to
	  cpufreq_init_policy(). ]
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 .../admin-guide/kernel-parameters.txt         |  5 +++
 Documentation/admin-guide/pm/cpufreq.rst      |  6 ++--
 drivers/cpufreq/cpufreq.c                     | 31 +++++++++++++------
 3 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index fb95fad81c79..8deb5a89328a 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -703,6 +703,11 @@
 	cpufreq.off=1	[CPU_FREQ]
 			disable the cpufreq sub-system
 
+	cpufreq.default_governor=
+			[CPU_FREQ] Name of the default cpufreq governor or
+			policy to use. This governor must be registered in the
+			kernel before the cpufreq driver probes.
+
 	cpu_init_udelay=N
 			[X86] Delay for N microsec between assert and de-assert
 			of APIC INIT to start processors.  This delay occurs
diff --git a/Documentation/admin-guide/pm/cpufreq.rst b/Documentation/admin-guide/pm/cpufreq.rst
index 0c74a7784964..368e612145d2 100644
--- a/Documentation/admin-guide/pm/cpufreq.rst
+++ b/Documentation/admin-guide/pm/cpufreq.rst
@@ -147,9 +147,9 @@ CPUs in it.
 
 The next major initialization step for a new policy object is to attach a
 scaling governor to it (to begin with, that is the default scaling governor
-determined by the kernel configuration, but it may be changed later
-via ``sysfs``).  First, a pointer to the new policy object is passed to the
-governor's ``->init()`` callback which is expected to initialize all of the
+determined by the kernel command line or configuration, but it may be changed
+later via ``sysfs``).  First, a pointer to the new policy object is passed to
+the governor's ``->init()`` callback which is expected to initialize all of the
 data structures necessary to handle the given policy and, possibly, to add
 a governor ``sysfs`` interface to it.  Next, the governor is started by
 invoking its ``->start()`` callback.
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index e9e8200a0211..ad94b1d47ddb 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -50,6 +50,8 @@ static LIST_HEAD(cpufreq_governor_list);
 #define for_each_governor(__governor)				\
 	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
 
+static char default_governor[CPUFREQ_NAME_LEN];
+
 /**
  * The "cpufreq driver" - the arch- or hardware-dependent low
  * level driver of CPUFreq support, and its spinlock. This lock
@@ -1061,7 +1063,6 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void)
 
 static int cpufreq_init_policy(struct cpufreq_policy *policy)
 {
-	struct cpufreq_governor *def_gov = cpufreq_default_governor();
 	struct cpufreq_governor *gov = NULL;
 	unsigned int pol = CPUFREQ_POLICY_UNKNOWN;
 	int ret;
@@ -1071,21 +1072,27 @@ static int cpufreq_init_policy(struct cpufreq_policy *policy)
 		gov = get_governor(policy->last_governor);
 		if (gov) {
 			pr_debug("Restoring governor %s for cpu %d\n",
-				 policy->governor->name, policy->cpu);
-		} else if (def_gov) {
-			gov = def_gov;
-			__module_get(gov->owner);
+				 gov->name, policy->cpu);
 		} else {
-			return -ENODATA;
+			gov = get_governor(default_governor);
+		}
+
+		if (!gov) {
+			gov = cpufreq_default_governor();
+			if (!gov)
+				return -ENODATA;
+			__module_get(gov->owner);
 		}
+
 	} else {
+
 		/* Use the default policy if there is no last_policy. */
 		if (policy->last_policy) {
 			pol = policy->last_policy;
-		} else if (def_gov) {
-			pol = cpufreq_parse_policy(def_gov->name);
+		} else {
+			pol = cpufreq_parse_policy(default_governor);
 			/*
-			 * In case the default governor is neiter "performance"
+			 * In case the default governor is neither "performance"
 			 * nor "powersave", fall back to the initial policy
 			 * value set by the driver.
 			 */
@@ -2795,13 +2802,19 @@ EXPORT_SYMBOL_GPL(cpufreq_unregister_driver);
 
 static int __init cpufreq_core_init(void)
 {
+	struct cpufreq_governor *gov = cpufreq_default_governor();
+
 	if (cpufreq_disabled())
 		return -ENODEV;
 
 	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
 	BUG_ON(!cpufreq_global_kobject);
 
+	if (!strlen(default_governor))
+		strncpy(default_governor, gov->name, CPUFREQ_NAME_LEN);
+
 	return 0;
 }
 module_param(off, int, 0444);
+module_param_string(default_governor, default_governor, CPUFREQ_NAME_LEN, 0444);
 core_initcall(cpufreq_core_init);
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Quentin Perret]

On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
>  static int __init cpufreq_core_init(void)
>  {
> +	struct cpufreq_governor *gov = cpufreq_default_governor();
> +
>  	if (cpufreq_disabled())
>  		return -ENODEV;
>  
>  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
>  	BUG_ON(!cpufreq_global_kobject);
>  
> +	if (!strlen(default_governor))

Should we test '!strlen(default_governor) && gov' here actually?
We check the return value of cpufreq_default_governor() in
cpufreq_init_policy(), so I'm guessing we should do the same here to be
on the safe side.

> +		strncpy(default_governor, gov->name, CPUFREQ_NAME_LEN);
> +
>  	return 0;
>  }
>  module_param(off, int, 0444);
> +module_param_string(default_governor, default_governor, CPUFREQ_NAME_LEN, 0444);
>  core_initcall(cpufreq_core_init);
> -- 
> 2.25.0.rc1.19.g042ed3e048af


Other than that, the whole series looks good to me.

Thanks,
Quentin

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Viresh Kumar]

On 29-06-20, 10:44, Quentin Perret wrote:
> On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
> >  static int __init cpufreq_core_init(void)
> >  {
> > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > +
> >  	if (cpufreq_disabled())
> >  		return -ENODEV;
> >  
> >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> >  	BUG_ON(!cpufreq_global_kobject);
> >  
> > +	if (!strlen(default_governor))
> 
> Should we test '!strlen(default_governor) && gov' here actually?
> We check the return value of cpufreq_default_governor() in
> cpufreq_init_policy(), so I'm guessing we should do the same here to be
> on the safe side.

With the current setup (the Kconfig option being a choice which
selects one governor at least), it is not possible for gov to be NULL
here. And so I didn't worry about it :)

-- 
viresh

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Quentin Perret]

On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote:
> On 29-06-20, 10:44, Quentin Perret wrote:
> > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
> > >  static int __init cpufreq_core_init(void)
> > >  {
> > > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > > +
> > >  	if (cpufreq_disabled())
> > >  		return -ENODEV;
> > >  
> > >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> > >  	BUG_ON(!cpufreq_global_kobject);
> > >  
> > > +	if (!strlen(default_governor))
> > 
> > Should we test '!strlen(default_governor) && gov' here actually?
> > We check the return value of cpufreq_default_governor() in
> > cpufreq_init_policy(), so I'm guessing we should do the same here to be
> > on the safe side.
> 
> With the current setup (the Kconfig option being a choice which
> selects one governor at least), it is not possible for gov to be NULL
> here. And so I didn't worry about it :)

Right, so should we remove the check in cpufreq_init_policy() then?
I don't mind either way as long as we are consitent :)

Thanks,
Quentin

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Viresh Kumar]

On 29-06-20, 10:48, Quentin Perret wrote:
> On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote:
> > On 29-06-20, 10:44, Quentin Perret wrote:
> > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
> > > >  static int __init cpufreq_core_init(void)
> > > >  {
> > > > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > > > +
> > > >  	if (cpufreq_disabled())
> > > >  		return -ENODEV;
> > > >  
> > > >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> > > >  	BUG_ON(!cpufreq_global_kobject);
> > > >  
> > > > +	if (!strlen(default_governor))
> > > 
> > > Should we test '!strlen(default_governor) && gov' here actually?
> > > We check the return value of cpufreq_default_governor() in
> > > cpufreq_init_policy(), so I'm guessing we should do the same here to be
> > > on the safe side.
> > 
> > With the current setup (the Kconfig option being a choice which
> > selects one governor at least), it is not possible for gov to be NULL
> > here. And so I didn't worry about it :)
> 
> Right, so should we remove the check in cpufreq_init_policy() then?
> I don't mind either way as long as we are consitent :)

We can get rid of that as well.

-- 
viresh

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Quentin Perret]

On Monday 29 Jun 2020 at 10:48:25 (+0100), Quentin Perret wrote:
> On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote:
> > On 29-06-20, 10:44, Quentin Perret wrote:
> > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
> > > >  static int __init cpufreq_core_init(void)
> > > >  {
> > > > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > > > +
> > > >  	if (cpufreq_disabled())
> > > >  		return -ENODEV;
> > > >  
> > > >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> > > >  	BUG_ON(!cpufreq_global_kobject);
> > > >  
> > > > +	if (!strlen(default_governor))
> > > 
> > > Should we test '!strlen(default_governor) && gov' here actually?
> > > We check the return value of cpufreq_default_governor() in
> > > cpufreq_init_policy(), so I'm guessing we should do the same here to be
> > > on the safe side.
> > 
> > With the current setup (the Kconfig option being a choice which
> > selects one governor at least), it is not possible for gov to be NULL
> > here. And so I didn't worry about it :)
> 
> Right, so should we remove the check in cpufreq_init_policy() then?
> I don't mind either way as long as we are consitent :)

And actually maybe we should remove the weakly defined
cpufreq_default_governor() implementation too? That'd make sure we get a
link-time error if for some reason things change in the Kconfig options.

Thanks,
Quentin

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Viresh Kumar]

On 29-06-20, 10:50, Quentin Perret wrote:
> On Monday 29 Jun 2020 at 10:48:25 (+0100), Quentin Perret wrote:
> > On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote:
> > > On 29-06-20, 10:44, Quentin Perret wrote:
> > > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
> > > > >  static int __init cpufreq_core_init(void)
> > > > >  {
> > > > > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > > > > +
> > > > >  	if (cpufreq_disabled())
> > > > >  		return -ENODEV;
> > > > >  
> > > > >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> > > > >  	BUG_ON(!cpufreq_global_kobject);
> > > > >  
> > > > > +	if (!strlen(default_governor))
> > > > 
> > > > Should we test '!strlen(default_governor) && gov' here actually?
> > > > We check the return value of cpufreq_default_governor() in
> > > > cpufreq_init_policy(), so I'm guessing we should do the same here to be
> > > > on the safe side.
> > > 
> > > With the current setup (the Kconfig option being a choice which
> > > selects one governor at least), it is not possible for gov to be NULL
> > > here. And so I didn't worry about it :)
> > 
> > Right, so should we remove the check in cpufreq_init_policy() then?
> > I don't mind either way as long as we are consitent :)
> 
> And actually maybe we should remove the weakly defined
> cpufreq_default_governor() implementation too? That'd make sure we get a
> link-time error if for some reason things change in the Kconfig options.

That would be fine I believe. I will do all that in a separate patch
then and let this series go through with no more changes :)

-- 
viresh

Re: [PATCH V4 3/3] cpufreq: Specify default governor on command line

[Quentin Perret]

On Monday 29 Jun 2020 at 15:24:23 (+0530), Viresh Kumar wrote:
> On 29-06-20, 10:50, Quentin Perret wrote:
> > On Monday 29 Jun 2020 at 10:48:25 (+0100), Quentin Perret wrote:
> > > On Monday 29 Jun 2020 at 15:16:27 (+0530), Viresh Kumar wrote:
> > > > On 29-06-20, 10:44, Quentin Perret wrote:
> > > > > On Monday 29 Jun 2020 at 13:55:00 (+0530), Viresh Kumar wrote:
> > > > > >  static int __init cpufreq_core_init(void)
> > > > > >  {
> > > > > > +	struct cpufreq_governor *gov = cpufreq_default_governor();
> > > > > > +
> > > > > >  	if (cpufreq_disabled())
> > > > > >  		return -ENODEV;
> > > > > >  
> > > > > >  	cpufreq_global_kobject = kobject_create_and_add("cpufreq", &cpu_subsys.dev_root->kobj);
> > > > > >  	BUG_ON(!cpufreq_global_kobject);
> > > > > >  
> > > > > > +	if (!strlen(default_governor))
> > > > > 
> > > > > Should we test '!strlen(default_governor) && gov' here actually?
> > > > > We check the return value of cpufreq_default_governor() in
> > > > > cpufreq_init_policy(), so I'm guessing we should do the same here to be
> > > > > on the safe side.
> > > > 
> > > > With the current setup (the Kconfig option being a choice which
> > > > selects one governor at least), it is not possible for gov to be NULL
> > > > here. And so I didn't worry about it :)
> > > 
> > > Right, so should we remove the check in cpufreq_init_policy() then?
> > > I don't mind either way as long as we are consitent :)
> > 
> > And actually maybe we should remove the weakly defined
> > cpufreq_default_governor() implementation too? That'd make sure we get a
> > link-time error if for some reason things change in the Kconfig options.
> 
> That would be fine I believe. I will do all that in a separate patch
> then and let this series go through with no more changes :)

OK, that works for me.

Thanks!
Quentin

Re: [PATCH V4 0/3] cpufreq: Allow default governor on cmdline and fix locking issues

[Rafael J. Wysocki]

On Mon, Jun 29, 2020 at 10:58 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Hi,
>
> I have picked Quentin's series over my patch, modified both and tested.
>
> V3->V4:
> - Do __module_get() for cpufreq_default_governor() case as well and get
>   rid of an extra variable.
> - Use a single character array, default_governor, instead of two of them.
>
> V2->V3:
> - default_governor is a string now and we don't set it on governor
>   registration or unregistration anymore.
> - Fixed locking issues in cpufreq_init_policy().
>
> --
> Viresh
>
> Original cover letter fro Quentin:
>
> This series enables users of prebuilt kernels (e.g. distro kernels) to
> specify their CPUfreq governor of choice using the kernel command line,
> instead of having to wait for the system to fully boot to userspace to
> switch using the sysfs interface. This is helpful for 2 reasons:
>   1. users get to choose the governor that runs during the actual boot;
>   2. it simplifies the userspace boot procedure a bit (one less thing to
>      worry about).
>
> To enable this, the first patch moves all governor init calls to
> core_initcall, to make sure they are registered by the time the drivers
> probe. This should be relatively low impact as registering a governor
> is a simple procedure (it gets added to a llist), and all governors
> already load at core_initcall anyway when they're set as the default
> in Kconfig. This also allows to clean-up the governors' init/exit code,
> and reduces boilerplate.
>
> The second patch introduces the new command line parameter, inspired by
> its cpuidle counterpart. More details can be found in the respective
> patch headers.
>
> Changes in v2:
>  - added Viresh's ack to patch 01
>  - moved the assignment of 'default_governor' in patch 02 to the governor
>    registration path instead of the driver registration (Viresh)
>
> Quentin Perret (2):
>   cpufreq: Register governors at core_initcall
>   cpufreq: Specify default governor on command line
>
> Viresh Kumar (1):
>   cpufreq: Fix locking issues with governors
>
>  .../admin-guide/kernel-parameters.txt         |  5 ++
>  Documentation/admin-guide/pm/cpufreq.rst      |  6 +-
>  .../platforms/cell/cpufreq_spudemand.c        | 26 +-----
>  drivers/cpufreq/cpufreq.c                     | 87 ++++++++++++-------
>  drivers/cpufreq/cpufreq_conservative.c        | 22 ++---
>  drivers/cpufreq/cpufreq_ondemand.c            | 24 ++---
>  drivers/cpufreq/cpufreq_performance.c         | 14 +--
>  drivers/cpufreq/cpufreq_powersave.c           | 18 +---
>  drivers/cpufreq/cpufreq_userspace.c           | 18 +---
>  include/linux/cpufreq.h                       | 14 +++
>  kernel/sched/cpufreq_schedutil.c              |  6 +-
>  11 files changed, 100 insertions(+), 140 deletions(-)
>
> --

All three patches applied as 5.9 material, thanks!