Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1334086 > unrolled thread

[PATCH 0/9] cpufreq governor improvements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/9] cpufreq governor improvements "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
    [PATCH 1/9] cpufreq: governor: Simplify gov_cancel_work() slightly "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
      Re: [PATCH 1/9] cpufreq: governor: Simplify gov_cancel_work()  slightly Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 06:50 +0100
    [PATCH 2/9] cpufreq: governor: Avoid atomic operations in hot paths "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
      Re: [PATCH 2/9] cpufreq: governor: Avoid atomic operations in hot  paths Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 07:20 +0100
      Re: [PATCH 2/9] cpufreq: governor: Avoid atomic operations in hot  paths Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 09:30 +0100
    [PATCH 7/9] cpufreq: governor: Move rate_mult to struct policy_dbs "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
      Re: [PATCH 7/9] cpufreq: governor: Move rate_mult to struct  policy_dbs Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 10:00 +0100
    [PATCH 3/9] cpufreq: governor: Fix nice contribution computation in dbs_check_cpu() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
      Re: [PATCH 3/9] cpufreq: governor: Fix nice contribution computation  in dbs_check_cpu() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 09:30 +0100
    [PATCH 9/9] cpufreq: governor: Use microseconds in sample delay computations "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
      Re: [PATCH 9/9] cpufreq: governor: Use microseconds in sample delay  computations Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 10:00 +0100
    [PATCH 8/9] cpufreq: ondemand: Simplify conditionals in od_dbs_timer() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-15 02:30 +0100
      Re: [PATCH 8/9] cpufreq: ondemand: Simplify conditionals in  od_dbs_timer() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-15 10:00 +0100

#1334086 — [PATCH 0/9] cpufreq governor improvements

Hi All,

Here's a buch of patches that slightly improve the ondemand/conservative code
on top of the current linux-next branch of my tree (linux-pm.git).

They don't change the way things work fundamentally, but some minor differences
may be noticeable.

[1/9] Do not do atomic_inc() in gov_cancel_work() as there's no reason to do it
      (new version).

[2/9] Avoid atomic ops in scheduler paths if not absolutely necessary (new version).

[3/9] Fix computation of the contribution from nice in dbs_check_cpu().

[4/9] Clean up load-related computations in the common governor code.

[5/9] Get rid of the ->gov_check_cpu callback.

[6/9] Make store_sampling_rate() reset the sample delay to 0 (instead of setting
      it to the new sampling rate) to avoid weird interactions with the ondemand
      governor.

[7/9] Move rate_mult to struct policy_dbs_info.

[8/9] Simplify conditionals in od_dbs_timer().

[9/9] Use microseconds in computations related to sample delay.

The series have been (lightly) tested on Toshiba Portege R500.

Thanks,
Rafael

[toc] | [next] | [standalone]

#1334088 — [PATCH 1/9] cpufreq: governor: Simplify gov_cancel_work() slightly

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The atomic work counter incrementation in gov_cancel_work() is not
necessary any more, because work items won't be queued up after
gov_clear_update_util() anyway, so drop it along with the comment
about how it may be missed by the gov_clear_update_util().

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This is a new version of https://patchwork.kernel.org/patch/8291021/ .

Changes from the previous version:
- Rebase.

