Date:   Wed, 21 Mar 2018 22:15:21 +0100
From:   Dietmar Eggemann <dietmar.eggemann@....com>
To:     Quentin Perret <quentin.perret@....com>,
        Patrick Bellasi <patrick.bellasi@....com>
Cc:     Juri Lelli <juri.lelli@...il.com>, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Thara Gopinath <thara.gopinath@...aro.org>,
        linux-pm@...r.kernel.org,
        Morten Rasmussen <morten.rasmussen@....com>,
        Chris Redpath <chris.redpath@....com>,
        Valentin Schneider <valentin.schneider@....com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Viresh Kumar <viresh.kumar@...aro.org>,
        Todd Kjos <tkjos@...gle.com>,
        Joel Fernandes <joelaf@...gle.com>
Subject: Re: [RFC PATCH 4/6] sched/fair: Introduce an energy estimation helper
 function

On 03/21/2018 03:02 PM, Quentin Perret wrote:
> On Wednesday 21 Mar 2018 at 12:26:21 (+0000), Patrick Bellasi wrote:
>> On 21-Mar 10:04, Juri Lelli wrote:
>>> Hi,
>>>
>>> On 20/03/18 09:43, Dietmar Eggemann wrote:
>>>> From: Quentin Perret <quentin.perret@....com>

[...]

>> Actually I think that this whole function can be written "just" as:
>>
>> ---8<---
>>     unsigned long util = cpu_util_wake(cpu);
>>
>>     if (cpu != dst_cpu)
>>          return util;
>>
>>     return min(util + task_util(p), capacity_orig_of(cpu));
>> ---8<---
>>
> 
> Yes this should be functionally equivalent. However, with your
> suggestion you can potentially remove the task contribution from the
> cpu_util in cpu_util_wake() and then add it back right after if
> cpu==dst_cpu. This is sub-optimal and that's why I implemented things
> slightly differently. But maybe this optimization really is too small to
> justify the extra complexity involved ...

What about we merge both functions by adding an additional 'int dst_cpu'
parameter to cpu_util_wake() (only lightly tested, w/o util_est):

--->8---

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 65a1bead0773..4d4f104d5b3d 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -5860,11 +5860,11 @@ static int wake_affine(struct sched_domain *sd, struct task_struct *p,
 }
 
 static inline unsigned long task_util(struct task_struct *p);
-static unsigned long cpu_util_wake(int cpu, struct task_struct *p);
+static unsigned long cpu_util_wake(int cpu, int dst_cpu, struct task_struct *p);
 
 static unsigned long capacity_spare_wake(int cpu, struct task_struct *p)
 {
-       return max_t(long, capacity_of(cpu) - cpu_util_wake(cpu, p), 0);
+       return max_t(long, capacity_of(cpu) - cpu_util_wake(cpu, -1, p), 0);
 }
 
 /*
@@ -6384,16 +6384,22 @@ static inline unsigned long task_util(struct task_struct *p)
  * cpu_util_wake: Compute CPU utilization with any contributions from
  * the waking task p removed.
  */
-static unsigned long cpu_util_wake(int cpu, struct task_struct *p)
+static unsigned long cpu_util_wake(int cpu, int dst_cpu, struct task_struct *p)
 {
        unsigned long util, capacity;
 
        /* Task has no contribution or is new */
-       if (cpu != task_cpu(p) || !p->se.avg.last_update_time)
+       if ((cpu != task_cpu(p) && cpu != dst_cpu) ||
+           dst_cpu == task_cpu(p) || !p->se.avg.last_update_time)
                return cpu_util(cpu);
 
        capacity = capacity_orig_of(cpu);
-       util = max_t(long, cpu_rq(cpu)->cfs.avg.util_avg - task_util(p), 0);
+       util = cpu_rq(cpu)->cfs.avg.util_avg;
+
+       if (likely(dst_cpu != cpu))
+               util = max_t(long, util - task_util(p), 0);
+       else
+               util += task_util(p);
 
        return (util >= capacity) ? capacity : util;
 }
@@ -6409,30 +6415,6 @@ static inline int cpu_overutilized(int cpu)
 }
 
 /*
- * Returns the util of "cpu" if "p" wakes up on "dst_cpu".
- */
-static unsigned long cpu_util_next(int cpu, struct task_struct *p, int dst_cpu)
-{
-       unsigned long util = cpu_rq(cpu)->cfs.avg.util_avg;
-       unsigned long capacity = capacity_orig_of(cpu);
-
-       /*
-        * If p is where it should be, or if it has no impact on cpu, there is
-        * not much to do.
-        */
-       if ((task_cpu(p) == dst_cpu) || (cpu != task_cpu(p) && cpu != dst_cpu))
-               goto clamp_util;
-
-       if (dst_cpu == cpu)
-               util += task_util(p);
-       else
-               util = max_t(long, util - task_util(p), 0);
-
-clamp_util:
-       return (util >= capacity) ? capacity : util;
-}
-
-/*
  * Disable WAKE_AFFINE in the case where task @p doesn't fit in the
  * capacity of either the waking CPU @cpu or the previous CPU @prev_cpu.
  *
@@ -6488,7 +6470,7 @@ static unsigned long compute_energy(struct task_struct *p, int dst_cpu)
        for_each_freq_domain(fdom) {
                fdom_max_util = 0;
                for_each_cpu_and(cpu, &(fdom->span), cpu_online_mask) {
-                       util = cpu_util_next(cpu, p, dst_cpu);
+                       util = cpu_util_wake(cpu, dst_cpu, p);
                        fdom_max_util = max(util, fdom_max_util);
                }
 
@@ -6506,7 +6488,7 @@ static unsigned long compute_energy(struct task_struct *p, int dst_cpu)
                 * busy time.
                 */
                for_each_cpu_and(cpu, &(fdom->span), cpu_online_mask) {
-                       util = cpu_util_next(cpu, p, dst_cpu);
+                       util = cpu_util_wake(cpu, dst_cpu, p);
                        energy += cs->power * util / cs->cap;
                }
        }

Powered by blists - more mailing lists