Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220523155140.2878563-1-vdonnefort@google.com>
 <20220523155140.2878563-7-vdonnefort@google.com> <CAKfTPtA9qTCteP+VfR_9qnQkoxq=k3ACGrdPiKmc-sox0vXmgw@mail.gmail.com>
 <Yp3L0EwERCp3iAYg@google.com>
In-Reply-To: <Yp3L0EwERCp3iAYg@google.com>
From:   Vincent Guittot <vincent.guittot@linaro.org>
Date:   Tue, 7 Jun 2022 08:59:50 +0200
Message-ID: <CAKfTPtCQLNhhNgXVYi0NAVpjmmy1duV9PW5kq16BPYSd2gtfXQ@mail.gmail.com>
Subject: Re: [PATCH v9 6/7] sched/fair: Remove task_util from effective
 utilization in feec()
To:     Vincent Donnefort <vdonnefort@google.com>
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 6 Jun 2022 at 11:41, Vincent Donnefort <vdonnefort@google.com> wrote:
>
> [...]
>
> > > +
> > > +/*
> > > + * 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().

Ok. A comment would be helpful

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