---
 drivers/cpufreq/cpufreq_governor.c |    8 --------
 1 file changed, 8 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -300,13 +300,6 @@ static void gov_cancel_work(struct cpufr
 {
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 
-	/* Tell dbs_update_util_handler() to skip queuing up work items. */
-	atomic_inc(&policy_dbs->work_count);
-	/*
-	 * If dbs_update_util_handler() is already running, it may not notice
-	 * the incremented work_count, so wait for it to complete to prevent its
-	 * work item from being queued up after the cancel_work_sync() below.
-	 */
 	gov_clear_update_util(policy_dbs->policy);
 	irq_work_sync(&policy_dbs->irq_work);
 	cancel_work_sync(&policy_dbs->work);
@@ -369,7 +362,6 @@ static void dbs_update_util_handler(stru
 	 * The work may not be allowed to be queued up right now.
 	 * Possible reasons:
 	 * - Work has already been queued up or is in progress.
-	 * - The governor is being stopped.
 	 * - It is too early (too little time from the previous sample).
 	 */
 	if (atomic_inc_return(&policy_dbs->work_count) == 1) {

[toc] | [prev] | [next] | [standalone]

#1334167 — Re: [PATCH 1/9] cpufreq: governor: Simplify gov_cancel_work() slightly

On 15-02-16, 02:12, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The atomic work counter incrementation in gov_cancel_work() is not
> necessary any more, because work items won't be queued up after
> gov_clear_update_util() anyway, so drop it along with the comment
> about how it may be missed by the gov_clear_update_util().
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a new version of https://patchwork.kernel.org/patch/8291021/ .
> 
> Changes from the previous version:
> - Rebase.
> 
> ---
>  drivers/cpufreq/cpufreq_governor.c |    8 --------
>  1 file changed, 8 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c
> @@ -300,13 +300,6 @@ static void gov_cancel_work(struct cpufr
>  {
>  	struct policy_dbs_info *policy_dbs = policy->governor_data;
>  
> -	/* Tell dbs_update_util_handler() to skip queuing up work items. */
> -	atomic_inc(&policy_dbs->work_count);
> -	/*
> -	 * If dbs_update_util_handler() is already running, it may not notice
> -	 * the incremented work_count, so wait for it to complete to prevent its
> -	 * work item from being queued up after the cancel_work_sync() below.
> -	 */
>  	gov_clear_update_util(policy_dbs->policy);
>  	irq_work_sync(&policy_dbs->irq_work);
>  	cancel_work_sync(&policy_dbs->work);
> @@ -369,7 +362,6 @@ static void dbs_update_util_handler(stru
>  	 * The work may not be allowed to be queued up right now.
>  	 * Possible reasons:
>  	 * - Work has already been queued up or is in progress.
> -	 * - The governor is being stopped.
>  	 * - It is too early (too little time from the previous sample).
>  	 */
>  	if (atomic_inc_return(&policy_dbs->work_count) == 1) {


Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1334089 — [PATCH 2/9] cpufreq: governor: Avoid atomic operations in hot paths

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rework the handling of work items by dbs_update_util_handler() and
dbs_work_handler() so the former (which is executed in scheduler
paths) only uses atomic operations when absolutely necessary.  That
is, when the policy is shared and dbs_update_util_handler() has
already decided that this is the time to queue up a work item.

In particular, this avoids the atomic ops entirely on platforms where
policy objects are never shared.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

This is a new version of https://patchwork.kernel.org/patch/8291051/ .

Changes from the previous version:
- Added a new "is_shared" field to struct policy_dbs_info to be set for
  shared policies to avoid evaluating cpumask_weight() every time
  dbs_update_util_handler() decides to take a sample.

---
 drivers/cpufreq/cpufreq_governor.c |   49 +++++++++++++++++++++++++------------
 drivers/cpufreq/cpufreq_governor.h |    3 ++
 2 files changed, 37 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -304,6 +304,7 @@ static void gov_cancel_work(struct cpufr
 	irq_work_sync(&policy_dbs->irq_work);
 	cancel_work_sync(&policy_dbs->work);
 	atomic_set(&policy_dbs->work_count, 0);
+	policy_dbs->work_in_progress = false;
 }
 
 static void dbs_work_handler(struct work_struct *work)
@@ -326,13 +327,15 @@ static void dbs_work_handler(struct work
 	policy_dbs->sample_delay_ns = jiffies_to_nsecs(delay);
 	mutex_unlock(&policy_dbs->timer_mutex);
 
+	/* Allow the utilization update handler to queue up more work. */
+	atomic_set(&policy_dbs->work_count, 0);
 	/*
-	 * If the atomic operation below is reordered with respect to the
-	 * sample delay modification, the utilization update handler may end
-	 * up using a stale sample delay value.
+	 * If the update below is reordered with respect to the sample delay
+	 * modification, the utilization update handler may end up using a stale
+	 * sample delay value.
 	 */
-	smp_mb__before_atomic();
-	atomic_dec(&policy_dbs->work_count);
+	smp_wmb();
+	policy_dbs->work_in_progress = false;
 }
 
 static void dbs_irq_work(struct irq_work *irq_work)
@@ -357,6 +360,7 @@ static void dbs_update_util_handler(stru
 {
 	struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
 	struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
+	u64 delta_ns;
 
 	/*
 	 * The work may not be allowed to be queued up right now.
@@ -364,17 +368,30 @@ static void dbs_update_util_handler(stru
 	 * - Work has already been queued up or is in progress.
 	 * - It is too early (too little time from the previous sample).
 	 */
-	if (atomic_inc_return(&policy_dbs->work_count) == 1) {
-		u64 delta_ns;
+	if (policy_dbs->work_in_progress)
+		return;
 
-		delta_ns = time - policy_dbs->last_sample_time;
-		if ((s64)delta_ns >= policy_dbs->sample_delay_ns) {
-			policy_dbs->last_sample_time = time;
-			gov_queue_irq_work(policy_dbs);
-			return;
-		}
-	}
-	atomic_dec(&policy_dbs->work_count);
+	/*
+	 * If the reads below are reordered before the check above, the value
+	 * of sample_delay_ns used in the computation may be stale.
+	 */
+	smp_rmb();
+	delta_ns = time - policy_dbs->last_sample_time;
+	if ((s64)delta_ns < policy_dbs->sample_delay_ns)
+		return;
+
+	/*
+	 * If the policy is not shared, the irq_work may be queued up right away
+	 * at this point.  Otherwise, we need to ensure that only one of the
+	 * CPUs sharing the policy will do that.
+	 */
+	if (policy_dbs->is_shared &&
+	    !atomic_add_unless(&policy_dbs->work_count, 1, 1))
+		return;
+
+	policy_dbs->last_sample_time = time;
+	policy_dbs->work_in_progress = true;
+	gov_queue_irq_work(policy_dbs);
 }
 
 static struct policy_dbs_info *alloc_policy_dbs_info(struct cpufreq_policy *policy,
@@ -551,6 +568,8 @@ static int cpufreq_governor_start(struct
 	if (!policy->cur)
 		return -EINVAL;
 
+	policy_dbs->is_shared = policy_is_shared(policy);
+
 	sampling_rate = dbs_data->sampling_rate;
 	ignore_nice = dbs_data->ignore_nice_load;
 
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -130,6 +130,9 @@ struct policy_dbs_info {
 	/* dbs_data may be shared between multiple policy objects */
 	struct dbs_data *dbs_data;
 	struct list_head list;
+	/* Status indicators */
+	bool is_shared;		/* This object is used by multiple CPUs */
+	bool work_in_progress;	/* Work is being queued up or in progress */
 };
 
 static inline void gov_update_sample_delay(struct policy_dbs_info *policy_dbs,

[toc] | [prev] | [next] | [standalone]

#1334180 — Re: [PATCH 2/9] cpufreq: governor: Avoid atomic operations in hot paths

On 15-02-16, 02:13, Rafael J. Wysocki wrote:
>  static void dbs_irq_work(struct irq_work *irq_work)
> @@ -357,6 +360,7 @@ static void dbs_update_util_handler(stru
>  {
>  	struct cpu_dbs_info *cdbs = container_of(data, struct cpu_dbs_info, update_util);
>  	struct policy_dbs_info *policy_dbs = cdbs->policy_dbs;
> +	u64 delta_ns;
>  
>  	/*
>  	 * The work may not be allowed to be queued up right now.
> @@ -364,17 +368,30 @@ static void dbs_update_util_handler(stru
>  	 * - Work has already been queued up or is in progress.
>  	 * - It is too early (too little time from the previous sample).
>  	 */
> -	if (atomic_inc_return(&policy_dbs->work_count) == 1) {
> -		u64 delta_ns;
> +	if (policy_dbs->work_in_progress)
> +		return;
>  
> -		delta_ns = time - policy_dbs->last_sample_time;
> -		if ((s64)delta_ns >= policy_dbs->sample_delay_ns) {
> -			policy_dbs->last_sample_time = time;
> -			gov_queue_irq_work(policy_dbs);
> -			return;
> -		}
> -	}
> -	atomic_dec(&policy_dbs->work_count);
> +	/*
> +	 * If the reads below are reordered before the check above, the value
> +	 * of sample_delay_ns used in the computation may be stale.
> +	 */
> +	smp_rmb();
> +	delta_ns = time - policy_dbs->last_sample_time;
> +	if ((s64)delta_ns < policy_dbs->sample_delay_ns)
> +		return;
> +
> +	/*
> +	 * If the policy is not shared, the irq_work may be queued up right away
> +	 * at this point.  Otherwise, we need to ensure that only one of the
> +	 * CPUs sharing the policy will do that.
> +	 */
> +	if (policy_dbs->is_shared &&
> +	    !atomic_add_unless(&policy_dbs->work_count, 1, 1))
> +		return;
> +
> +	policy_dbs->last_sample_time = time;
> +	policy_dbs->work_in_progress = true;
> +	gov_queue_irq_work(policy_dbs);
>  }
>  
>  static struct policy_dbs_info *alloc_policy_dbs_info(struct cpufreq_policy *policy,
> @@ -551,6 +568,8 @@ static int cpufreq_governor_start(struct
>  	if (!policy->cur)
>  		return -EINVAL;
>  
> +	policy_dbs->is_shared = policy_is_shared(policy);
> +
>  	sampling_rate = dbs_data->sampling_rate;
>  	ignore_nice = dbs_data->ignore_nice_load;
>  
> Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
> +++ linux-pm/drivers/cpufreq/cpufreq_governor.h
> @@ -130,6 +130,9 @@ struct policy_dbs_info {
>  	/* dbs_data may be shared between multiple policy objects */
>  	struct dbs_data *dbs_data;
>  	struct list_head list;
> +	/* Status indicators */
> +	bool is_shared;		/* This object is used by multiple CPUs */
> +	bool work_in_progress;	/* Work is being queued up or in progress */
>  };
>  
>  static inline void gov_update_sample_delay(struct policy_dbs_info *policy_dbs,

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1334233 — Re: [PATCH 2/9] cpufreq: governor: Avoid atomic operations in hot paths

On 15-02-16, 02:13, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Rework the handling of work items by dbs_update_util_handler() and
> dbs_work_handler() so the former (which is executed in scheduler
> paths) only uses atomic operations when absolutely necessary.  That
> is, when the policy is shared and dbs_update_util_handler() has
> already decided that this is the time to queue up a work item.
> 
> In particular, this avoids the atomic ops entirely on platforms where
> policy objects are never shared.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> This is a new version of https://patchwork.kernel.org/patch/8291051/ .
> 
> Changes from the previous version:
> - Added a new "is_shared" field to struct policy_dbs_info to be set for
>   shared policies to avoid evaluating cpumask_weight() every time
>   dbs_update_util_handler() decides to take a sample.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1334090 — [PATCH 7/9] cpufreq: governor: Move rate_mult to struct policy_dbs

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The rate_mult field in struct od_cpu_dbs_info_s is used by the code
shared with the conservative governor and to access it that code
has to do an ugly governor type check.  However, first of all it
is ever only used for policy->cpu, so it is per-policy rather than
per-CPU and second, it is initialized to 1 by cpufreq_governor_start(),
so if the conservative governor never modifies it, it will have no
effect on the results of any computations.

For these reasons, move rate_mult to struct policy_dbs_info (as a
common field).

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_governor.c |   25 +++++++++----------------
 drivers/cpufreq/cpufreq_governor.h |    3 ++-
 drivers/cpufreq/cpufreq_ondemand.c |   23 +++++++++++++++--------
 3 files changed, 26 insertions(+), 25 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -138,24 +138,17 @@ unsigned int dbs_update(struct cpufreq_p
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
 	struct od_dbs_tuners *od_tuners = dbs_data->tuners;
-	unsigned int sampling_rate = dbs_data->sampling_rate;
 	unsigned int ignore_nice = dbs_data->ignore_nice_load;
 	unsigned int max_load = 0;
-	unsigned int j;
+	unsigned int sampling_rate, j;
 
-	if (gov->governor == GOV_ONDEMAND) {
-		struct od_cpu_dbs_info_s *od_dbs_info =
-				gov->get_cpu_dbs_info_s(policy->cpu);
-
-		/*
-		 * Sometimes, the ondemand governor uses an additional
-		 * multiplier to give long delays. So apply this multiplier to
-		 * the 'sampling_rate', so as to keep the wake-up-from-idle
-		 * detection logic a bit conservative.
-		 */
-		sampling_rate *= od_dbs_info->rate_mult;
-
-	}
+	/*
+	 * Sometimes governors may use an additional multiplier to increase
+	 * sample delays temporarily.  Apply that multiplier to sampling_rate
+	 * so as to keep the wake-up-from-idle detection logic a bit
+	 * conservative.
+	 */
+	sampling_rate = dbs_data->sampling_rate * policy_dbs->rate_mult;
 
 	/* Get Absolute Load */
 	for_each_cpu(j, policy->cpus) {
@@ -546,6 +539,7 @@ static int cpufreq_governor_start(struct
 		return -EINVAL;
 
 	policy_dbs->is_shared = policy_is_shared(policy);
+	policy_dbs->rate_mult = 1;
 
 	sampling_rate = dbs_data->sampling_rate;
 	ignore_nice = dbs_data->ignore_nice_load;
@@ -579,7 +573,6 @@ static int cpufreq_governor_start(struct
 		struct od_ops *od_ops = gov->gov_ops;
 		struct od_cpu_dbs_info_s *od_dbs_info = gov->get_cpu_dbs_info_s(cpu);
 
-		od_dbs_info->rate_mult = 1;
 		od_dbs_info->sample_type = OD_NORMAL_SAMPLE;
 		od_ops->powersave_bias_init_cpu(cpu);
 	}
Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -130,6 +130,8 @@ struct policy_dbs_info {
 	/* dbs_data may be shared between multiple policy objects */
 	struct dbs_data *dbs_data;
 	struct list_head list;
+	/* Multiplier for increasing sample delay temporarily. */
+	unsigned int rate_mult;
 	/* Status indicators */
 	bool is_shared;		/* This object is used by multiple CPUs */
 	bool work_in_progress;	/* Work is being queued up or in progress */
@@ -163,7 +165,6 @@ struct od_cpu_dbs_info_s {
 	unsigned int freq_lo;
 	unsigned int freq_lo_jiffies;
 	unsigned int freq_hi_jiffies;
-	unsigned int rate_mult;
 	unsigned int sample_type:1;
 };
 
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -164,7 +164,7 @@ static void od_update(struct cpufreq_pol
 	if (load > dbs_data->up_threshold) {
 		/* If switching to max speed, apply sampling_down_factor */
 		if (policy->cur < policy->max)
-			dbs_info->rate_mult = dbs_data->sampling_down_factor;
+			policy_dbs->rate_mult = dbs_data->sampling_down_factor;
 		dbs_freq_increase(policy, policy->max);
 	} else {
 		/* Calculate the next frequency proportional to load */
@@ -175,7 +175,7 @@ static void od_update(struct cpufreq_pol
 		freq_next = min_f + load * (max_f - min_f) / 100;
 
 		/* No longer fully busy, reset rate_mult */
-		dbs_info->rate_mult = 1;
+		policy_dbs->rate_mult = 1;
 
 		if (!od_tuners->powersave_bias) {
 			__cpufreq_driver_target(policy, freq_next,
@@ -214,7 +214,7 @@ static unsigned int od_dbs_timer(struct
 			delay = dbs_info->freq_hi_jiffies;
 		} else {
 			delay = delay_for_sampling_rate(dbs_data->sampling_rate
-							* dbs_info->rate_mult);
+							* policy_dbs->rate_mult);
 		}
 	}
 
@@ -266,20 +266,27 @@ static ssize_t store_up_threshold(struct
 static ssize_t store_sampling_down_factor(struct dbs_data *dbs_data,
 		const char *buf, size_t count)
 {
-	unsigned int input, j;
+	struct policy_dbs_info *policy_dbs;
+	unsigned int input;
 	int ret;
 	ret = sscanf(buf, "%u", &input);
 
 	if (ret != 1 || input > MAX_SAMPLING_DOWN_FACTOR || input < 1)
 		return -EINVAL;
+
 	dbs_data->sampling_down_factor = input;
 
 	/* Reset down sampling multiplier in case it was active */
-	for_each_online_cpu(j) {
-		struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info,
-				j);
-		dbs_info->rate_mult = 1;
+	list_for_each_entry(policy_dbs, &dbs_data->policy_dbs_list, list) {
+		/*
+		 * Doing this without locking might lead to using different
+		 * rate_mult values in od_update() and od_dbs_timer().
+		 */
+		mutex_lock(&policy_dbs->timer_mutex);
+		policy_dbs->rate_mult = 1;
+		mutex_unlock(&policy_dbs->timer_mutex);
 	}
+
 	return count;
 }
 

[toc] | [prev] | [next] | [standalone]

#1334291 — Re: [PATCH 7/9] cpufreq: governor: Move rate_mult to struct policy_dbs

On 15-02-16, 02:20, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The rate_mult field in struct od_cpu_dbs_info_s is used by the code
> shared with the conservative governor and to access it that code
> has to do an ugly governor type check.  However, first of all it
> is ever only used for policy->cpu, so it is per-policy rather than
> per-CPU and second, it is initialized to 1 by cpufreq_governor_start(),
> so if the conservative governor never modifies it, it will have no
> effect on the results of any computations.
> 
> For these reasons, move rate_mult to struct policy_dbs_info (as a
> common field).
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   25 +++++++++----------------
>  drivers/cpufreq/cpufreq_governor.h |    3 ++-
>  drivers/cpufreq/cpufreq_ondemand.c |   23 +++++++++++++++--------
>  3 files changed, 26 insertions(+), 25 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1334091 — [PATCH 3/9] cpufreq: governor: Fix nice contribution computation in dbs_check_cpu()

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The contribution of the CPU nice time to the idle time in dbs_check_cpu()
is computed in a bogus way, as the code may subtract current and previous
nice values for different CPUs.

That doesn't matter for cases when cpufreq policies are not shared,
but may lead to problems otherwise.

Fix the computation and simplify it to avoid taking unnecessary steps.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_governor.c |   18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -198,22 +198,10 @@ void dbs_check_cpu(struct cpufreq_policy
 		j_cdbs->prev_cpu_idle = cur_idle_time;
 
 		if (ignore_nice) {
-			struct cpu_dbs_info *cdbs = gov->get_cpu_cdbs(cpu);
-			u64 cur_nice;
-			unsigned long cur_nice_jiffies;
+			u64 cur_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE];
 
-			cur_nice = kcpustat_cpu(j).cpustat[CPUTIME_NICE] -
-					 cdbs->prev_cpu_nice;
-			/*
-			 * Assumption: nice time between sampling periods will
-			 * be less than 2^32 jiffies for 32 bit sys
-			 */
-			cur_nice_jiffies = (unsigned long)
-					cputime64_to_jiffies64(cur_nice);
-
-			cdbs->prev_cpu_nice =
-				kcpustat_cpu(j).cpustat[CPUTIME_NICE];
-			idle_time += jiffies_to_usecs(cur_nice_jiffies);
+			idle_time += cputime_to_usecs(cur_nice - j_cdbs->prev_cpu_nice);
+			j_cdbs->prev_cpu_nice = cur_nice;
 		}
 
 		if (unlikely(!wall_time || wall_time < idle_time))

[toc] | [prev] | [next] | [standalone]

#1334231 — Re: [PATCH 3/9] cpufreq: governor: Fix nice contribution computation in dbs_check_cpu()

On 15-02-16, 02:15, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The contribution of the CPU nice time to the idle time in dbs_check_cpu()
> is computed in a bogus way, as the code may subtract current and previous
> nice values for different CPUs.
> 
> That doesn't matter for cases when cpufreq policies are not shared,
> but may lead to problems otherwise.
> 
> Fix the computation and simplify it to avoid taking unnecessary steps.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_governor.c |   18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1334092 — [PATCH 9/9] cpufreq: governor: Use microseconds in sample delay computations

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Do not convert microseconds to jiffies and the other way around
in governor computations related to the sampling rate and sample
delay and drop delay_for_sampling_rate() which isn't of any use
then.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_conservative.c |    2 +-
 drivers/cpufreq/cpufreq_governor.c     |    4 +---
 drivers/cpufreq/cpufreq_governor.h     |   15 ++-------------
 drivers/cpufreq/cpufreq_ondemand.c     |   28 +++++++++++++---------------
 4 files changed, 17 insertions(+), 32 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_governor.h
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.h
+++ linux-pm/drivers/cpufreq/cpufreq_governor.h
@@ -163,8 +163,8 @@ struct od_cpu_dbs_info_s {
 	struct cpu_dbs_info cdbs;
 	struct cpufreq_frequency_table *freq_table;
 	unsigned int freq_lo;
-	unsigned int freq_lo_jiffies;
-	unsigned int freq_hi_jiffies;
+	unsigned int freq_lo_delay_us;
+	unsigned int freq_hi_delay_us;
 	unsigned int sample_type:1;
 };
 
@@ -223,17 +223,6 @@ struct od_ops {
 	void (*freq_increase)(struct cpufreq_policy *policy, unsigned int freq);
 };
 
-static inline int delay_for_sampling_rate(unsigned int sampling_rate)
-{
-	int delay = usecs_to_jiffies(sampling_rate);
-
-	/* We want all CPUs to do sampling nearly on same jiffy */
-	if (num_online_cpus() > 1)
-		delay -= jiffies % delay;
-
-	return delay;
-}
-
 extern struct mutex dbs_data_mutex;
 unsigned int dbs_update(struct cpufreq_policy *policy);
 int cpufreq_governor_dbs(struct cpufreq_policy *policy, unsigned int event);
Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -102,7 +102,7 @@ static unsigned int cs_dbs_timer(struct
 	}
 
  out:
-	return delay_for_sampling_rate(dbs_data->sampling_rate);
+	return dbs_data->sampling_rate;
 }
 
 static int dbs_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -66,8 +66,8 @@ static int should_io_be_busy(void)
 
 /*
  * Find right freq to be set now with powersave_bias on.
- * Returns the freq_hi to be used right now and will set freq_hi_jiffies,
- * freq_lo, and freq_lo_jiffies in percpu area for averaging freqs.
+ * Returns the freq_hi to be used right now and will set freq_hi_delay_us,
+ * freq_lo, and freq_lo_delay_us in percpu area for averaging freqs.
  */
 static unsigned int generic_powersave_bias_target(struct cpufreq_policy *policy,
 		unsigned int freq_next, unsigned int relation)
@@ -75,7 +75,7 @@ static unsigned int generic_powersave_bi
 	unsigned int freq_req, freq_reduc, freq_avg;
 	unsigned int freq_hi, freq_lo;
 	unsigned int index = 0;
-	unsigned int jiffies_total, jiffies_hi, jiffies_lo;
+	unsigned int delay_hi_us;
 	struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info,
 						   policy->cpu);
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
@@ -84,7 +84,7 @@ static unsigned int generic_powersave_bi
 
 	if (!dbs_info->freq_table) {
 		dbs_info->freq_lo = 0;
-		dbs_info->freq_lo_jiffies = 0;
+		dbs_info->freq_lo_delay_us = 0;
 		return freq_next;
 	}
 
@@ -107,17 +107,15 @@ static unsigned int generic_powersave_bi
 	/* Find out how long we have to be in hi and lo freqs */
 	if (freq_hi == freq_lo) {
 		dbs_info->freq_lo = 0;
-		dbs_info->freq_lo_jiffies = 0;
+		dbs_info->freq_lo_delay_us = 0;
 		return freq_lo;
 	}
-	jiffies_total = usecs_to_jiffies(dbs_data->sampling_rate);
-	jiffies_hi = (freq_avg - freq_lo) * jiffies_total;
-	jiffies_hi += ((freq_hi - freq_lo) / 2);
-	jiffies_hi /= (freq_hi - freq_lo);
-	jiffies_lo = jiffies_total - jiffies_hi;
+	delay_hi_us = (freq_avg - freq_lo) * dbs_data->sampling_rate;
+	delay_hi_us += (freq_hi - freq_lo) / 2;
+	delay_hi_us /= freq_hi - freq_lo;
+	dbs_info->freq_hi_delay_us = delay_hi_us;
 	dbs_info->freq_lo = freq_lo;
-	dbs_info->freq_lo_jiffies = jiffies_lo;
-	dbs_info->freq_hi_jiffies = jiffies_hi;
+	dbs_info->freq_lo_delay_us = dbs_data->sampling_rate - delay_hi_us;
 	return freq_hi;
 }
 
@@ -205,7 +203,7 @@ static unsigned int od_dbs_timer(struct
 	if (sample_type == OD_SUB_SAMPLE && policy_dbs->sample_delay_ns > 0) {
 		__cpufreq_driver_target(policy, dbs_info->freq_lo,
 					CPUFREQ_RELATION_H);
-		return dbs_info->freq_lo_jiffies;
+		return dbs_info->freq_lo_delay_us;
 	}
 
 	od_update(policy);
@@ -213,10 +211,10 @@ static unsigned int od_dbs_timer(struct
 	if (dbs_info->freq_lo) {
 		/* Setup timer for SUB_SAMPLE */
 		dbs_info->sample_type = OD_SUB_SAMPLE;
-		return dbs_info->freq_hi_jiffies;
+		return dbs_info->freq_hi_delay_us;
 	}
 
-	return delay_for_sampling_rate(dbs_data->sampling_rate * policy_dbs->rate_mult);
+	return dbs_data->sampling_rate * policy_dbs->rate_mult;
 }
 
 /************************** sysfs interface ************************/
Index: linux-pm/drivers/cpufreq/cpufreq_governor.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c
+++ linux-pm/drivers/cpufreq/cpufreq_governor.c
@@ -282,7 +282,6 @@ static void dbs_work_handler(struct work
 	struct policy_dbs_info *policy_dbs;
 	struct cpufreq_policy *policy;
 	struct dbs_governor *gov;
-	unsigned int delay;
 
 	policy_dbs = container_of(work, struct policy_dbs_info, work);
 	policy = policy_dbs->policy;
@@ -293,8 +292,7 @@ static void dbs_work_handler(struct work
 	 * ondemand governor isn't updating the sampling rate in parallel.
 	 */
 	mutex_lock(&policy_dbs->timer_mutex);
-	delay = gov->gov_dbs_timer(policy);
-	policy_dbs->sample_delay_ns = jiffies_to_nsecs(delay);
+	gov_update_sample_delay(policy_dbs, gov->gov_dbs_timer(policy));
 	mutex_unlock(&policy_dbs->timer_mutex);
 
 	/* Allow the utilization update handler to queue up more work. */

[toc] | [prev] | [next] | [standalone]

#1334293 — Re: [PATCH 9/9] cpufreq: governor: Use microseconds in sample delay computations

On 15-02-16, 02:22, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Do not convert microseconds to jiffies and the other way around
> in governor computations related to the sampling rate and sample
> delay and drop delay_for_sampling_rate() which isn't of any use
> then.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |    2 +-
>  drivers/cpufreq/cpufreq_governor.c     |    4 +---
>  drivers/cpufreq/cpufreq_governor.h     |   15 ++-------------
>  drivers/cpufreq/cpufreq_ondemand.c     |   28 +++++++++++++---------------
>  4 files changed, 17 insertions(+), 32 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1334093 — [PATCH 8/9] cpufreq: ondemand: Simplify conditionals in od_dbs_timer()

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reduce the indentation level in the conditionals in od_dbs_timer()
and drop the delay variable from it.

No functional changes.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_ondemand.c |   24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_ondemand.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_ondemand.c
+++ linux-pm/drivers/cpufreq/cpufreq_ondemand.c
@@ -194,7 +194,7 @@ static unsigned int od_dbs_timer(struct
 	struct policy_dbs_info *policy_dbs = policy->governor_data;
 	struct dbs_data *dbs_data = policy_dbs->dbs_data;
 	struct od_cpu_dbs_info_s *dbs_info = &per_cpu(od_cpu_dbs_info, policy->cpu);
-	int delay, sample_type = dbs_info->sample_type;
+	int sample_type = dbs_info->sample_type;
 
 	/* Common NORMAL_SAMPLE setup */
 	dbs_info->sample_type = OD_NORMAL_SAMPLE;
@@ -203,22 +203,20 @@ static unsigned int od_dbs_timer(struct
 	 * it then.
 	 */
 	if (sample_type == OD_SUB_SAMPLE && policy_dbs->sample_delay_ns > 0) {
-		delay = dbs_info->freq_lo_jiffies;
 		__cpufreq_driver_target(policy, dbs_info->freq_lo,
 					CPUFREQ_RELATION_H);
-	} else {
-		od_update(policy);
-		if (dbs_info->freq_lo) {
-			/* Setup timer for SUB_SAMPLE */
-			dbs_info->sample_type = OD_SUB_SAMPLE;
-			delay = dbs_info->freq_hi_jiffies;
-		} else {
-			delay = delay_for_sampling_rate(dbs_data->sampling_rate
-							* policy_dbs->rate_mult);
-		}
+		return dbs_info->freq_lo_jiffies;
 	}
 
-	return delay;
+	od_update(policy);
+
+	if (dbs_info->freq_lo) {
+		/* Setup timer for SUB_SAMPLE */
+		dbs_info->sample_type = OD_SUB_SAMPLE;
+		return dbs_info->freq_hi_jiffies;
+	}
+
+	return delay_for_sampling_rate(dbs_data->sampling_rate * policy_dbs->rate_mult);
 }
 
 /************************** sysfs interface ************************/

[toc] | [prev] | [next] | [standalone]

#1334286 — Re: [PATCH 8/9] cpufreq: ondemand: Simplify conditionals in od_dbs_timer()

On 15-02-16, 02:21, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Reduce the indentation level in the conditionals in od_dbs_timer()
> and drop the delay variable from it.
> 
> No functional changes.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_ondemand.c |   24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web