From df24014abe3694e7c34ce5e50248611b7a93fe83 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Mon, 29 Apr 2019 15:03:58 +0530 Subject: cpufreq: Call transition notifier only once for each policy Currently, the notifiers are called once for each CPU of the policy->cpus cpumask. It would be more optimal if the notifier can be called only once and all the relevant information be provided to it. Out of the 23 drivers that register for the transition notifiers today, only 4 of them do per-cpu updates and the callback for the rest can be called only once for the policy without any impact. This would also avoid multiple function calls to the notifier callbacks and reduce multiple iterations of notifier core's code (which does locking as well). This patch adds pointer to the cpufreq policy to the struct cpufreq_freqs, so the notifier callback has all the information available to it with a single call. The five drivers which perform per-cpu updates are updated to use the cpufreq policy. The freqs->cpu field is redundant now and is removed. Acked-by: David S. Miller (sparc) Signed-off-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/cpufreq/cpufreq.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) (limited to 'drivers/cpufreq') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 7ea217c88c2e..c66eefe928c5 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -340,11 +340,14 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy, struct cpufreq_freqs *freqs, unsigned int state) { + int cpu; + BUG_ON(irqs_disabled()); if (cpufreq_disabled()) return; + freqs->policy = policy; freqs->flags = cpufreq_driver->flags; pr_debug("notification %u of frequency transition to %u kHz\n", state, freqs->new); @@ -364,10 +367,8 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy, } } - for_each_cpu(freqs->cpu, policy->cpus) { - srcu_notifier_call_chain(&cpufreq_transition_notifier_list, - CPUFREQ_PRECHANGE, freqs); - } + srcu_notifier_call_chain(&cpufreq_transition_notifier_list, + CPUFREQ_PRECHANGE, freqs); adjust_jiffies(CPUFREQ_PRECHANGE, freqs); break; @@ -377,11 +378,11 @@ static void cpufreq_notify_transition(struct cpufreq_policy *policy, pr_debug("FREQ: %u - CPUs: %*pbl\n", freqs->new, cpumask_pr_args(policy->cpus)); - for_each_cpu(freqs->cpu, policy->cpus) { - trace_cpu_frequency(freqs->new, freqs->cpu); - srcu_notifier_call_chain(&cpufreq_transition_notifier_list, - CPUFREQ_POSTCHANGE, freqs); - } + for_each_cpu(cpu, policy->cpus) + trace_cpu_frequency(freqs->new, cpu); + + srcu_notifier_call_chain(&cpufreq_transition_notifier_list, + CPUFREQ_POSTCHANGE, freqs); cpufreq_stats_record_transition(policy, freqs->new); policy->cur = freqs->new; -- cgit v1.2.3 From 2acb9bdae92d0662cd4fa11c481e80b6202b5630 Mon Sep 17 00:00:00 2001 From: "Rafael J. Wysocki" Date: Fri, 10 May 2019 12:35:53 +0200 Subject: cpufreq: Explain the kobject_put() in cpufreq_policy_alloc() It may not be particularly clear why the kobject_put() after failing kobject_init_and_add() in cpufreq_policy_alloc() is not redundant, so add a comment to explain that. Signed-off-by: Rafael J. Wysocki Acked-by: Viresh Kumar --- drivers/cpufreq/cpufreq.c | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'drivers/cpufreq') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index c66eefe928c5..57a3853a55fb 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1134,6 +1134,11 @@ static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu) cpufreq_global_kobject, "policy%u", cpu); if (ret) { pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret); + /* + * The entire policy object will be freed below, but the extra + * memory allocated for the kobject name needs to be freed by + * releasing the kobject. + */ kobject_put(&policy->kobj); goto err_free_real_cpus; } -- cgit v1.2.3 From ab05d97a3785088fa2964219227a1ba95d97d590 Mon Sep 17 00:00:00 2001 From: Yue Hu Date: Mon, 29 Apr 2019 15:24:18 +0800 Subject: cpufreq: Don't find governor for setpolicy drivers in cpufreq_init_policy() In cpufreq_init_policy() we will check if there's last_governor for target and setpolicy type. However last_governor is set only if has_target() is true in cpufreq_offline(). That means find last_governor for setpolicy type is pointless. Also new_policy.governor will not be used if ->setpolicy callback is set in cpufreq_set_policy(). Moreover, there's duplicate ->setpolicy check in using default policy path. Let's add a new helper function to avoid it. Also update comments. Signed-off-by: Yue Hu Acked-by: Viresh Kumar Signed-off-by: Rafael J. Wysocki --- drivers/cpufreq/cpufreq.c | 116 ++++++++++++++++++++++++++-------------------- 1 file changed, 65 insertions(+), 51 deletions(-) (limited to 'drivers/cpufreq') diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 57a3853a55fb..f941efd9b836 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -619,50 +619,52 @@ static struct cpufreq_governor *find_governor(const char *str_governor) return NULL; } +static int cpufreq_parse_policy(char *str_governor, + struct cpufreq_policy *policy) +{ + if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) { + policy->policy = CPUFREQ_POLICY_PERFORMANCE; + return 0; + } + if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) { + policy->policy = CPUFREQ_POLICY_POWERSAVE; + return 0; + } + return -EINVAL; +} + /** - * cpufreq_parse_governor - parse a governor string + * cpufreq_parse_governor - parse a governor string only for !setpolicy */ static int cpufreq_parse_governor(char *str_governor, struct cpufreq_policy *policy) { - if (cpufreq_driver->setpolicy) { - if (!strncasecmp(str_governor, "performance", CPUFREQ_NAME_LEN)) { - policy->policy = CPUFREQ_POLICY_PERFORMANCE; - return 0; - } - - if (!strncasecmp(str_governor, "powersave", CPUFREQ_NAME_LEN)) { - policy->policy = CPUFREQ_POLICY_POWERSAVE; - return 0; - } - } else { - struct cpufreq_governor *t; + struct cpufreq_governor *t; - mutex_lock(&cpufreq_governor_mutex); + mutex_lock(&cpufreq_governor_mutex); - t = find_governor(str_governor); - if (!t) { - int ret; + t = find_governor(str_governor); + if (!t) { + int ret; - mutex_unlock(&cpufreq_governor_mutex); + mutex_unlock(&cpufreq_governor_mutex); - ret = request_module("cpufreq_%s", str_governor); - if (ret) - return -EINVAL; + ret = request_module("cpufreq_%s", str_governor); + if (ret) + return -EINVAL; - mutex_lock(&cpufreq_governor_mutex); + mutex_lock(&cpufreq_governor_mutex); - t = find_governor(str_governor); - } - if (t && !try_module_get(t->owner)) - t = NULL; + t = find_governor(str_governor); + } + if (t && !try_module_get(t->owner)) + t = NULL; - mutex_unlock(&cpufreq_governor_mutex); + mutex_unlock(&cpufreq_governor_mutex); - if (t) { - policy->governor = t; - return 0; - } + if (t) { + policy->governor = t; + return 0; } return -EINVAL; @@ -784,8 +786,13 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, if (ret != 1) return -EINVAL; - if (cpufreq_parse_governor(str_governor, &new_policy)) - return -EINVAL; + if (cpufreq_driver->setpolicy) { + if (cpufreq_parse_policy(str_governor, &new_policy)) + return -EINVAL; + } else { + if (cpufreq_parse_governor(str_governor, &new_policy)) + return -EINVAL; + } ret = cpufreq_set_policy(policy, &new_policy); @@ -1051,32 +1058,39 @@ __weak struct cpufreq_governor *cpufreq_default_governor(void) static int cpufreq_init_policy(struct cpufreq_policy *policy) { - struct cpufreq_governor *gov = NULL; + struct cpufreq_governor *gov = NULL, *def_gov = NULL; struct cpufreq_policy new_policy; memcpy(&new_policy, policy, sizeof(*policy)); - /* Update governor of new_policy to the governor used before hotplug */ - gov = find_governor(policy->last_governor); - if (gov) { - pr_debug("Restoring governor %s for cpu %d\n", + def_gov = cpufreq_default_governor(); + + if (has_target()) { + /* + * Update governor of new_policy to the governor used before + * hotplug + */ + gov = find_governor(policy->last_governor); + if (gov) { + pr_debug("Restoring governor %s for cpu %d\n", policy->governor->name, policy->cpu); + } else { + if (!def_gov) + return -ENODATA; + gov = def_gov; + } + new_policy.governor = gov; } else { - gov = cpufreq_default_governor(); - if (!gov) - return -ENODATA; - } - - new_policy.governor = gov; - - /* Use the default policy if there is no last_policy. */ - if (cpufreq_driver->setpolicy) { - if (policy->last_policy) + /* Use the default policy if there is no last_policy. */ + if (policy->last_policy) { new_policy.policy = policy->last_policy; - else - cpufreq_parse_governor(gov->name, &new_policy); + } else { + if (!def_gov) + return -ENODATA; + cpufreq_parse_policy(def_gov->name, &new_policy); + } } - /* set default policy */ + return cpufreq_set_policy(policy, &new_policy); } -- cgit v1.2.3