linux-kernel - Re: [PATCH v9 6/7] sched/fair: Remove task_util from effective utilization in feec()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 6 Jun 2022 10:41:36 +0100
From:   Vincent Donnefort <vdonnefort@...gle.com>
To:     Vincent Guittot <vincent.guittot@...aro.org>
Cc:     peterz@...radead.org, mingo@...hat.com,
        linux-kernel@...r.kernel.org, dietmar.eggemann@....com,
        morten.rasmussen@....com, chris.redpath@....com,
        qperret@...gle.com, tao.zhou@...ux.dev, kernel-team@...roid.com,
        Vincent Donnefort <vincent.donnefort@....com>
Subject: Re: [PATCH v9 6/7] sched/fair: Remove task_util from effective
 utilization in feec()

[...]

> > +
> > +/*
> > + * 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
> >