On 26-06-15, 11:27, Preeti U Murthy wrote:

On 06/22/2015 01:32 PM, Viresh Kumar wrote:

...

Conservative governor has its own 'enable' field to check two things:

• If conservative governor is used for a CPU or not
• If governor is currently enabled or not, as there can be a race around the notifier being called while it was getting unregistered from cpufreq_governor_dbs().

The race is between changing governors in cpufreq_set_policy() and the notifier being called, isn't it ? The governor will get unregistered when we remove the cpufreq module and here too we do not set policy->governor to NULL nor set the enable bit to 0. So it does not look like we were protecting these checks against un-registering the governor.

I was talking about the same race which I believed to exist in 2/10 as well.. But there is no such race it seems as we discussed yesterday. So, only the first point is what the enable field was required for.

And because of that getting NAK'd, here is the new version:

-------------------------8<-----------------

Message-Id: 4ce8fc98a46af394118237ef90ed0b80a7652cfb.1435299551.git.viresh.kumar@linaro.org From: Viresh Kumar viresh.kumar@linaro.org Date: Fri, 19 Jun 2015 11:40:14 +0530 Subject: [PATCH] cpufreq: conservative: remove 'enable' field

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.ccdbs 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 0e4154e584bf..f53719e5bed9 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.ccdbs->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 af63402a94a9..836aefd03c1b 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 *ccdbs = cdbs->ccdbs;

/* 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; - } - ccdbs->policy = NULL; mutex_destroy(&ccdbs->timer_mutex); return 0; diff --git a/drivers/cpufreq/cpufreq_governor.h b/drivers/cpufreq/cpufreq_governor.h index 2125c299c602..a0d24149f18c 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 */

On 06/22/2015 01:32 PM, Viresh Kumar wrote:

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 enqueued (by mistake) for all policy->cpus.

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

This patch is about queuing work only on the cpu for which it was canceled earlier.

gov_queue_work() was missing the CPU parameter and it's better to club 'modify_all' and the new 'cpu' parameter to a 'cpus' mask. And so this patch also changes the prototype of gov_queue_work() and fixes its caller sites.

This looks good, except I did not understand the motivation to change the 'modify_all' to 'load_eval'. Neither is saying the purpose better than the other.

Regards Preeti U Murthy

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 | 2 +- drivers/cpufreq/cpufreq_ondemand.c | 7 ++++--- 4 files changed, 17 insertions(+), 26 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c index f53719e5bed9..2ab53d96c078 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->ccdbs->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 836aefd03c1b..416a8c5665dd 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 = ccdbs->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(&ccdbs->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->ccdbs, sampling_rate))

• modify_all = false;
  

• load_eval = need_load_eval(cdbs->ccdbs, 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(&ccdbs->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 a0d24149f18c..dc2ad8a427f3 100644 --- a/drivers/cpufreq/cpufreq_governor.h +++ b/drivers/cpufreq/cpufreq_governor.h @@ -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 11db20079fc6..774bbddae2c9 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->ccdbs->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.ccdbs->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.ccdbs->timer_mutex);

On 26-06-15, 12:33, Preeti U Murthy wrote:

On 06/22/2015 01:32 PM, Viresh Kumar wrote:

...

__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 416a8c5665dd..ac288f0efb06 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);

-}

Looking at this patch I feel now that, you can simply make the above function non static and call the same from update_sampling_rate() in patch[04/10] It already gives scope to pass in the cpu parameter, if not the mask. Anyway you want to pass in a single cpu from update_sampling_rate(). This will avoid having to change the other call sites of gov_queue_work().

I also see elsewhere in the kernel where a fn() and __fn() are sometimes both non-static, with __fn() giving you the additional benefit of passing a critical parameter. Eg: __hrtimer_start_range_ns() and hrtimer_start_range_ns().

Keeping two functions that way isn't a problem and is beneficial many a times. But that doesn't fit here.

The above function doesn't have the necessary lock and so was never used directly. And there is no need to keep two routines now that only one is sufficient.

