Implement per policy instance of governor

PATCH In review Viresh Kumar 3 participants 10 messages Mar 27, 2013 – Mar 31, 2013 CPU frequency scaling framework

lore

Share & tools

lore

Viresh Kumar

author maintainer

2013-03-27 15:58 UTC #0 lore raw

v4 cover Implement per policy instance of governor

Hi Guys,

All patches are pushed here for others to apply (you can apply from mail to):
http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/governor-per-policy-v4

Currently, there can't be multiple instances of single governor_type. If we have a multi-package system, where we have multiple instances of struct policy (per package), we can't have multiple instances of same governor. i.e. We can't have multiple instances of ondemand governor for multiple packages.

Governors directory in sysfs is created at /sys/devices/system/cpu/cpufreq/ governor-name/. Which again reflects that there can be only one instance of a governor_type in the system.

This is a bottleneck for multicluster system, where we want different packages to use same governor type, but with different tunables.

This patchset is inclined towards fixing this issue. Now we will create governors directory in cpu/cpu*/cpufreq/<gov> for platforms which have multiple struct policy alive at any moment. Platform drivers requiring this feature must set have_governor_per_policy variable in their instance of cpufreq_driver. For others the interface is kept same: cpu/cpufreq/<gov>.

This is V4 of this patchset. V3 is already applied by Rafael in his linux-next branch. Jacob Shin reported some regressions with this patchset and when I went into testing it with his configuration I found more issues then what he reported.

To test these over linux-next you need to revert following first:
db9baec cpufreq: Get rid of "struct global_attr"
86bd6f0 cpufreq: governor: Implement per policy instances of governors 8ae67b1 cpufreq: Add per policy governor-init/exit infrastructure

I have tested this for following now and believe there are no more regressions with it:
- platform with a single policy instance or single group of cpu
- platform with multiple policies but which don't want per policy instance of governor
- platform with multiple policies and which want per policy instance of governor

I have tried with different settings and combinations of governors.

@Rafael: To simplify your life I have sorted out your branch and you can simply pickup the complete branch that I have pushed.

V3->V4:
- We have two instances of all show/store routines for ondemand/conservative governor. One for per-policy instance of governor and other for one governor instance for all policies.
- Dropped: db9baec cpufreq: Get rid of "struct global_attr".
- Fixed cpufreq_governor_dbs for multiple policies using same governor instance.
- Implemented few macro's in cpufreq_governor.h to make above stuff clean.
- Renamed have_multiple_policies to have_governor_per_policy
- Some more minor cleanups

Viresh Kumar (2):
cpufreq: Add per policy governor-init/exit infrastructure
cpufreq: governor: Implement per policy instances of governors

 drivers/cpufreq/cpufreq.c              |  36 ++++-
 drivers/cpufreq/cpufreq_conservative.c | 193 ++++++++++++++----------
 drivers/cpufreq/cpufreq_governor.c     | 212 +++++++++++++++++---------
 drivers/cpufreq/cpufreq_governor.h     | 117 +++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 263 ++++++++++++++++++++-------------
 include/linux/cpufreq.h                |  17 ++-
 6 files changed, 562 insertions(+), 276 deletions(-)

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author maintainer

2013-03-27 15:58 UTC #1 lore raw

PATCH v4 1/2 cpufreq: Add per policy governor-init/exit infrastructure

Governors directory in sysfs is created at /sys/devices/system/cpu/cpufreq/ governor-name/. Which again reflects that there can be only one instance of a governor_type in the system.

This is a bottleneck for multicluster system, where we want different packages to use same governor type, but with different tunables.

This patch is inclined towards providing this infrastructure. Because we are required to allocate governor's resources dynamically now, we must do it at policy creation and end. And so got CPUFREQ_GOV_POLICY_INIT/EXIT.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c | 21 ++++++++++++++++++---
 include/linux/cpufreq.h   |  9 ++++++---
 2 files changed, 24 insertions(+), 6 deletions(-)

drivers/cpufreq/cpufreq.c +18 -3

