Its better to use __func__ to print functions name instead of writing the name in the print statement. This also has the advantage that a change in function's name doesn't force us to change the print message as well.

Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 46251e8d30f2..f620055279f3 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -2074,8 +2074,7 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, if (!try_module_get(policy->governor->owner)) return -EINVAL;

- pr_debug("__cpufreq_governor for CPU %u, event %u\n", - policy->cpu, event); + pr_debug("%s: for CPU %u, event %u\n", __func__, policy->cpu, event);

mutex_lock(&cpufreq_governor_lock); if ((policy->governor_enabled && event == CPUFREQ_GOV_START)

Conservative governor has its own 'enable' field to check if conservative governor is used for a CPU or not

This can be checked by policy->governor with 'cpufreq_gov_conservative' and so this field can be dropped.

Because its not guaranteed that dbs_info->cdbs.shared will a valid pointer for all CPUs (will be NULL for CPUs that don't use ondemand/conservative governors), we can't use it anymore. Lets get policy with cpufreq_cpu_get() instead.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq_conservative.c | 34 +++++++++++++++++++++------------- drivers/cpufreq/cpufreq_governor.c | 12 +----------- drivers/cpufreq/cpufreq_governor.h | 1 - 3 files changed, 22 insertions(+), 25 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c index 84a1506950a7..18bfbc313e48 100644 --- a/drivers/cpufreq/cpufreq_conservative.c +++ b/drivers/cpufreq/cpufreq_conservative.c @@ -23,6 +23,19 @@

static DEFINE_PER_CPU(struct cs_cpu_dbs_info_s, cs_cpu_dbs_info);

+static int cs_cpufreq_governor_dbs(struct cpufreq_policy *policy, + unsigned int event); + +#ifndef CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE +static +#endif +struct cpufreq_governor cpufreq_gov_conservative = { + .name = "conservative", + .governor = cs_cpufreq_governor_dbs, + .max_transition_latency = TRANSITION_LATENCY_LIMIT, + .owner = THIS_MODULE, +}; + static inline unsigned int get_freq_target(struct cs_dbs_tuners *cs_tuners, struct cpufreq_policy *policy) { @@ -119,12 +132,14 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val, struct cpufreq_freqs *freq = data; struct cs_cpu_dbs_info_s *dbs_info = &per_cpu(cs_cpu_dbs_info, freq->cpu); - struct cpufreq_policy *policy; + struct cpufreq_policy *policy = cpufreq_cpu_get(freq->cpu);

- if (!dbs_info->enable) + if (!policy) return 0;

- policy = dbs_info->cdbs.shared->policy; + /* policy isn't governed by conservative governor */ + if (policy->governor != &cpufreq_gov_conservative) + goto policy_put;

/* * we only care if our internally tracked freq moves outside the 'valid' @@ -134,6 +149,9 @@ static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val, || dbs_info->requested_freq < policy->min) dbs_info->requested_freq = freq->new;

+policy_put: + cpufreq_cpu_put(policy); + return 0; }

@@ -367,16 +385,6 @@ static int cs_cpufreq_governor_dbs(struct cpufreq_policy *policy, return cpufreq_governor_dbs(policy, &cs_dbs_cdata, event); }

-#ifndef CONFIG_CPU_FREQ_DEFAULT_GOV_CONSERVATIVE -static -#endif -struct cpufreq_governor cpufreq_gov_conservative = { - .name = "conservative", - .governor = cs_cpufreq_governor_dbs, - .max_transition_latency = TRANSITION_LATENCY_LIMIT, - .owner = THIS_MODULE, -}; - static int __init cpufreq_gov_dbs_init(void) { return cpufreq_register_governor(&cpufreq_gov_conservative); diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 939197ffa4ac..750626d8fb03 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -463,7 +463,6 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, cdata->get_cpu_dbs_info_s(cpu);

cs_dbs_info->down_skip = 0; - cs_dbs_info->enable = 1; cs_dbs_info->requested_freq = policy->cur; } else { struct od_ops *od_ops = cdata->gov_ops; @@ -482,9 +481,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, static int cpufreq_governor_stop(struct cpufreq_policy *policy, struct dbs_data *dbs_data) { - struct common_dbs_data *cdata = dbs_data->cdata; - unsigned int cpu = policy->cpu; - struct cpu_dbs_info *cdbs = cdata->get_cpu_cdbs(cpu); + struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(policy->cpu); struct cpu_common_dbs_info *shared = cdbs->shared;

/* State should be equivalent to START */ @@ -493,13 +490,6 @@ static int cpufreq_governor_stop(struct cpufreq_policy *policy,

gov_cancel_work(dbs_data, policy);

- if (cdata->governor == GOV_CONSERVATIVE) { - struct cs_cpu_dbs_info_s *cs_dbs_info = - cdata->get_cpu_dbs_info_s(cpu); - - cs_dbs_info->enable = 0; - } - shared->policy = NULL; mutex_destroy(&shared->timer_mutex); return 0; diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h index 50f171796632..5621bb03e874 100644 --- a/drivers/cpufreq/cpufreq_governor.h +++ b/drivers/cpufreq/cpufreq_governor.h @@ -170,7 +170,6 @@ struct cs_cpu_dbs_info_s { struct cpu_dbs_info cdbs; unsigned int down_skip; unsigned int requested_freq; - unsigned int enable:1; };

/* Per policy Governors sysfs tunables */

The sampling rate is updated with a call to update_sampling_rate(), and we process CPUs one by one here. While the work is canceled on per-cpu basis, it is getting queued (by mistake) for all policy->cpus.

This would result in wasting cpu cycles for queuing works which are already queued and never canceled.

This patch changes this behavior to queue work only on the cpu, for which it was canceled earlier.

To do that, replace 'modify_all' parameter to gov_queue_work() with a mask of CPUs. Also the last parameter to ->gov_dbs_timer() was named 'modify_all' earlier, but its purpose was to decide if load has to be evaluated again or not. Lets rename that to load_eval.

Fixes: 031299b3be30 ("cpufreq: governors: Avoid unnecessary per cpu timer interrupts") Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq_conservative.c | 4 ++-- drivers/cpufreq/cpufreq_governor.c | 30 ++++++++++-------------------- drivers/cpufreq/cpufreq_governor.h | 4 ++-- drivers/cpufreq/cpufreq_ondemand.c | 7 ++++--- 4 files changed, 18 insertions(+), 27 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c index 18bfbc313e48..1aa3bd46cea3 100644 --- a/drivers/cpufreq/cpufreq_conservative.c +++ b/drivers/cpufreq/cpufreq_conservative.c @@ -116,11 +116,11 @@ static void cs_check_cpu(int cpu, unsigned int load) }

static unsigned int cs_dbs_timer(struct cpu_dbs_info *cdbs, - struct dbs_data *dbs_data, bool modify_all) + struct dbs_data *dbs_data, bool load_eval) { struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;

- if (modify_all) + if (load_eval) dbs_check_cpu(dbs_data, cdbs->shared->policy->cpu);

return delay_for_sampling_rate(cs_tuners->sampling_rate); diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 750626d8fb03..a890450711bb 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -167,7 +167,7 @@ static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, }

void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, - unsigned int delay, bool all_cpus) + unsigned int delay, const struct cpumask *cpus) { int i;

@@ -175,19 +175,8 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, if (!policy->governor_enabled) goto out_unlock;

- if (!all_cpus) { - /* - * Use raw_smp_processor_id() to avoid preemptible warnings. - * We know that this is only called with all_cpus == false from - * works that have been queued with *_work_on() functions and - * those works are canceled during CPU_DOWN_PREPARE so they - * can't possibly run on any other CPU. - */ - __gov_queue_work(raw_smp_processor_id(), dbs_data, delay); - } else { - for_each_cpu(i, policy->cpus) - __gov_queue_work(i, dbs_data, delay); - } + for_each_cpu(i, cpus) + __gov_queue_work(i, dbs_data, delay);

out_unlock: mutex_unlock(&cpufreq_governor_lock); @@ -232,7 +221,8 @@ static void dbs_timer(struct work_struct *work) struct cpufreq_policy *policy = shared->policy; struct dbs_data *dbs_data = policy->governor_data; unsigned int sampling_rate, delay; - bool modify_all = true; + const struct cpumask *cpus; + bool load_eval;

mutex_lock(&shared->timer_mutex);

@@ -246,11 +236,11 @@ static void dbs_timer(struct work_struct *work) sampling_rate = od_tuners->sampling_rate; }

