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 patchset is inclined towards fixing this issue.

Viresh Kumar (4):
cpufreq: Don't check cpu_online(policy->cpu)
cpufreq: stats: Get rid of CPUFREQ_STATDEVICE_ATTR
cpufreq: Add per policy governor-init/exit infrastructure
cpufreq: governor: Implement per policy instances of governors

 drivers/cpufreq/cpufreq.c              |  41 ++++---
 drivers/cpufreq/cpufreq_conservative.c | 142 +++++++++++++----------
 drivers/cpufreq/cpufreq_governor.c     | 140 +++++++++++++---------
 drivers/cpufreq/cpufreq_governor.h     |  42 ++++---
 drivers/cpufreq/cpufreq_ondemand.c     | 205 +++++++++++++++++++--------------
 drivers/cpufreq/cpufreq_stats.c        |  18 +--
 drivers/cpufreq/cpufreq_userspace.c    |   2 -
 drivers/cpufreq/freq_table.c           |   6 -
 include/linux/cpufreq.h                |  26 +----
 9 files changed, 346 insertions(+), 276 deletions(-)

1.7.12.rc2.18.g61b472e

policy->cpu or cpus in policy->cpus can't be offline anymore. And so we don't need to check if they are online or not.

---
 drivers/cpufreq/cpufreq.c           | 17 +++--------------
 drivers/cpufreq/cpufreq_governor.c  |  2 +-
 drivers/cpufreq/cpufreq_userspace.c |  2 --
 drivers/cpufreq/freq_table.c        |  6 ------
 4 files changed, 4 insertions(+), 23 deletions(-)

index 9656420..e619f4f 100644
@@ -76,10 +76,6 @@ static int lock_policy_rwsem_##mode					\
 	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
 	BUG_ON(policy_cpu == -1);					\
 	down_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));		\
-	if (unlikely(!cpu_online(cpu))) {				\
-		up_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));	\
-		return -1;						\
-	}								\
 									\
 	return 0;							\
 }

