Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Mon, 6 Jun 2022 10:41:36 +0100
From:   Vincent Donnefort <vdonnefort@google.com>
To:     Vincent Guittot <vincent.guittot@linaro.org>
Cc:     peterz@infradead.org, mingo@redhat.com,
        linux-kernel@vger.kernel.org, dietmar.eggemann@arm.com,
        morten.rasmussen@arm.com, chris.redpath@arm.com,
        qperret@google.com, tao.zhou@linux.dev, kernel-team@android.com,
        Vincent Donnefort <vincent.donnefort@arm.com>
Subject: Re: [PATCH v9 6/7] sched/fair: Remove task_util from effective
 utilization in feec()
Message-ID: <Yp3L0EwERCp3iAYg@google.com>
References: <20220523155140.2878563-1-vdonnefort@google.com>
 <20220523155140.2878563-7-vdonnefort@google.com>
 <CAKfTPtA9qTCteP+VfR_9qnQkoxq=k3ACGrdPiKmc-sox0vXmgw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKfTPtA9qTCteP+VfR_9qnQkoxq=k3ACGrdPiKmc-sox0vXmgw@mail.gmail.com>
Precedence: bulk

[...]

> > +
> > +/*
> > + * compute_energy(): Use the Energy Model to estimate the energy that @pd would
> > + * consume for a given utilization landscape @eenv. If @dst_cpu < 0 the task
> 
> I find this comment a bit confusing because compute_energy() adds the
> task contribution if dst_cpu >= 0 but doesn't remove it. The fact that
> eenv->pd_busy_time has been previously computed without the
> contribution of the task, is outside the scope of this this function
> whereas the comment suggest that the remove will happen in
> compute_energy()

Arg, leftover from a previous version where this function was adding or removing
the contribution. I'll update!

> 
> > + * contribution is removed from the energy estimation.
> > + */
> > +static inline unsigned long
> > +compute_energy(struct energy_env *eenv, struct perf_domain *pd,
> > +              struct cpumask *pd_cpus, struct task_struct *p, int dst_cpu)
> > +{
> > +       unsigned long max_util = eenv_pd_max_util(eenv, pd_cpus, p, dst_cpu);
> > +       unsigned long busy_time = eenv->pd_busy_time;
> > +
> > +       if (dst_cpu >= 0)
> > +               busy_time = min(eenv->pd_cap, busy_time + eenv->task_busy_time);
> > +
> > +       return em_cpu_energy(pd->em_pd, max_util, busy_time, eenv->cpu_cap);
> >  }
> >

[...]

> > @@ -6878,13 +6947,15 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
> >                 if (max_spare_cap_cpu < 0 && !compute_prev_delta)
> >                         continue;
> >
> > +               eenv_pd_busy_time(&eenv, cpus, p);
> >                 /* Compute the 'base' energy of the pd, without @p */
> > -               base_energy_pd = compute_energy(p, -1, cpus, pd);
> > +               base_energy_pd = compute_energy(&eenv, pd, cpus, p, -1);
> >                 base_energy += base_energy_pd;
> >
> >                 /* Evaluate the energy impact of using prev_cpu. */
> >                 if (compute_prev_delta) {
> > -                       prev_delta = compute_energy(p, prev_cpu, cpus, pd);
> > +                       prev_delta = compute_energy(&eenv, pd, cpus, p,
> > +                                                   prev_cpu);
> >                         if (prev_delta < base_energy_pd)
> 
> side question:
> -base_energy_pd is the energy for the perf domain without task p
> -prev_delta is the energy for the same perf domain if task p is put on dst_cpu
> 
> How can prev_delta be lower than base_energy ?

It can happen if one of the CPU utilization is updated in the middle of feec().

> 
> if dst_cpu doesn't belong to the perf domain, prev_delta should be
> equal to base_energy_pd
> if dst_cpu belongs to the perf domain, the compute_energy should be
> higher because the busy_time will be higher
> 
> >                                 goto unlock;
> >                         prev_delta -= base_energy_pd;
> > @@ -6893,8 +6964,8 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
> >
> >                 /* Evaluate the energy impact of using max_spare_cap_cpu. */
> >                 if (max_spare_cap_cpu >= 0) {
> > -                       cur_delta = compute_energy(p, max_spare_cap_cpu, cpus,
> > -                                                  pd);
> > +                       cur_delta = compute_energy(&eenv, pd, cpus, p,
> > +                                                  max_spare_cap_cpu);
> >                         if (cur_delta < base_energy_pd)
> 
> same question as above
> 
> >                                 goto unlock;
> >                         cur_delta -= base_energy_pd;
> > --
> > 2.36.1.124.g0e6072fb45-goog
> >