CPUFreq: Implement per policy instances of governors

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 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 04aab05..40f7083 100644
@@ -1081,6 +1081,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;

@@ -1669,7 +1671,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;
 	struct cpufreq_driver *driver = rcu_dereference(cpufreq_driver);
 
 	pr_debug("setting new policy for CPU %u: %u - %u kHz\n", policy->cpu,

@@ -1724,18 +1726,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 +1 -0

index a22944c..3b822ce 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                        *

Quoted 3 lines · Viresh Kumar

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.

Are those new events NOPs now?

include/linux/cpufreq.h static inline unsigned long cpufreq_scale(unsigned long old, u_int div, u_int mu · line 186

Quoted 2 lines

include/linux/cpufreq.h +2 -0

+#define CPUFREQ_GOV_POLICY_INIT      4
+#define CPUFREQ_GOV_POLICY_EXIT      4

Why don't you use different values here?

If you need only one value, one #define should be sufficient.

Quoted 7 lines · Rafael J. Wysocki

Quoted 3 lines · Viresh Kumar

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.

Are those new events NOPs now?

No. These are used atleast for ondemand & conservative.

Quoted 6 lines

include/linux/cpufreq.h static inline unsigned long cpufreq_scale(unsigned long old, u_int div, u_int mu · line 186

Quoted 2 lines

include/linux/cpufreq.h +2 -0

+#define CPUFREQ_GOV_POLICY_INIT      4
+#define CPUFREQ_GOV_POLICY_EXIT      4

Why don't you use different values here?

If you need only one value, one #define should be sufficient.

Naah, something happened... I got fixed this locally but couldn't get that into the commit somehow. EXIT should have 5 here.

Quoted 1 line · Viresh Kumar

No. These are used atleast for ondemand & conservative.

They will be after the next patch, you mean? :-)

Well, it appeared so from the next patch ...

Why don't you use different values here?

If you need only one value, one #define should be sufficient.

have_multiple_policies is required by platforms having multiple clock-domains for cpus, i.e. supporting multiple policies for cpus. This patch adds in a Kconfig option for enabling execution of this code.

Reported-by Borislav Petkov <[email protected]>

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

---
 drivers/cpufreq/Kconfig | 3 +++
 include/linux/cpufreq.h | 4 ++++
 2 files changed, 7 insertions(+)

drivers/cpufreq/Kconfig +0 -0

index cbcb21e..e6e6939 100644

drivers/cpufreq/Kconfig +0 -0

 config CPU_FREQ_STAT
 	tristate "CPU frequency translation statistics"
 	select CPU_FREQ_TABLE

include/linux/cpufreq.h +0 -0

index c5ac9a5..0d84bfa 100644

drivers/cpufreq/Kconfig config CPU_FREQ_TABLE · line 28

Quoted 3 lines · Viresh Kumar

drivers/cpufreq/Kconfig +3 -0

+config CPU_FREQ_HAVE_MULTIPLE_POLICIES
+     bool
+

So I suppose some architectures will select this, right?

What architecture they are?

I'm not really sure I like this. ->

include/linux/cpufreq.h static inline bool policy_is_shared(struct cpufreq_policy *policy) · line 152

Quoted 9 lines

static inline struct kobject * get_governor_parent_kobj(struct cpufreq_policy *policy) {

include/linux/cpufreq.h +2 -0

+#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
      if (policy->have_multiple_policies)
              return &policy->kobj;
      else
+#endif
              return cpufreq_global_kobject;

-> I wonder why don't you arrange things so that policy->kobj is always returned, but it points to cpufreq_global_kobject in case there's only one (i.e. make policy->kobj a pointer)?

Quoted 7 lines · Rafael J. Wysocki

drivers/cpufreq/Kconfig config CPU_FREQ_TABLE · line 28

Quoted 3 lines · Viresh Kumar

drivers/cpufreq/Kconfig +3 -0

+config CPU_FREQ_HAVE_MULTIPLE_POLICIES
+     bool
+

So I suppose some architectures will select this, right?

Yes. And they have to enable have_multiple_policies too from their drivers init code.

Quoted 1 line

What architecture they are?

Atleast all big.LITTLE SoCs. Or any other SoC that has multiple policy structs alive at any time.

Quoted 15 lines

I'm not really sure I like this. ->

include/linux/cpufreq.h static inline bool policy_is_shared(struct cpufreq_policy *policy) · line 152

Quoted 9 lines

static inline struct kobject * get_governor_parent_kobj(struct cpufreq_policy *policy) {

include/linux/cpufreq.h +2 -0

+#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
      if (policy->have_multiple_policies)
              return &policy->kobj;
      else
+#endif
              return cpufreq_global_kobject;

-> I wonder why don't you arrange things so that policy->kobj is always returned, but it points to cpufreq_global_kobject in case there's only one (i.e. make policy->kobj a pointer)?

policy->kobj is a struct instance rather than a pointer and it is widely used within cpufreq.c.

If you don't like this one then we can add another entry into struct policy like: gov_sysfs_parent.

Quoted 2 lines · Viresh Kumar

If you don't like this one then we can add another entry into struct policy like: gov_sysfs_parent.

I don't know. This is going to look kind of ugly this way or another I think.

Maybe I'll figure out something ...

We don't need to keep two structures for file attributes, global_attr and freq_attr. Lets use freq_attr only for cpufreq core and drivers.

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

---
 drivers/cpufreq/acpi-cpufreq.c |  9 ++++-----
 drivers/cpufreq/intel_pstate.c | 30 +++++++++++++++---------------
 include/linux/cpufreq.h        | 16 ----------------
 3 files changed, 19 insertions(+), 36 deletions(-)

drivers/cpufreq/acpi-cpufreq.c +4 -5

index 937bc28..14219c3 100644
@@ -160,19 +160,18 @@ static ssize_t _store_boost(const char *buf, size_t count)
 	return count;
 }
 
-static ssize_t store_global_boost(struct kobject *kobj, struct attribute *attr,
-				  const char *buf, size_t count)
+static ssize_t store_global_boost(struct cpufreq_policy *policy,
+		const char *buf, size_t count)
 {
 	return _store_boost(buf, count);
 }
 
-static ssize_t show_global_boost(struct kobject *kobj,
-				 struct attribute *attr, char *buf)
+static ssize_t show_global_boost(struct cpufreq_policy *policy, char *buf)
 {
 	return sprintf(buf, "%u\n", boost_enabled);
 }
 
-static struct global_attr global_boost = __ATTR(boost, 0644,
+static struct freq_attr global_boost = __ATTR(boost, 0644,
 						show_global_boost,
 						store_global_boost);
 

drivers/cpufreq/intel_pstate.c +0 -2

index 86ad482..0f2d6a0 100644
@@ -273,15 +273,15 @@ static void intel_pstate_debug_expose_params(void)
 /************************** debugfs end ************************/
 
 /************************** sysfs begin ************************/
-#define show_one(file_name, object)					\
-	static ssize_t show_##file_name					\

drivers/cpufreq/intel_pstate.c +9 -9

-static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
-				const char *buf, size_t count)
+static ssize_t store_no_turbo(struct cpufreq_policy *policy, const char *buf,
+		size_t count)
 {
 	unsigned int input;
 	int ret;

@@ -293,8 +293,8 @@ static ssize_t store_no_turbo(struct kobject *a, struct attribute *b,
 	return count;
 }
 
-static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
-				const char *buf, size_t count)
+static ssize_t store_max_perf_pct(struct cpufreq_policy *policy,
+		const char *buf, size_t count)
 {
 	unsigned int input;
 	int ret;

@@ -307,8 +307,8 @@ static ssize_t store_max_perf_pct(struct kobject *a, struct attribute *b,
 	return count;
 }
 
-static ssize_t store_min_perf_pct(struct kobject *a, struct attribute *b,
-				const char *buf, size_t count)
+static ssize_t store_min_perf_pct(struct cpufreq_policy *policy,
+		const char *buf, size_t count)
 {
 	unsigned int input;
 	int ret;

@@ -325,9 +325,9 @@ show_one(no_turbo, no_turbo);
 show_one(max_perf_pct, max_perf_pct);
 show_one(min_perf_pct, min_perf_pct);
 
-define_one_global_rw(no_turbo);
-define_one_global_rw(max_perf_pct);
-define_one_global_rw(min_perf_pct);
+cpufreq_freq_attr_rw(no_turbo);
+cpufreq_freq_attr_rw(max_perf_pct);
+cpufreq_freq_attr_rw(min_perf_pct);
 
 static struct attribute *intel_pstate_attributes[] = {
 	&no_turbo.attr,

include/linux/cpufreq.h +0 -9

index 0d84bfa..a092fcb 100644
@@ -324,22 +324,6 @@ __ATTR(_name, _perm, show_##_name, NULL)
 static struct freq_attr _name =			\
 __ATTR(_name, 0644, show_##_name, store_##_name)
 
-struct global_attr {
-	struct attribute attr;
-	ssize_t (*show)(struct kobject *kobj,
-			struct attribute *attr, char *buf);
-	ssize_t (*store)(struct kobject *a, struct attribute *b,
-			 const char *c, size_t count);
-};
-
-#define define_one_global_ro(_name)		\

Why did you change all of the lines of this macro instead of changing just the one line you needed to change?

Please don't do that.

Quoted 2 lines · Rafael J. Wysocki

Why did you change all of the lines of this macro instead of changing just the one line you needed to change?

I didn't like the indentation used within the macro. So did it.

Quoted 4 lines · Viresh Kumar

Quoted 2 lines · Rafael J. Wysocki

Why did you change all of the lines of this macro instead of changing just the one line you needed to change?

Quoted 1 line

I didn't like the indentation used within the macro. So did it.

In general, things like that are for separate cleanup patches. If you mix functional changes with cleanups, poeple get confused and it's difficult to see what's needed and what's "optional".

I know it's tempting to fix stuff like that along with doing functional changes and I do that sometimes. Not very often, though, and with care.

This is targetted for 3.10-rc1 or linux-next just after the merge window.