[04/18] cpufreq: Manage fallback policies in a list

Message ID 43d728016b775d1b0fc02c981eb0520ac08297f5.1422346933.git.viresh.kumar@linaro.org
State New
Headers show

Commit Message

Viresh Kumar Jan. 27, 2015, 8:36 a.m.
Policies manage a group of CPUs and tracking them on per-cpu basis isn't the
best approach for sure.

The obvious loss is the amount of memory consumed for keeping a per-cpu copy of
the same pointer. But the bigger problem is managing such a data structure as we
need to update it for all policy->cpus.

To make it simple, lets manage fallback CPUs in a list rather than a per-cpu
variable.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 51 +++++++++++++++++++++++++++--------------------
 include/linux/cpufreq.h   |  5 ++++-
 2 files changed, 33 insertions(+), 23 deletions(-)

Comments

Viresh Kumar Feb. 3, 2015, 4:10 a.m. | #1
On 3 February 2015 at 06:11, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> This becomes quite inefficient on a system with many CPUs having different
> policies.  My approach would be to somehow attach the fallback policy information
> to the CPU device object.

It will be same as the per-cpu approach which we are fed up of.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 4, 2015, 6:18 a.m. | #2
On 3 February 2015 at 20:34, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> No, it won't be the same.  The per-CPU memory is special.

> The general idea of the existing code is sane in my view.  It connects the
> fallback policy information with the given CPU directly, which generally is
> a good idea.  Storing that information in the per-CPU memory isn't a good
> idea, however.

Okay, will see what can be done.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Feb. 4, 2015, 6:20 a.m. | #3
On 4 February 2015 at 03:58, Saravana Kannan <skannan@codeaurora.org> wrote:
> Can you explain why we need a fallback list in the first place? Now that we
> are not destroying and creating policy objects, I don't see any point in the
> fallback list.

Because we wanted to mark the policy inactive. But as I have introduced another
field for that now, probably it can be fixed. Will check again on what
can be done.

Can you review the other patches so that they are reviewed once before sending
V2 here ?
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index cf8ce523074f..f253cf45f910 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -37,6 +37,11 @@  static LIST_HEAD(cpufreq_policy_list);
 #define for_each_policy(__policy)				\
 	list_for_each_entry(__policy, &cpufreq_policy_list, policy_list)
 
+/* Iterate over offline CPUs policies */
+static LIST_HEAD(cpufreq_fallback_list);
+#define for_each_fallback_policy(__policy)			\
+	list_for_each_entry(__policy, &cpufreq_fallback_list, fallback_list)
+
 /* Iterate over governors */
 static LIST_HEAD(cpufreq_governor_list);
 #define for_each_governor(__governor)				\
@@ -49,7 +54,6 @@  static LIST_HEAD(cpufreq_governor_list);
  */
 static struct cpufreq_driver *cpufreq_driver;
 static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
-static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data_fallback);
 static DEFINE_RWLOCK(cpufreq_driver_lock);
 DEFINE_MUTEX(cpufreq_governor_lock);
 
@@ -999,13 +1003,16 @@  static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 
 static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 {
-	struct cpufreq_policy *policy;
+	struct cpufreq_policy *policy = NULL, *temp;
 	unsigned long flags;
 
 	read_lock_irqsave(&cpufreq_driver_lock, flags);
-
-	policy = per_cpu(cpufreq_cpu_data_fallback, cpu);
-
+	for_each_fallback_policy(temp) {
+		if (cpumask_test_cpu(cpu, temp->related_cpus)) {
+			policy = temp;
+			break;
+		}
+	}
 	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	if (policy)
@@ -1029,6 +1036,7 @@  static struct cpufreq_policy *cpufreq_policy_alloc(void)
 		goto err_free_cpumask;
 
 	INIT_LIST_HEAD(&policy->policy_list);
+	INIT_LIST_HEAD(&policy->fallback_list);
 	init_rwsem(&policy->rwsem);
 	spin_lock_init(&policy->transition_lock);
 	init_waitqueue_head(&policy->transition_wait);
@@ -1142,6 +1150,11 @@  static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 		policy = cpufreq_policy_alloc();
 		if (!policy)
 			goto nomem_out;
+	} else {
+		/* Don't need fallback list anymore */
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
+		list_del(&policy->fallback_list);
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 	}
 
 	/*
@@ -1296,11 +1309,8 @@  static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	if (cpufreq_driver->exit)
 		cpufreq_driver->exit(policy);
 err_set_policy_cpu:
-	if (recover_policy) {
-		/* Do not leave stale fallback data behind. */
-		per_cpu(cpufreq_cpu_data_fallback, cpu) = NULL;
+	if (recover_policy)
 		cpufreq_policy_put_kobj(policy);
-	}
 	cpufreq_policy_free(policy);
 
 nomem_out:
@@ -1333,21 +1343,22 @@  static int __cpufreq_remove_dev_prepare(struct device *dev,
 
 	pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
 
-	write_lock_irqsave(&cpufreq_driver_lock, flags);
-
 	policy = per_cpu(cpufreq_cpu_data, cpu);
-
-	/* Save the policy somewhere when doing a light-weight tear-down */
-	if (cpufreq_suspended)
-		per_cpu(cpufreq_cpu_data_fallback, cpu) = policy;
-
-	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
-
 	if (!policy) {
 		pr_debug("%s: No cpu_data found\n", __func__);
 		return -EINVAL;
 	}
 
+	down_read(&policy->rwsem);
+	cpus = cpumask_weight(policy->cpus);
+	up_read(&policy->rwsem);
+
+	/* Save the policy when doing a light-weight tear-down of last cpu */
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+	if (cpufreq_suspended && cpus == 1)
+		list_add(&policy->fallback_list, &cpufreq_fallback_list);
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 	if (has_target()) {
 		ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
 		if (ret) {
@@ -1359,10 +1370,6 @@  static int __cpufreq_remove_dev_prepare(struct device *dev,
 			policy->governor->name, CPUFREQ_NAME_LEN);
 	}
 
-	down_read(&policy->rwsem);
-	cpus = cpumask_weight(policy->cpus);
-	up_read(&policy->rwsem);
-
 	if (cpu != policy->cpu) {
 		sysfs_remove_link(&dev->kobj, "cpufreq");
 	} else if (cpus > 1) {
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 2ee4888c1f47..df6af4cfa26a 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -87,7 +87,10 @@  struct cpufreq_policy {
 	struct cpufreq_real_policy	user_policy;
 	struct cpufreq_frequency_table	*freq_table;
 
-	struct list_head        policy_list;
+	/* Internal lists */
+	struct list_head        policy_list;	/* policies of online CPUs */
+	struct list_head	fallback_list;	/* policies of offline CPUs */
+
 	struct kobject		kobj;
 	struct completion	kobj_unregister;