- if (!need_load_eval(cdbs->shared, sampling_rate)) - modify_all = false; + load_eval = need_load_eval(cdbs->shared, sampling_rate); + cpus = load_eval ? policy->cpus : cpumask_of(raw_smp_processor_id());

- delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, modify_all); - gov_queue_work(dbs_data, policy, delay, modify_all); + delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, load_eval); + gov_queue_work(dbs_data, policy, delay, cpus);

mutex_unlock(&shared->timer_mutex); } @@ -474,7 +464,7 @@ static int cpufreq_governor_start(struct cpufreq_policy *policy, }

gov_queue_work(dbs_data, policy, delay_for_sampling_rate(sampling_rate), - true); + policy->cpus); return 0; }

diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h index 5621bb03e874..52665a0624b2 100644 --- a/drivers/cpufreq/cpufreq_governor.h +++ b/drivers/cpufreq/cpufreq_governor.h @@ -211,7 +211,7 @@ struct common_dbs_data { void *(*get_cpu_dbs_info_s)(int cpu); unsigned int (*gov_dbs_timer)(struct cpu_dbs_info *cdbs, struct dbs_data *dbs_data, - bool modify_all); + bool load_eval); void (*gov_check_cpu)(int cpu, unsigned int load); int (*init)(struct dbs_data *dbs_data, bool notify); void (*exit)(struct dbs_data *dbs_data, bool notify); @@ -273,7 +273,7 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu); int cpufreq_governor_dbs(struct cpufreq_policy *policy, struct common_dbs_data *cdata, unsigned int event); void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, - unsigned int delay, bool all_cpus); + unsigned int delay, const struct cpumask *cpus); void od_register_powersave_bias_handler(unsigned int (*f) (struct cpufreq_policy *, unsigned int, unsigned int), unsigned int powersave_bias); diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index 1fa9088c84a8..2474c9c34022 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -192,7 +192,7 @@ static void od_check_cpu(int cpu, unsigned int load) }

