[v3,3/6] cpufreq: schedutil: update CFS util only if used

Message ID 20171130114723.29210-4-patrick.bellasi@arm.com
State New
Headers show
Series
  • [v3,1/6] cpufreq: schedutil: reset sg_cpus's flags at IDLE enter
Related show

Commit Message

Patrick Bellasi Nov. 30, 2017, 11:47 a.m.
Currently the utilization of the FAIR class is collected before locking
the policy. Although that should not be a big issue for most cases, we
also don't really know how much latency there can be between the
utilization reading and its usage.

Let's get the FAIR utilization right before its usage to be better in
sync with the current status of a CPU.

Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>

Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Cc: Ingo Molnar <mingo@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pm@vger.kernel.org

---
Changes from v2:
- rebased on v4.15-rc1

Change-Id: I9291a560bcad7db76894e3f0fcdb917511d0479e
---
 kernel/sched/cpufreq_schedutil.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

-- 
2.14.1

Comments

Juri Lelli Nov. 30, 2017, 1:22 p.m. | #1
Hi,

On 30/11/17 11:47, Patrick Bellasi wrote:
> Currently the utilization of the FAIR class is collected before locking

> the policy. Although that should not be a big issue for most cases, we

> also don't really know how much latency there can be between the

> utilization reading and its usage.

> 

> Let's get the FAIR utilization right before its usage to be better in

> sync with the current status of a CPU.

> 

> Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>

> Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

> Cc: Ingo Molnar <mingo@redhat.com>

> Cc: Peter Zijlstra <peterz@infradead.org>

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

> Cc: Viresh Kumar <viresh.kumar@linaro.org>

> Cc: linux-kernel@vger.kernel.org

> Cc: linux-pm@vger.kernel.org

> 

> ---

> Changes from v2:

> - rebased on v4.15-rc1

> 

> Change-Id: I9291a560bcad7db76894e3f0fcdb917511d0479e

> ---

>  kernel/sched/cpufreq_schedutil.c | 3 +--

>  1 file changed, 1 insertion(+), 2 deletions(-)

> 

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> index 448f49de5335..40521d59630b 100644

> --- a/kernel/sched/cpufreq_schedutil.c

> +++ b/kernel/sched/cpufreq_schedutil.c

> @@ -351,10 +351,9 @@ static void sugov_update_shared(struct update_util_data *hook, u64 time,

>  	unsigned int next_f;

>  	bool rt_mode;

>  

> -	sugov_get_util(&util, &max, sg_cpu->cpu);

> -

>  	raw_spin_lock(&sg_policy->update_lock);

>  

> +	sugov_get_util(&util, &max, sg_cpu->cpu);

>  	sg_cpu->util = util;

>  	sg_cpu->max = max;


Patch looks good.

Reviewed-by: Juri Lelli <juri.lelli@redhat.com>


However, not sure $SUBJECT is really in sync with what the patch does?
CFS gets "used" before and after the patch... last paragraph of the
changelog looks more like it. :)

Best,

Juri
Patrick Bellasi Dec. 7, 2017, 2:19 p.m. | #2
On 07-Dec 10:45, Viresh Kumar wrote:
> On 30-11-17, 15:57, Patrick Bellasi wrote:

> > Yes, that's a pretty trivial update with a confusing changelog.

> > 

> > If we think it's worth to keep (and correct as well) I'll update the

> > commit message.

> 

> We also need to update the commit log based on feedback from Vikram on

> V2. Which said that the utilization can't change around the lock here

> as we are within rq lock section, though max can change (maybe). So

> this patch only takes care of locking before reading max.


Ok, right... will do.

Thus you are still of the opinion to keep this patch in the series?

-- 
#include <best/regards.h>

Patrick Bellasi

Patch

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 448f49de5335..40521d59630b 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -351,10 +351,9 @@  static void sugov_update_shared(struct update_util_data *hook, u64 time,
 	unsigned int next_f;
 	bool rt_mode;
 
-	sugov_get_util(&util, &max, sg_cpu->cpu);
-
 	raw_spin_lock(&sg_policy->update_lock);
 
+	sugov_get_util(&util, &max, sg_cpu->cpu);
 	sg_cpu->util = util;
 	sg_cpu->max = max;