On 06/22/2015 01:32 PM, Viresh Kumar wrote:

'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.

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

Reviewed-by: Preeti U Murthy preeti@linux.vnet.ibm.com

---

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 774bbddae2c9..841e1fa96ee7 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.ccdbs->timer_mutex);
  

• if (!delayed_work_pending(&dbs_info->cdbs.dwork)) {
  

• 	mutex_unlock(&dbs_info->cdbs.ccdbs->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.ccdbs->timer_mutex);
  cancel_delayed_work_sync(&dbs_info->cdbs.dwork);
  

• 	mutex_lock(&dbs_info->cdbs.ccdbs->timer_mutex);
  
  gov_queue_work(dbs_data, policy,
  	       usecs_to_jiffies(new_rate),
  	       cpumask_of(cpu));
  
  

  }

• mutex_unlock(&dbs_info->cdbs.ccdbs->timer_mutex);
  

  }

}

On 06/22/2015 01:32 PM, Viresh Kumar wrote:

Currently update_sampling_rate() runs over each online CPU and cancels/queues work on it. Its very inefficient for the case where a single policy manages multiple CPUs, as they can be processed together.

Also drop the unnecessary cancel_delayed_work_sync() as we are doing a mod_delayed_work_on() in gov_queue_work(), which will take care of pending works for us.

This looks fine, except for one point. See below:

Signed-off-by: Viresh Kumar viresh.kumar@linaro.org

drivers/cpufreq/cpufreq_ondemand.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_ondemand.c b/drivers/cpufreq/cpufreq_ondemand.c index 841e1fa96ee7..cfecd3b67cb3 100644 --- a/drivers/cpufreq/cpufreq_ondemand.c +++ b/drivers/cpufreq/cpufreq_ondemand.c @@ -247,40 +247,48 @@ static void update_sampling_rate(struct dbs_data *dbs_data, unsigned int new_rate) { struct od_dbs_tuners *od_tuners = dbs_data->tuners;

• struct cpufreq_policy *policy;

• struct od_cpu_dbs_info_s *dbs_info;

• unsigned long next_sampling, appointed_at;

• struct cpumask cpumask; int cpu;

• cpumask_copy(&cpumask, cpu_online_mask);

• od_tuners->sampling_rate = new_rate = max(new_rate, dbs_data->min_sampling_rate);

• for_each_online_cpu(cpu) {

• struct cpufreq_policy *policy;
  

• struct od_cpu_dbs_info_s *dbs_info;
  

• unsigned long next_sampling, appointed_at;
  

• for_each_cpu(cpu, &cpumask) { policy = cpufreq_cpu_get(cpu); if (!policy) continue;

• /* clear all CPUs of this policy */
  

• cpumask_andnot(&cpumask, &cpumask, policy->cpus);
  

• if (policy->governor != &cpufreq_gov_ondemand) { cpufreq_cpu_put(policy); continue; }

• dbs_info = &per_cpu(od_cpu_dbs_info, cpu); cpufreq_cpu_put(policy);

• /*
  

•  * Checking this for any CPU of the policy is fine. As either
  

•  * all would have queued work or none.
  

Are you sure that the state of the work will be the same across all policy cpus ? 'Pending' only refers to twork awaiting for the timer to fire and then queue itself on the runqueue right ? On some of the policy->cpus, timers may be yet to fire, while on others it might already have ?

•  */
  

  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)) {
  

• 	cancel_delayed_work_sync(&dbs_info->cdbs.dwork);
  

• 	gov_queue_work(dbs_data, policy,
  

• 		       usecs_to_jiffies(new_rate),
  

• 		       cpumask_of(cpu));
  

• if (!time_before(next_sampling, appointed_at))
  

• 	continue;
  
  

• }
  

• gov_queue_work(dbs_data, policy, usecs_to_jiffies(new_rate),
  

• 	       policy->cpus);
  

  }

}

Regards Preeti U Murthy