static unsigned int od_dbs_timer(struct cpu_dbs_info *cdbs, - struct dbs_data *dbs_data, bool modify_all) + struct dbs_data *dbs_data, bool load_eval) { struct cpufreq_policy *policy = cdbs->shared->policy; unsigned int cpu = policy->cpu; @@ -201,7 +201,7 @@ static unsigned int od_dbs_timer(struct cpu_dbs_info *cdbs, struct od_dbs_tuners *od_tuners = dbs_data->tuners; int delay = 0, sample_type = dbs_info->sample_type;

- if (!modify_all) + if (!load_eval) goto max_delay;

/* Common NORMAL_SAMPLE setup */ @@ -284,7 +284,8 @@ static void update_sampling_rate(struct dbs_data *dbs_data, mutex_lock(&dbs_info->cdbs.shared->timer_mutex);

gov_queue_work(dbs_data, policy, - usecs_to_jiffies(new_rate), true); + usecs_to_jiffies(new_rate), + cpumask_of(cpu));

} mutex_unlock(&dbs_info->cdbs.shared->timer_mutex);

__gov_queue_work() isn't required anymore and can be merged with gov_queue_work(). Do it.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq_governor.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index a890450711bb..3ddc27764e10 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -158,25 +158,20 @@ void dbs_check_cpu(struct dbs_data *dbs_data, int cpu) } EXPORT_SYMBOL_GPL(dbs_check_cpu);

-static inline void __gov_queue_work(int cpu, struct dbs_data *dbs_data, - unsigned int delay) -{ - struct cpu_dbs_info *cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); - - mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); -} - void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, unsigned int delay, const struct cpumask *cpus) { - int i; + struct cpu_dbs_info *cdbs; + int cpu;

mutex_lock(&cpufreq_governor_lock); if (!policy->governor_enabled) goto out_unlock;

- for_each_cpu(i, cpus) - __gov_queue_work(i, dbs_data, delay); + for_each_cpu(cpu, cpus) { + cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); + mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); + }

out_unlock: mutex_unlock(&cpufreq_governor_lock);

'timer_mutex' is required to sync work-handlers of policy->cpus. update_sampling_rate() is just canceling the works and queuing them again. This isn't protecting anything at all in update_sampling_rate() and is not gonna be of any use.

Even if a work-handler is already running for a CPU, cancel_delayed_work_sync() will wait for it to finish.