@@ -719,8 +715,6 @@ static int cpufreq_add_dev_symlink(unsigned int cpu,
 
 		if (j == cpu)
 			continue;
-		if (!cpu_online(j))
-			continue;
 
 		pr_debug("CPU %u already managed, adding link\n", j);
 		managed_policy = cpufreq_cpu_get(cpu);

@@ -777,8 +771,6 @@ static int cpufreq_add_dev_interface(unsigned int cpu,
 
 	spin_lock_irqsave(&cpufreq_driver_lock, flags);
 	for_each_cpu(j, policy->cpus) {
-		if (!cpu_online(j))
-			continue;
 		per_cpu(cpufreq_cpu_data, j) = policy;
 		per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
 	}

@@ -1005,11 +997,8 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;
 
-	for_each_cpu(j, policy->cpus) {
-		if (!cpu_online(j))
-			continue;
+	for_each_cpu(j, policy->cpus)
 		per_cpu(cpufreq_policy_cpu, j) = cpu;
-	}
 
 #ifdef CONFIG_CPU_FREQ_TABLE
 	cpufreq_frequency_table_update_policy_cpu(policy);

@@ -1468,7 +1457,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy,
 	if (target_freq == policy->cur)
 		return 0;
 
-	if (cpu_online(policy->cpu) && cpufreq_driver->target)
+	if (cpufreq_driver->target)
 		retval = cpufreq_driver->target(policy, target_freq, relation);
 
 	return retval;

@@ -1506,7 +1495,7 @@ int __cpufreq_driver_getavg(struct cpufreq_policy *policy, unsigned int cpu)
 	if (cpufreq_disabled())
 		return ret;
 
-	if (!(cpu_online(cpu) && cpufreq_driver->getavg))
+	if (!cpufreq_driver->getavg)
 		return 0;
 
 	policy = cpufreq_cpu_get(policy->cpu);

index 79795c4..e4a306c 100644
@@ -225,7 +225,7 @@ int cpufreq_governor_dbs(struct dbs_data *dbs_data,
 
 	switch (event) {
 	case CPUFREQ_GOV_START:
-		if ((!cpu_online(cpu)) || (!policy->cur))
+		if (!policy->cur)
 			return -EINVAL;
 
 		mutex_lock(&dbs_data->mutex);

index c8c3d29..bbeb9c0 100644
@@ -118,8 +118,6 @@ static int cpufreq_governor_userspace(struct cpufreq_policy *policy,
 
 	switch (event) {
 	case CPUFREQ_GOV_START:
-		if (!cpu_online(cpu))
-			return -EINVAL;
 		BUG_ON(!policy->cur);
 		mutex_lock(&userspace_mutex);
 

index aa5bd39..d7a7966 100644
@@ -63,9 +63,6 @@ int cpufreq_frequency_table_verify(struct cpufreq_policy *policy,
 	pr_debug("request for verification of policy (%u - %u kHz) for cpu %u\n",
 					policy->min, policy->max, policy->cpu);
 
-	if (!cpu_online(policy->cpu))
-		return -EINVAL;
-
 	cpufreq_verify_within_limits(policy, policy->cpuinfo.min_freq,
 				     policy->cpuinfo.max_freq);
 

@@ -121,9 +118,6 @@ int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
 		break;
 	}
 
-	if (!cpu_online(policy->cpu))
-		return -EINVAL;
-
 	for (i = 0; (table[i].frequency != CPUFREQ_TABLE_END); i++) {
 		unsigned int freq = table[i].frequency;
 		if (freq == CPUFREQ_ENTRY_INVALID)

1.7.12.rc2.18.g61b472e

Macro "CPUFREQ_STATDEVICE_ATTR" is defined local to cpufreq_stats.c file and is almost a copy of the generic version present in cpufreq.h file. Lets use the generic version instead.

---
 drivers/cpufreq/cpufreq_stats.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

index 572124c..a2dee4c 100644
@@ -24,12 +24,6 @@
 
 static spinlock_t cpufreq_stats_lock;
 
-#define CPUFREQ_STATDEVICE_ATTR(_name, _mode, _show) \
-static struct freq_attr _attr_##_name = {\
-	.attr = {.name = __stringify(_name), .mode = _mode, }, \
-	.show = _show,\
-};
-
 struct cpufreq_stats {
 	unsigned int cpu;
 	unsigned int total_trans;

@@ -136,17 +130,17 @@ static ssize_t show_trans_table(struct cpufreq_policy *policy, char *buf)
 		return PAGE_SIZE;
 	return len;
 }
-CPUFREQ_STATDEVICE_ATTR(trans_table, 0444, show_trans_table);
+cpufreq_freq_attr_ro(trans_table);
 #endif
 
-CPUFREQ_STATDEVICE_ATTR(total_trans, 0444, show_total_trans);
-CPUFREQ_STATDEVICE_ATTR(time_in_state, 0444, show_time_in_state);
+cpufreq_freq_attr_ro(total_trans);
+cpufreq_freq_attr_ro(time_in_state);
 
 static struct attribute *default_attrs[] = {
-	&_attr_total_trans.attr,
-	&_attr_time_in_state.attr,
+	&total_trans.attr,
+	&time_in_state.attr,
 #ifdef CONFIG_CPU_FREQ_STAT_DETAILS
-	&_attr_trans_table.attr,
+	&trans_table.attr,
 #endif
 	NULL
 };

1.7.12.rc2.18.g61b472e

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.

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

index e619f4f..1ae78d4 100644
@@ -1076,6 +1076,7 @@ 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_write(cpu);
 		kobj = &data->kobj;
 		cmp = &data->kobj_unregister;

@@ -1655,7 +1656,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);

@@ -1709,18 +1710,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;

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                        *
  *********************************************************************/
 
-#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	4
 
 struct cpufreq_governor {
 	char	name[CPUFREQ_NAME_LEN];

1.7.12.rc2.18.g61b472e

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 uses the infrastructure provided by earlier patch and implements init/exit routines for ondemand and conservative governors.

---
 drivers/cpufreq/cpufreq.c              |   4 -
 drivers/cpufreq/cpufreq_conservative.c | 142 +++++++++++++----------
 drivers/cpufreq/cpufreq_governor.c     | 138 +++++++++++++---------
 drivers/cpufreq/cpufreq_governor.h     |  42 ++++---
 drivers/cpufreq/cpufreq_ondemand.c     | 205 +++++++++++++++++++--------------
 include/linux/cpufreq.h                |  19 +--
 6 files changed, 314 insertions(+), 236 deletions(-)

index 1ae78d4..7aacfbf 100644
@@ -1551,9 +1551,6 @@ static int __cpufreq_governor(struct cpufreq_policy *policy,
 						policy->cpu, event);
 	ret = policy->governor->governor(policy, event);
 
-	if (!policy->governor->initialized && (event == CPUFREQ_GOV_START))
-		policy->governor->initialized = 1;
-
 	/* we keep one module reference alive for
 			each CPU governed by this CPU */
 	if ((event != CPUFREQ_GOV_START) || ret)

@@ -1577,7 +1574,6 @@ int cpufreq_register_governor(struct cpufreq_governor *governor)
 
 	mutex_lock(&cpufreq_governor_mutex);
 
-	governor->initialized = 0;
 	err = -EBUSY;
 	if (__find_governor(governor->name) == NULL) {
 		err = 0;

index 3b822ce..5dae7e6 100644
@@ -183,11 +183,10 @@ static inline unsigned long cpufreq_scale(unsigned long old, u_int div, u_int mu
 #define CPUFREQ_GOV_STOP	2
 #define CPUFREQ_GOV_LIMITS	3
 #define CPUFREQ_GOV_POLICY_INIT	4
-#define CPUFREQ_GOV_POLICY_EXIT	4
+#define CPUFREQ_GOV_POLICY_EXIT	5
 
 struct cpufreq_governor {
 	char	name[CPUFREQ_NAME_LEN];
-	int	initialized;
 	int	(*governor)	(struct cpufreq_policy *policy,
 				 unsigned int event);
 	ssize_t	(*show_setspeed)	(struct cpufreq_policy *policy,

@@ -307,22 +306,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)		\
-static struct global_attr _name =		\
-__ATTR(_name, 0444, show_##_name, NULL)
-
-#define define_one_global_rw(_name)		\
-static struct global_attr _name =		\
-__ATTR(_name, 0644, show_##_name, store_##_name)
-
 struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu);
 void cpufreq_cpu_put(struct cpufreq_policy *data);
 const char *cpufreq_get_current_driver(void);

1.7.12.rc2.18.g61b472e

Hi,

[...]

Thanks for pointing out. Would be fixed in next version.

Adding following to the original patch:

b/drivers/cpufreq/cpufreq_conservative.c
index 6b13f9f..5d03577 100644

@@ -329,6 +329,11 @@ static int cs_init(struct dbs_data *dbs_data)
        return 0;
 }
 
+static void cs_exit(struct dbs_data *dbs_data)
+{
+       kfree(dbs_data->tuners);
+}
+
 define_get_cpu_dbs_routines(cs_cpu_dbs_info);
 
 static struct notifier_block cs_cpufreq_notifier_block = {

@@ -348,6 +353,7 @@ static struct common_dbs_data cs_dbs_cdata = {
        .gov_check_cpu = cs_check_cpu,
        .gov_ops = &cs_ops,
        .init = cs_init,
+       .exit = cs_exit,
 };
 
 static int cs_cpufreq_governor_dbs(struct cpufreq_policy *policy,

b/drivers/cpufreq/cpufreq_governor.c
index 925f5b3..7722505 100644

@@ -255,6 +255,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                                        latency * LATENCY_MULTIPLIER));
                return 0;
        case CPUFREQ_GOV_POLICY_EXIT:
+               cdata->exit(dbs_data);
                kfree(dbs_data);
                policy->governor_data = NULL;
                return 0;

b/drivers/cpufreq/cpufreq_governor.h
index 8bd8df6..6301790 100644

@@ -139,6 +139,7 @@ struct common_dbs_data {
        void (*gov_dbs_timer)(struct work_struct *work);
        void (*gov_check_cpu)(int cpu, unsigned int load);
        int (*init)(struct dbs_data *dbs_data);
+       void (*exit)(struct dbs_data *dbs_data);
 
        /* Governor specific ops, see below */
        void *gov_ops;

b/drivers/cpufreq/cpufreq_ondemand.c
index 5eda540..f2539e0 100644

@@ -525,6 +525,11 @@ static int od_init(struct dbs_data *dbs_data)
        return 0;
 }
 
+static void od_exit(struct dbs_data *dbs_data)
+{
+       kfree(dbs_data->tuners);
+}
+
 define_get_cpu_dbs_routines(od_cpu_dbs_info);
 
 static struct od_ops od_ops = {

@@ -542,6 +547,7 @@ static struct common_dbs_data od_dbs_cdata = {
        .gov_check_cpu = od_check_cpu,
        .gov_ops = &od_ops,
        .init = od_init,
+       .exit = od_exit,
 };
 
 static int od_cpufreq_governor_dbs(struct cpufreq_policy *policy,

Well, [1-2/4] are things I can take for v3.9 no problem. The other two I'd wait for the next cycle to be honest. We already have 30+ cpufreq patches scheduled for v3.9 and some of them quite subtle for that matter.

Thanks,
Rafael

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

To be honest, i wanted to get these in 3.9 :) .
And that's why hurried on them and got this full series working in a single day of work :)

Anyway, i understand your point and i also believe last two patches require some testing/review before these getting in.

One important point i would like to highlight is:
governors directory would be present in cpu/cpu*/cpufreq/ now instead of cpu/cpufreq/.

Uh, hold on, isn't this breaking a bunch of userspace with this move? Also, on all those other systems which don't need per-policy governors, we probably don't need this. So maybe this should be made optional, to be enabled by a config option IMO...

Thanks.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

That's why i am highlighting it again and again. :)
What i believe is, the place where this directory was present earlier (cpu/cpufreq/) wasn't the right place. Everything else was in cpu/cpu*/cpufreq, then why this in cpu/cpufreq/ ?

I don't know how much of a pain it would be to fix userspace for it, but i know it wouldn't be that small.

I had another idea of doing this only for platforms where we have multiple struct policy alive at the same time. But didn't wanted to implement it before discussing this further.

Ah, see, someone caught up with it :).

For the simple reason that the "cpu*" stuff is per-cpu - the "cpu/cpufreq" is per system, i.e. one governor for the whole system.

I wouldn't fix userspace but simply not touch it. You can add your per-policy stuff in "cpu/cpu*" as new sysfs nodes and no need to change anything. And, also, as I suggested earlier, you should make it configurable since this code wouldn't make sense on x86, for example, where one system-wide governor should suffice.

Simply put it behind a config option like CONFIG_CPU_IDLE_MULTIPLE_DRIVERS, call the whole menu "Multi-power-domain-policy" something and that should be modulary enough.

Thanks.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

That's not completely true. There lies cpufreq directory in cpu/cpu*/ too, where we have per policy stuff in cpu/cpu*/, like policy tunables and stats. And the same is true for governor too.

That was slightly confusing to me :(
The whole governor directory is per policy, i have to keep that in cpu/cpu*/cpufreq
instead of cpu/cpufreq .

Its not only for multicluster system, but a system where multiple cpus have separate clock control and hence multiple policy structures.

Problem with this is it would fail for single image solutions on which everybody is working on. So, with multiple platforms compiled into a single image, this wouldn't
work.

$ tree /sys/devices/system/cpu/cpu0/cpufreq/
/sys/devices/system/cpu/cpu0/cpufreq/
├── affected_cpus
├── bios_limit
├── cpb
├── cpuinfo_cur_freq
├── cpuinfo_max_freq
├── cpuinfo_min_freq
├── cpuinfo_transition_latency
├── related_cpus
├── scaling_available_frequencies
├── scaling_available_governors
├── scaling_cur_freq
├── scaling_driver
├── scaling_governor
├── scaling_max_freq
├── scaling_min_freq
├── scaling_setspeed
└── stats
    ├── time_in_state
    ├── total_trans
    └── trans_table

1 directory, 19 files

$ grep -r . /sys/devices/system/cpu/cpu0/cpufreq/*
/sys/devices/system/cpu/cpu0/cpufreq/affected_cpus:0
/sys/devices/system/cpu/cpu0/cpufreq/bios_limit:4000000
/sys/devices/system/cpu/cpu0/cpufreq/cpb:1
/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_cur_freq:1400000
/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq:4000000
/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_min_freq:1400000
/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_transition_latency:4000
/sys/devices/system/cpu/cpu0/cpufreq/related_cpus:0 1
/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_frequencies:4000000 3400000 2800000 2100000 1400000
/sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors:powersave userspace conservative ondemand perform
/sys/devices/system/cpu/cpu0/cpufreq/scaling_cur_freq:1400000
/sys/devices/system/cpu/cpu0/cpufreq/scaling_driver:acpi-cpufreq
/sys/devices/system/cpu/cpu0/cpufreq/scaling_governor:ondemand
/sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq:4000000
/sys/devices/system/cpu/cpu0/cpufreq/scaling_min_freq:1400000
/sys/devices/system/cpu/cpu0/cpufreq/scaling_setspeed:<unsupported>
/sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state:4000000 3089328
/sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state:3400000 47448
/sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state:2800000 67185
/sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state:2100000 92731
/sys/devices/system/cpu/cpu0/cpufreq/stats/time_in_state:1400000 11416914
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:   From  :    To
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:         :   4000000   3400000   2800000   2100000   140000
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:  4000000:         0     34756     46388     53179    21824
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:  3400000:     12938         0      3755      3555     1450
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:  2800000:     19940         0         0      4547     2565
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:  2100000:     18523         0         0         0     4275
/sys/devices/system/cpu/cpu0/cpufreq/stats/trans_table:  1400000:    301168         0         0         0
/sys/devices/system/cpu/cpu0/cpufreq/stats/total_trans:799918

Show me the policy tunables here.

So make a /sys/devices/system/cpu/cpufreq/policies/ and add functionality to assign cpus to policies or whatever the design of this thing will be.

What are those systems? Examples?

Single-image solutions will enable that config option and get built with it - no problem at all.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

All files which are directly present in cpu/cpu*/cpufreq/ folder. I am not talking about governor tunables but policy tunables. Things like scaling_[min]max_freq are policy tunables.

Policies don't have a name associated with them and so cpu/cpufreq/policies doesn't make any sense. Rather one policy is related to multiple cpus and its tunables are linked in all the cpus that belong to it, like scaling_[min]max_freq.

Don't have examples of these, but there can be few. Over that it is a must for multicluster systems as clusters normally have separate clock control.

But then we will get governors tunables in cpu/cpu*/cpufreq/ instead of cpu/cpufreq/ . Will that not break userspace for other systems?

No, on x86 those are the P-states frequencies. They're defined by the hardware.

Then do the following:

cpu/cpufreq/policies/
|-> policy0

    |-> min_freq
    |-> max_freq
    |-> affected_cpus
    ...

or whatever needs to be a flexible interface for multi-policy cpufreq support.

Remember: once you do those, they're more or less cast in stone so take your time and do the design right, do not hurry those.

Yeah, nice try. We only support real hardware in the kernel, not what could there be.

What's wrong with having both? The cpu/cpufreq/ governor will set the system-wide governor and the cpu/cpu*/cpufreq/ will add the different policies.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

A question we need to answer: What is "policy"?

For me, "policy" is the hardware policy for a group of cpus, defined by the hardware. We call them P-states. But these are strictly per policy (i.e. per cpu group sharing clock line).

So, if we have two packages with two cpus each, we will have two copies of these P-states and every other file/directory in cpu/cpu*/cpufreq. One common copy would be shared between cpu/cpu[0-1]/cpufreq directory and other one between cpu/cpu[2-3]/cpufreq.

We correlate things with cpus rather than policies and so the current directory structure of cpu/cpu*/cpufreq/*** is the best suited ones.

The multi policy part is already well implemented, we are talking about governor per policy here.

Yes, and that's why cpu/cpu*/cpufreq/ondemand/*** suits the best, with exactly the same logic that went for P-states or cpufreq-stats.

Hmm.. confused..
Consider two systems:
- A dual core system, with cores sharing clocks.
- A dual cluster system (dual core per cluster), with separate clocks per cluster.

Where will you keep governor directories for both of these configurations?

We need to select only one... cpu/cpufreq doesn't suit the second case at all as we need to use ondemand governor for both the clusters but with separate tunables. And so a single cpu/cpufreq/ondemand directory wouldn't solve the issue.

--
viresh

Ok, show me the details of that layout. How is that going to look?

One thing I've come to realize with the current interface is that if you want to change stuff, you need to iterate over all cpus instead of writing to a system-wide node.

And, in this case, if you can and need to change the policy per clock-domain, I wouldn't make it needlessly too-granulary per-cpu.

That's why I'm advocating the cpu/cpufreq/ path.

See above.

Easy: as said above, make the policy granularity per clock-domain. On systems which have only one set of P-states - like it is the case with the overwhelming majority of systems running linux now - nothing should change.

Think of it this way: what is the highest granularity you need per clock-domain? If you want to control the policy per clock-domain, then cpu/cpufreq/ is what you want. If you want finer-grained control - and you need to think hard of what use cases are sensible for that finer-grained solution - then you're better off with cpu/cpu*/ layout.

In both cases though, having clear examples of why you've come up with the layout you're advocating would help reviewers a lot. If you simply come and say we need this because there might be systems out there who could use it, then that probably is not going to get you that far.

HTH.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

I don't have board right now to take the snapshot, but it would be like:

$ tree /sys/devices/system/cpu/cpu0/cpufreq/
/sys/devices/system/cpu/cpu0/cpufreq/
├── affected_cpus
├── bios_limit
├── cpb
├── cpuinfo_cur_freq
├── cpuinfo_max_freq
├── cpuinfo_min_freq
├── cpuinfo_transition_latency
├── related_cpus
├── scaling_available_frequencies
├── scaling_available_governors
├── scaling_cur_freq
├── scaling_driver
├── scaling_governor
├── scaling_max_freq
├── scaling_min_freq
├── scaling_setspeed
└── stats
    ├── time_in_state
    ├── total_trans
    └── trans_table
└── ondemand
    ├── sampling_rate
    ├── up_threshold
    └── ignore_nice
etc..

Not really. Following is the way by which cpu/cpu*/cpufreq directories are created:

For policy->cpu:

	ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq,
				   &dev->kobj, "cpufreq");

This creates cpufreq directory for policy in policy->cpu...

For all other cpus in policy->cpus, we do:

		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
					"cpufreq");

And so whatever gets added in cpu/cpu0/cpufreq directory is reflected in all other policy->cpus.

Its already like this, i.e. per policy or clock-domain. Other cpus just have a link. And that's why in my code, i just add governor directory in policy->cpu's cpufreq directory and it gets reflected in other cpus of policy->cpus.

That's why i said P-states as policy tunables.

Currently its not per policy, but single instance of any governor is supported. And it is present in cpu/cpufreq . That's why i said earlier, it isn't the right place for governor's directory. It is very much related to a policy or clock-domain.

I want to control it over clock-domain, but can't get that in cpu/cpufreq/. Policies don't have numbers assigned to them.

So, i am working on ARM's big.LITTLE system where we have two clusters. One of A15s and other of A7s. Because of their different power ratings or performance figures, we need to have separate set of ondemand tunables for them. And hence this patch. Though this patch is required for any multi-cluster system.

--
viresh

So this is adding the current governor as a per-cpu thing.

That's not what I meant - I meant from userspace:

for $i in $(grep processor /proc/cpuinfo | awk '{ print $3 }'); do

	echo "performance" > /sys/devices/system/cpu/cpu$i/cpufreq/scaling_governor;

done

Instead of

echo "performance" > /sys/devices/system/cpu/cpufreq/scaling_governor

which is hypothetical but sets it for the whole system without fuss.

[ … ]

So, give them names.

So you want this (values after "="):

cpu/cpufreq/
|-> policy0

    |-> name		= A15
    |-> min_freq	= ...
    |-> max_freq	= ...
    |-> affected_cpus	= 0,1,2,...
    |-> ondemand
        |-> sampling_rate
	|-> up_threshold
	|-> ignore_nice
    ...
|-> policy1
    |-> name		= A7
    |-> min_freq	= ...
    |-> max_freq	= ...
    |-> affected_cpus	= n,n+1,n+2,...
    |-> performance
        |-> sampling_rate
	|-> up_threshold
	|-> ignore_nice
    ...

Other arches create other policies and that's it. If you need another policy added to the set, you simply add 'policyN++' and that's it.

I think this is cleaner but whatever - I don't care that much. My only strong concern is that this thing should be a Kconfig option and optional for arches where it doesn't apply.

Thanks.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Its per policy, but yes it is replicated into all cpus as all policy->cpus share the same directory.

We actually need to do this only for policy->cpu, but yes the user might not be well aware of policy cpu-groups and may do what you wrote.

But that is true even today, when we need to change any policy tunables or P-states.

IMHO, names doesn't suit policies.

We may have two clusters of A7's also, in a non-big little arch. Then these names become very confusing.

For me, adding policy->names per arch is increasing complexity without much gain. We already have an existing infrastructure where this info is present inside cpus and that looks good to me.

Your concern is: we don't want to fix userspace for existing platforms where we have just a single cluster and so struct policy in the system.

so, a good solution instead of Kconfig stuff is, add another field in policy structure that would be filled by platform specific cpufreq drivers init() routine. Based on which we can decide to put governor directory in cpu/cpufreq/gov-name or cpu/cpu*/cpufreq/gov-name.

But i am not sure if keeping both kind of directory locations for separate platforms is a good idea. Userspace needs to adapt to these changes as well for multi-cluster platforms.

@Rafael: you know about any other multi-cluster/multi-clock-domain platform leaving big.LITTLE, where we have multiple structs policy?

No, as I said so many times already and you're unwilling to understand
it: multiple policies support in cpufreq should be optional and selectable in Kconfig so that systems which don't need that, don't have to see or use it. It is yet another feature which doesn't apply universally so we make such features optional. Like the rest of the gazillion things in the kernel already.

The existing sysfs layout cannot be changed because you're breaking userspace and we don't do that. It is that simple.

Concerning adding new sysfs entries, I told you to make it as easy as possible and as sensible as possible, dictated by the use cases. If you can't come up with some, then talk to the people who are going to use your design and ask them what makes sense the most.

*Then* write the code.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

I am willing to, but not able to :)

I understand what Kconfig options are for, but i am not able to understand what's the benefit of this option here. For example: for single image solutions we need to keep it enabled. And so, would need some sort of logic in cpufreq core & platform driver to decide where to create the governors directory.

The code without Kconfig option would be as simple as:

platform_driver:
init(struct cpufreq_policy *policy)
{
..

    policy->have_multiple_policies = true;

..
}

cpufreq-core:

add_dev()

{
    if (policy->have_multiple_policies)
        create-folder-in-cpu/cpu*/cpufreq;
    else
        create-folder-in-cpu/cpufreq;
}

And so, platforms like Krait or big.LITTLE can set it to true from their cpufreq-drivers. And this wouldn't break any of the current platforms.

That's fine. I understood it already. :)

The problem i see is:
- both governor tunables, cpufreq-stats & policy tunables (P-states) have the same requirement. They are all per policy or clock-domain, instead of per cpu.
- I want to keep all of these at the same place, as they should be present in the
same hierarchy.
- If we move everything to cpu/cpufreq/policy-names/ then also we would break existing userspace stuff for stats and P-states.
- If we move everything to cpu/cpu*/cpufreq/ then also we would break existing userspace stuff for governors.

For me cpu/cpu*/ is the most sensible as it is an very easy/convenient interface for users. I am the first one who is going to use it :)

@Rafael: What's your view on this discussion we are having? We probably need few more "minds" to jump in, as we are not moving towards a conclusion. :)

--
viresh

Are you kidding me? You're simply not reading what I'm saying to you:
"... should be optional and selectable in Kconfig so that systems which don't need that, don't have to see or use it." Because on those systems it doesn't apply.

How about we add an x86-specific extension which is a big wad of code and is needlessly run on ARM just because it is easier?

That's why we do config options, so that code which doesn't apply on a specific system, doesn't run on it.

Goddammit, how hard is to understand that??!

So keep it enabled!

Yes, this is how it could be done.

And this is what I mean by making it optional:

You go and abstract away the "create-folder-in-cpu/cpu*/cpufreq" functionality. If this is a function called add_additional_sysfs_entries(), for example, it should do nothing when CONFIG_CPUFREQ_MULTIPLE_POLICIES is disabled. Otherwise, it will do your dance:

#ifdef CONFIG_CPUFREQ_MULTIPLE_POLICIES
static int add_additional_sysfs_entries(...)
{

do all stuff required for multiple policies

}
#else /* CONFIG_CPUFREQ_MULTIPLE_POLICIES */
static int add_additional_sysfs_entries(...)

{
	return 0;
}

#endif /* CONFIG_CPUFREQ_MULTIPLE_POLICIES */

and all the rest of the stuff which is needed for multiple system policies, should be abstracted that way, more functions added, whatever. If it is starting to become more, you can create your own compilation unit. And so on and so on... the kernel is full of examples how to do stuff like that.

Not really, you obviously didn't.

No, you're not allowed to change existing sysfs layout. FULLSTOP.

Simply add the new stuff to cpu/cpu*/cpufreq/ with code which is enabled when CONFIG_CPUFREQ_MULTIPLE_POLICIES is set. If CONFIG_CPUFREQ_MULTIPLE_POLICIES is not enabled, nothing changes.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

There isn't lot of code that we have to keep inside the macro you suggest. Its just an if else (with single line block), which would give the parent kobject. Nothing else.

I didn't wanted to create a macro for just that. For me an if/else is not that big code.

Anyway, if nobody else comes on my side i can create that macro for you. But, personally i would prefer code without such macros.

--
viresh

Yeah, I imagine for you it isn't, no.

Here's an even cleaner way:

platform_driver:
init(struct cpufreq_policy *policy)

{
	...

	add_additional_sysfs_entries(policy);

	...
}

...

static void add_additional_sysfs_entries(struct cpufreq_policy *policy) {

#ifdef CONFIG_CPUFREQ_MULTIPLE_POLICIES
	create-folder-in-cpu/cpu*/cpufreq;
	...

#endif
}

and the platform driver will have in its Kconfig section:

config CPUFREQ_PLATFORM_DRIVER_X

	...
	select CPUFREQ_MULTIPLE_POLICIES

You don't need the policy->have_multiple_policies member even.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Tricky part is the name of this routine: add_additional_sysfs_entries().

I am not proposing additional set of directories inside cpu/cpu*/cpufreq/ but either of cpu/cpufreq/ or cpu/cpu*/cpufreq/ for governor.

And so, keeping that additional variable looks a better solution. Let me get a patch with this change only and see how it looks.

Now you're just being silly - this is just an example how to do it. If you want me to do it for ya, you need to send me your monthly salary.

Yeah, I don't think you understand me at all. :(

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Haha... I don't want you to do it. I don't want such routine to be exposed to cpufreq drivers as this belongs to the core code.

Just some kind of indication from platform driver is required about how/where it wants its governor directory to be present. And the variable suits more here.

Lets discuss it more on the next patch i send :)

--
viresh

The indication is this:

config CPUFREQ_PLATFORM_DRIVER_X

        ...
        select CPUFREQ_MULTIPLE_POLICIES

You really need to slow down and really look at what I'm proposing.

And this variable means nothing on other systems so why add it in the first place?

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

This indication isn't enough. On a single image solution, we need to identify the system which needs support for multiple policies and i still feel we need that variable type indication :)

On Tue, Feb 05, 2013 at 05:54:57PM +0530, Viresh Kumar wrote:q

If the image is going to run also on systems which support only a single policy, then I guess you can make it a bool, stuff it in struct cpufreq_policy and ifdef around it.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

That's what i proposed initially.

Qualcomm's ARM based "krait". Currently shipping in millions of Android phones.

http://en.wikipedia.org/wiki/Krait_(CPU)

Thanks Charles for pointing it out, I knew there is one :)

--
viresh

Actually shooting myself in the foot here, Krait is not such a great example because although you can use difference between frequencies you are less likely to use different tunables (not inconceivable but unlikely). The best examples systems are multi cluster and hereterogeneous systems, like the recently announced Samsung Exynos 5 octa http://en.wikipedia.org/wiki/Exynos_(system_on_chip). We will see more systems like this appearing, sporting low power cores combined with high performance ones, all running at the same time. I appreciate this is all very new, but more will come, and the requirement to have different tunables per cluster is very real. In ARM on our own multi cluster test chip, using an experimental version of this approach, we have seen good improvements in power consumpti
on without compromising performance.

(Apologies ahead for any bit my mail server appends, not much I can do about it)

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Ok, thanks for giving this insight, this is useful.

Question: do you need the granularity of that control to be per cpu (with that I mean what linux understands under "cpu," i.e. logical or physical core) or does one governor suffice per a set of cores, or as you call it, a cluster?

Yeah, my condolences :-)

Leaving it in, in case you haven't seen how it looks like :-)

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Later. Whatever you'd like to call it, but essentially a set of cpus, as linux understands them, that are logically related by the fact that you'd like to be able to use the same tuning policy and same governor across all of them.

Cheers

Charles

(apologies again for annoying addendums to follow)

-- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Right, policy will be applied to the whole set, yes, but can you imagine that per-core settings could also make sense at some point?

Thanks.

Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

After a long & hot discussion over the changes made by this patchset, following patches came out:

--------------x------------------x-------------------

commit 15b5548c9ccfb8088270f7574710d9d67edfe33b

Author: Viresh Kumar <[email protected]>
Date:   Tue Feb 5 21:29:05 2013 +0530

cpufreq: Make governors directory sysfs location based on
have_multiple_policies

Until now directory for governors tunables was getting created in cpu/cpufreq/<gov-name>. With the introduction of following patch:
"cpufreq: governor: Implement per policy instances of governors"

this directory would be created in
cpu/cpu<num>/cpufreq/<gov-name>. This might
break userspace of existing platforms. Lets do this change only
for platforms
which need support for multiple policies and thus above mentioned patch.

From now on, such platforms would be require to do following from
their init()
routines:

policy->have_multiple_policies = true;

---
 drivers/cpufreq/cpufreq_governor.c |  2 +-
 include/linux/cpufreq.h            | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

b/drivers/cpufreq/cpufreq_governor.c
index 545ae24..41ee86f 100644

@@ -300,7 +300,7 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                                             dbs_data->cdata->gov_dbs_timer);
                }
 
-               rc = sysfs_create_group(&policy->kobj,
+               rc = sysfs_create_group(get_governor_parent_kobj(policy),
                                dbs_data->cdata->attr_group);
                if (rc) {
                        mutex_unlock(&dbs_data->mutex);

index 5dae7e6..6e1abd2 100644
@@ -107,6 +107,11 @@ struct cpufreq_policy {
        unsigned int            policy; /* see above */
        struct cpufreq_governor *governor; /* see below */
        void                    *governor_data;
+       /* This should be set by init() of 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 */
+       bool                    have_multiple_policies;
 
        struct work_struct      update; /* if update_policy() needs to be
                                         * called, but you're in IRQ context */

@@ -134,6 +139,15 @@ static inline bool policy_is_shared(struct

cpufreq_policy *policy)

        return cpumask_weight(policy->cpus) > 1;

}

+static inline struct kobject * +get_governor_parent_kobj(struct cpufreq_policy *policy)
+{
+ if (policy->have_multiple_policies)
+ return &policy->kobj;
+ else
+ return cpufreq_global_kobject;
+}
+
/******************** cpufreq transition notifiers *******************/

#define CPUFREQ_PRECHANGE (0)

--------------x------------------x-------------------

Plus the following patch, though i am still not in favor of this patch. @Rafael: Please share your opinion too on this one. :)

--------------x------------------x------------------- commit 1c7e9871fce7388136eda1c86ca75a520e4d3b9d

Author: Viresh Kumar <[email protected]>
Date:   Tue Feb 5 21:41:40 2013 +0530

cpufreq: Add Kconfig option to enable/disable have_multiple_policies

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.

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

index cbcb21e..e6e6939 100644
@@ -23,6 +23,9 @@ config CPU_FREQ_TABLE
 config CPU_FREQ_GOV_COMMON
        bool
 
+config CPU_FREQ_HAVE_MULTIPLE_POLICIES
+       bool
+
 config CPU_FREQ_STAT
        tristate "CPU frequency translation statistics"
        select CPU_FREQ_TABLE

index 6e1abd2..a092fcb 100644
@@ -107,11 +107,13 @@ struct cpufreq_policy {
        unsigned int            policy; /* see above */
        struct cpufreq_governor *governor; /* see below */
        void                    *governor_data;
+#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES
        /* This should be set by init() of 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 */
        bool                    have_multiple_policies;
+#endif
 
        struct work_struct      update; /* if update_policy() needs to be
                                         * called, but you're in IRQ context */

@@ -142,9 +144,11 @@ static inline bool policy_is_shared(struct

cpufreq_policy *policy)
static inline struct kobject * get_governor_parent_kobj(struct cpufreq_policy *policy) {
+#ifdef CONFIG_CPU_FREQ_HAVE_MULTIPLE_POLICIES

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

}

--------------x------------------x-------------------

Hi Rafael,

Because this patch was quite big (317 insertions(+), 238 deletions(-)), i was planning a detailed self review to capture any mistakes and luckily i found one for above patch :)

b/drivers/cpufreq/cpufreq_governor.c
index 41ee86f..fe037c0 100644

@@ -342,7 +342,8 @@ int cpufreq_governor_dbs(struct cpufreq_policy *policy,
                mutex_lock(&dbs_data->mutex);
                mutex_destroy(&cpu_cdbs->timer_mutex);
 
-               sysfs_remove_group(&policy->kobj, dbs_data->cdata->attr_group);
+               sysfs_remove_group(get_governor_parent_kobj(policy),
+                               dbs_data->cdata->attr_group);
                if (dbs_data->cdata->governor == GOV_CONSERVATIVE)
                        cpufreq_unregister_notifier(cs_ops->notifier_block,
                                        CPUFREQ_TRANSITION_NOTIFIER);

I have pushed the complete patchset here:

http://git.linaro.org/gitweb?p=people/vireshk/linux.git;a=shortlog;h=refs/heads/cpufreq-updates

Viresh, perhaps you should ask Stephen Rothwell to pull in your tree to get some more testing before Rafael pulls it in for 3.10?

Its has been made clear by Rafael that these patches wouldn't make it for 3.9 (though i wanted them to :) ), and so once the merge window is over Rafael might pull them in and so they would reach Stephen's linux-next too...

I am not sure if sending a cpufreq pull request directly to Stephen is preferred.
@Rafael: ??

You may do that, if you want, but that's slightly confusing.

Also the policy is that material which is not going to be included into v3.9 shouldn't be in linux-next before v3.9-rc1.

Moreover, for build testing it is sufficient to put it into a branch somewhere at git.kernel.org (as you have already noticed :-)).

Thanks,
Rafael

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