index c5996fe..08df7a1 100644
@@ -1070,6 +1070,8 @@ static int __cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
+		__cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
+
 		lock_policy_rwsem_read(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;

@@ -1651,7 +1653,7 @@ EXPORT_SYMBOL(cpufreq_get_policy);
 static int __cpufreq_set_policy(struct cpufreq_policy *data,
 				struct cpufreq_policy *policy)
 {
-	int ret = 0;
+	int ret = 0, failed = 1;
 
 	pr_debug("setting new policy for CPU %u: %u - %u kHz\n", policy->cpu,
 		policy->min, policy->max);

@@ -1705,18 +1707,31 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data,
 			pr_debug("governor switch\n");
 
 			/* end old governor */
-			if (data->governor)
+			if (data->governor) {
 				__cpufreq_governor(data, CPUFREQ_GOV_STOP);
+				__cpufreq_governor(data,
+						CPUFREQ_GOV_POLICY_EXIT);
+			}
 
 			/* start new governor */
 			data->governor = policy->governor;
-			if (__cpufreq_governor(data, CPUFREQ_GOV_START)) {
+			if (!__cpufreq_governor(data, CPUFREQ_GOV_POLICY_INIT)) {
+				if (!__cpufreq_governor(data, CPUFREQ_GOV_START))
+					failed = 0;
+				else
+					__cpufreq_governor(data,
+							CPUFREQ_GOV_POLICY_EXIT);
+			}
+
+			if (failed) {
 				/* new governor failed, so re-start old one */
 				pr_debug("starting governor %s failed\n",
 							data->governor->name);
 				if (old_gov) {
 					data->governor = old_gov;
 					__cpufreq_governor(data,
+							CPUFREQ_GOV_POLICY_INIT);
+					__cpufreq_governor(data,
 							   CPUFREQ_GOV_START);
 				}
 				ret = -EINVAL;

include/linux/cpufreq.h +6 -3

index a22944c..b7393b5 100644
@@ -106,6 +106,7 @@ struct cpufreq_policy {
 					 * governors are used */
 	unsigned int		policy; /* see above */
 	struct cpufreq_governor	*governor; /* see below */
+	void			*governor_data;
 
 	struct work_struct	update; /* if update_policy() needs to be
 					 * called, but you're in IRQ context */

@@ -178,9 +179,11 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div, u_int mu
  *                          CPUFREQ GOVERNORS                        *
  *********************************************************************/
 
-#define CPUFREQ_GOV_START  1
-#define CPUFREQ_GOV_STOP   2
-#define CPUFREQ_GOV_LIMITS 3
+#define CPUFREQ_GOV_START	1
+#define CPUFREQ_GOV_STOP	2
+#define CPUFREQ_GOV_LIMITS	3
+#define CPUFREQ_GOV_POLICY_INIT	4
+#define CPUFREQ_GOV_POLICY_EXIT	5
 
 struct cpufreq_governor {
 	char	name[CPUFREQ_NAME_LEN];

Signature · 1 line

1.7.12.rc2.18.g61b472e

Viresh Kumar

author maintainer

2013-03-27 15:58 UTC #2 lore raw

PATCH v4 2/2 cpufreq: governor: Implement per policy instances of governors

In reply to Viresh Kumar

Governors directory in sysfs is created at /sys/devices/system/cpu/cpufreq/ governor-name/. Which again reflects that there can be only one instance of a governor_type in the system.

This is a bottleneck for multicluster system, where we want different packages to use same governor type, but with different tunables.

This patch uses the infrastructure provided by earlier patch and implements init/exit routines for ondemand and conservative governors.

Signed-off-by Viresh Kumar <[email protected]>

---
 drivers/cpufreq/cpufreq.c              |  15 +-
 drivers/cpufreq/cpufreq_conservative.c | 193 ++++++++++++++----------
 drivers/cpufreq/cpufreq_governor.c     | 212 +++++++++++++++++---------
 drivers/cpufreq/cpufreq_governor.h     | 117 +++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 263 ++++++++++++++++++++-------------
 include/linux/cpufreq.h                |   8 +
 6 files changed, 538 insertions(+), 270 deletions(-)

drivers/cpufreq/cpufreq.c +11 -4

index 08df7a1..85963fc 100644
@@ -128,6 +128,11 @@ void disable_cpufreq(void)
 static LIST_HEAD(cpufreq_governor_list);
 static DEFINE_MUTEX(cpufreq_governor_mutex);
 
+bool have_governor_per_policy(void)
+{
+	return cpufreq_driver->have_governor_per_policy;
+}
+
 static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs)
 {
 	struct cpufreq_policy *data;

@@ -1546,10 +1551,12 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 	ret = policy->governor->governor(policy, event);
 
-	if (event == CPUFREQ_GOV_START)
-		policy->governor->initialized++;
-	else if (event == CPUFREQ_GOV_STOP)
-		policy->governor->initialized--;
+	if (!ret) {
+		if (event == CPUFREQ_GOV_POLICY_INIT)
+			policy->governor->initialized++;
+		else if (event == CPUFREQ_GOV_POLICY_EXIT)
+			policy->governor->initialized--;
+	}
 
 	/* we keep one module reference alive for
 			each CPU governed by this CPU */

drivers/cpufreq/cpufreq_conservative.c +118 ~~-75~~

353 lines not on this page · raw message

drivers/cpufreq/cpufreq_governor.c +142 ~~-70~~

332 lines not on this page · raw message

drivers/cpufreq/cpufreq_governor.h +102 ~~-15~~

164 lines not on this page · raw message

drivers/cpufreq/cpufreq_ondemand.c +157 ~~-106~~

486 lines not on this page · raw message

include/linux/cpufreq.h +8 -0

index b7393b5..4bbc572 100644
@@ -232,6 +232,13 @@ struct cpufreq_driver {
 	struct module           *owner;
 	char			name[CPUFREQ_NAME_LEN];
 	u8			flags;
+	/*
+	 * This should be set by platforms having multiple clock-domains, i.e.
+	 * supporting multiple policies. With this sysfs directories of governor
+	 * would be created in cpu/cpu<num>/cpufreq/ directory and so they can
+	 * use the same governor with different tunables for different clusters.
+	 */
+	bool			have_governor_per_policy;
 
 	/* needed by all drivers */
 	int	(*init)		(struct cpufreq_policy *policy);

@@ -332,6 +339,7 @@ const char *cpufreq_get_current_driver(void);
  *********************************************************************/
 int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu);
 int cpufreq_update_policy(unsigned int cpu);
+bool have_governor_per_policy(void);
 
 #ifdef CONFIG_CPU_FREQ
 /* query the current CPU frequency (in kHz). If zero, cpufreq couldn't detect it */

Signature · 1 line

1.7.12.rc2.18.g61b472e

Jacob Shin

Tested-by on the v4 series

2013-03-27 17:19 UTC #3 lore raw

In reply to Viresh Kumar

Quoted 60 lines · Viresh Kumar

Hi Guys,
All patches are pushed here for others to apply (you can apply from mail to):
http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/governor-per-policy-v4
Currently, there can't be multiple instances of single governor_type. If we have a multi-package system, where we have multiple instances of struct policy (per package), we can't have multiple instances of same governor. i.e. We can't have multiple instances of ondemand governor for multiple packages.
Governors directory in sysfs is created at /sys/devices/system/cpu/cpufreq/ governor-name/. Which again reflects that there can be only one instance of a governor_type in the system.
This is a bottleneck for multicluster system, where we want different packages to use same governor type, but with different tunables.
This patchset is inclined towards fixing this issue. Now we will create governors directory in cpu/cpu*/cpufreq/<gov> for platforms which have multiple struct policy alive at any moment. Platform drivers requiring this feature must set have_governor_per_policy variable in their instance of cpufreq_driver. For others the interface is kept same: cpu/cpufreq/<gov>.
This is V4 of this patchset. V3 is already applied by Rafael in his linux-next branch. Jacob Shin reported some regressions with this patchset and when I went into testing it with his configuration I found more issues then what he reported.
To test these over linux-next you need to revert following first:
db9baec cpufreq: Get rid of "struct global_attr"
86bd6f0 cpufreq: governor: Implement per policy instances of governors 8ae67b1 cpufreq: Add per policy governor-init/exit infrastructure
I have tested this for following now and believe there are no more regressions with it:
- platform with a single policy instance or single group of cpu
- platform with multiple policies but which don't want per policy instance of governor
- platform with multiple policies and which want per policy instance of governor
I have tried with different settings and combinations of governors.
@Rafael: To simplify your life I have sorted out your branch and you can simply pickup the complete branch that I have pushed.
V3->V4:
- We have two instances of all show/store routines for ondemand/conservative governor. One for per-policy instance of governor and other for one governor instance for all policies.
- Dropped: db9baec cpufreq: Get rid of "struct global_attr".
- Fixed cpufreq_governor_dbs for multiple policies using same governor instance.
- Implemented few macro's in cpufreq_governor.h to make above stuff clean.
- Renamed have_multiple_policies to have_governor_per_policy
- Some more minor cleanups
Viresh Kumar (2):
cpufreq: Add per policy governor-init/exit infrastructure
cpufreq: governor: Implement per policy instances of governors

Quoted 12 lines

 drivers/cpufreq/cpufreq.c              |  36 ++++-
 drivers/cpufreq/cpufreq_conservative.c | 193 ++++++++++++++----------
 drivers/cpufreq/cpufreq_governor.c     | 212 +++++++++++++++++---------
 drivers/cpufreq/cpufreq_governor.h     | 117 +++++++++++++--
 drivers/cpufreq/cpufreq_ondemand.c     | 263 ++++++++++++++++++++-------------
 include/linux/cpufreq.h                |  17 ++-
 6 files changed, 562 insertions(+), 276 deletions(-)

Signature · 1 line

1.7.12.rc2.18.g61b472e

V4 tested good on AMD machine that the regression was reported on.

Thanks,

Tested-by Jacob Shin <[email protected]>

Rafael J. Wysocki

2013-03-30 02:09 UTC #4 lore raw

In reply to Viresh Kumar

Quoted 41 lines · Viresh Kumar

Hi Guys,
All patches are pushed here for others to apply (you can apply from mail to):
http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/governor-per-policy-v4
Currently, there can't be multiple instances of single governor_type. If we have a multi-package system, where we have multiple instances of struct policy (per package), we can't have multiple instances of same governor. i.e. We can't have multiple instances of ondemand governor for multiple packages.
Governors directory in sysfs is created at /sys/devices/system/cpu/cpufreq/ governor-name/. Which again reflects that there can be only one instance of a governor_type in the system.
This is a bottleneck for multicluster system, where we want different packages to use same governor type, but with different tunables.
This patchset is inclined towards fixing this issue. Now we will create governors directory in cpu/cpu*/cpufreq/<gov> for platforms which have multiple struct policy alive at any moment. Platform drivers requiring this feature must set have_governor_per_policy variable in their instance of cpufreq_driver. For others the interface is kept same: cpu/cpufreq/<gov>.
This is V4 of this patchset. V3 is already applied by Rafael in his linux-next branch. Jacob Shin reported some regressions with this patchset and when I went into testing it with his configuration I found more issues then what he reported.
To test these over linux-next you need to revert following first:
db9baec cpufreq: Get rid of "struct global_attr"
86bd6f0 cpufreq: governor: Implement per policy instances of governors 8ae67b1 cpufreq: Add per policy governor-init/exit infrastructure
I have tested this for following now and believe there are no more regressions with it:
- platform with a single policy instance or single group of cpu
- platform with multiple policies but which don't want per policy instance of governor
- platform with multiple policies and which want per policy instance of governor

Quoted 4 lines

I have tried with different settings and combinations of governors.
@Rafael: To simplify your life I have sorted out your branch and you can simply pickup the complete branch that I have pushed.

I've applied these two patches and re-applied the queue on top of them.

All is in my linux-next branch now.

Thanks,
Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Viresh Kumar

author maintainer

2013-03-30 03:36 UTC #5 lore raw

Quoted 10 lines · Rafael J. Wysocki

Quoted 4 lines · Viresh Kumar
To test these over linux-next you need to revert following first:
db9baec cpufreq: Get rid of "struct global_attr"
86bd6f0 cpufreq: governor: Implement per policy instances of governors 8ae67b1 cpufreq: Add per policy governor-init/exit infrastructure
I've applied these two patches and re-applied the queue on top of them.
All is in my linux-next branch now.

You were supposed to remove below patch too, which you missed :)

c7baa5e cpufreq: Get rid of "struct global_attr"

Rafael J. Wysocki

2013-03-30 22:01 UTC #6 lore raw

Quoted 8 lines · Viresh Kumar

Quoted 6 lines · Rafael J. Wysocki
Quoted 4 lines · Viresh Kumar
To test these over linux-next you need to revert following first:
db9baec cpufreq: Get rid of "struct global_attr"
86bd6f0 cpufreq: governor: Implement per policy instances of governors 8ae67b1 cpufreq: Add per policy governor-init/exit infrastructure

Quoted 7 lines

Quoted 3 lines
I've applied these two patches and re-applied the queue on top of them.
All is in my linux-next branch now.
You were supposed to remove below patch too, which you missed :)
c7baa5e cpufreq: Get rid of "struct global_attr"

OK, I'll remove it.

And by the way, I'm no longer amused with the mess going on in the cpufreq land.

Please send me *patches*. I'm not interested in git pointers.

Moreover, please send those patches in a way allowing me to figure out the ordering without looking into your git tree.

If you have a number of outstanding cpufreq patches for me to apply, please create a patch series out of them and send it.

Thanks,
Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Viresh Kumar

author maintainer

2013-03-31 01:30 UTC #7 lore raw

Quoted 1 line · Rafael J. Wysocki

OK, I'll remove it.

Thanks.

Quoted 3 lines

And by the way, I'm no longer amused with the mess going on in the cpufreq land.
Please send me *patches*. I'm not interested in git pointers.

Hmm.

Quoted 2 lines

Moreover, please send those patches in a way allowing me to figure out the ordering without looking into your git tree.

Yes, i was planning to do so.

Quoted 2 lines

If you have a number of outstanding cpufreq patches for me to apply, please create a patch series out of them and send it.

Do you mean, i should merge all stuff i have queued up for 3.10 in a single patchset?
Like for-3.10? Will do.

Rafael J. Wysocki

2013-03-31 01:41 UTC #8 lore raw

Quoted 14 lines · Viresh Kumar

Quoted 1 line · Rafael J. Wysocki
OK, I'll remove it.
Thanks.
Quoted 3 lines
And by the way, I'm no longer amused with the mess going on in the cpufreq land.
Please send me *patches*. I'm not interested in git pointers.
Hmm.
Quoted 2 lines
Moreover, please send those patches in a way allowing me to figure out the ordering without looking into your git tree.

Quoted 8 lines

Yes, i was planning to do so.
Quoted 2 lines
If you have a number of outstanding cpufreq patches for me to apply, please create a patch series out of them and send it.
Do you mean, i should merge all stuff i have queued up for 3.10 in a single patchset?
Like for-3.10? Will do.

Yes, thanks.

But the patches moving cpufreq drivers into drivers/cpufreq from somewhere else need each an ACK from the maintainer of that other place. I won't take any patches like that without AKCs.

Thanks,
Rafael

Signature · 2 lines

I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Viresh Kumar

author maintainer

2013-03-31 03:47 UTC #9 lore raw

Quoted 5 lines · Rafael J. Wysocki

Yes, thanks.
But the patches moving cpufreq drivers into drivers/cpufreq from somewhere else need each an ACK from the maintainer of that other place. I won't take any patches like that without AKCs.

Okay. I will ask respective maintainers (Who haven't acked until now) to provide one.