Drop these unnecessary locks.

Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq_ondemand.c | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index 2474c9c34022..f1551fc7b4fd 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -267,28 +267,20 @@ static void update_sampling_rate(struct dbs_data *dbs_data, dbs_info = &per_cpu(od_cpu_dbs_info, cpu); cpufreq_cpu_put(policy);

- mutex_lock(&dbs_info->cdbs.shared->timer_mutex); - - if (!delayed_work_pending(&dbs_info->cdbs.dwork)) { - mutex_unlock(&dbs_info->cdbs.shared->timer_mutex); + if (!delayed_work_pending(&dbs_info->cdbs.dwork)) continue; - }

next_sampling = jiffies + usecs_to_jiffies(new_rate); appointed_at = dbs_info->cdbs.dwork.timer.expires;

if (time_before(next_sampling, appointed_at)) { - - mutex_unlock(&dbs_info->cdbs.shared->timer_mutex); cancel_delayed_work_sync(&dbs_info->cdbs.dwork); - mutex_lock(&dbs_info->cdbs.shared->timer_mutex);

gov_queue_work(dbs_data, policy, usecs_to_jiffies(new_rate), cpumask_of(cpu));

} - mutex_unlock(&dbs_info->cdbs.shared->timer_mutex); } }

cpufreq_governor_lock is abused by using it outside of cpufreq core, i.e. in cpufreq-governors. But we didn't had a solution at that point of time, and so doing that was the only acceptable solution:

6f1e4efd882e ("cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled")

The cpufreq governor core is fixed now against possible races and things are in much better shape.

cpufreq core is checking for invalid state-transitions of governors in __cpufreq_governor() with help of governor_enabled flag. The governor core is already taking care of that now and so we can get rid of those extra checks in __cpufreq_governor().

To do that, we first need to get rid of the dependency on governor_enabled flag in governor core, in gov_queue_work.

This patch is about getting rid of this dependency.

When a CPU is hot removed we'll cancel all the delayed work items via gov_cancel_work(). Normally this will just cancels a delayed timer on each CPU that the policy is managing and the work won't run. But if the work is already running, the workqueue code will wait for the work to finish before continuing to prevent the work items from re-queuing themselves like they normally do.

This scheme will work most of the time, except for the case where the work function determines that it should adjust the delay for all other CPUs that the policy is managing. If this scenario occurs, the canceling CPU will cancel its own work but queue up the other CPUs works to run.

And we will enter a situation where gov_cancel_work() has returned with work being queued on few CPUs.

To fix that in a different (non-hacky) way, set set shared->policy to false before trying to cancel the work. It should be updated within timer_mutex, which will prevent the work-handlers to start. Once the work-handlers finds that we are already trying to stop the governor, it will exit early. And that will prevent queuing of works again as well.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq_governor.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 3ddc27764e10..bb12acff3ba6 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -164,17 +164,10 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, struct cpu_dbs_info *cdbs; int cpu;

- mutex_lock(&cpufreq_governor_lock); - if (!policy->governor_enabled) - goto out_unlock; - for_each_cpu(cpu, cpus) { cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); } - -out_unlock: - mutex_unlock(&cpufreq_governor_lock); } EXPORT_SYMBOL_GPL(gov_queue_work);

@@ -213,14 +206,25 @@ static void dbs_timer(struct work_struct *work) struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info, dwork.work); struct cpu_common_dbs_info *shared = cdbs->shared; - struct cpufreq_policy *policy = shared->policy; - struct dbs_data *dbs_data = policy->governor_data; + struct cpufreq_policy *policy; + struct dbs_data *dbs_data; unsigned int sampling_rate, delay; const struct cpumask *cpus; bool load_eval;

mutex_lock(&shared->timer_mutex);

+ policy = shared->policy; + + /* + * Governor might already be disabled and there is no point continuing + * with the work-handler. + */ + if (!policy) + goto unlock; + + dbs_data = policy->governor_data; + if (dbs_data->cdata->governor == GOV_CONSERVATIVE) { struct cs_dbs_tuners *cs_tuners = dbs_data->tuners;

@@ -237,6 +241,7 @@ static void dbs_timer(struct work_struct *work) delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, load_eval); gov_queue_work(dbs_data, policy, delay, cpus);

+unlock: mutex_unlock(&shared->timer_mutex); }

@@ -473,9 +478,17 @@ static int cpufreq_governor_stop(struct cpufreq_policy *policy, if (!shared || !shared->policy) return -EBUSY;

+ /* + * Work-handler must see this updated, as it should not proceed any + * further after governor is disabled. And so timer_mutex is taken while + * updating this value. + */ + mutex_lock(&shared->timer_mutex); + shared->policy = NULL; + mutex_unlock(&shared->timer_mutex); + gov_cancel_work(dbs_data, policy);

- shared->policy = NULL; mutex_destroy(&shared->timer_mutex); return 0; }