[v3,2/5] sched: Take cpufreq feedback into account

Aggregate the different pressures applied on the capacity of CPUs and
create a new function that returns the actual capacity of the CPU:
  get_actual_cpu_capacity()

Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org>
Reviewed-by: Lukasz Luba <lukasz.luba@arm.com>
---
 kernel/sched/fair.c | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

On 08/01/2024 14:48, Vincent Guittot wrote:
> Aggregate the different pressures applied on the capacity of CPUs and
> create a new function that returns the actual capacity of the CPU:
>   get_actual_cpu_capacity()

   function name                scaling

(1) arch_scale_cpu_capacity() -	uarch

(2) get_actual_cpu_capacity() -	hw + cpufreq/thermal of (1)

(3) capacity_of()	      -	rt (rt/dl/irq) of (2) (used by fair)

Although (1) - (3) are very close to each other from the functional
standpoint, their names are not very coherent.

I assume this makes it hard to understand all of this when reading the
code w/o knowing these patches before.

Why is (2) tagged with 'actual'?

This is especially visible in feec() where local variable cpu_cap
relates to (3) whereas cpu_actual_cap related to (2).

[...]

On Tue, 9 Jan 2024 at 12:22, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> On 08/01/2024 14:48, Vincent Guittot wrote:
> > Aggregate the different pressures applied on the capacity of CPUs and
> > create a new function that returns the actual capacity of the CPU:
> >   get_actual_cpu_capacity()
>
>    function name                scaling
>
> (1) arch_scale_cpu_capacity() - uarch
>
> (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1)
>
> (3) capacity_of()             - rt (rt/dl/irq) of (2) (used by fair)
>
> Although (1) - (3) are very close to each other from the functional

I don't get your point as name of (1) and (3) have not been changed by the patch

> standpoint, their names are not very coherent.
>
> I assume this makes it hard to understand all of this when reading the
> code w/o knowing these patches before.
>
> Why is (2) tagged with 'actual'?

This is the actual max compute capacity of the cpu at now  i.e.
possibly reduced because of temporary frequency capping

So (2) equals (1) minus temporary performance capping and (3)
additionally subtracts the time used by other class to (2)


>
> This is especially visible in feec() where local variable cpu_cap
> relates to (3) whereas cpu_actual_cap related to (2).
>
> [...]
>

On 09/01/2024 15:30, Vincent Guittot wrote:
> On Tue, 9 Jan 2024 at 12:22, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>>
>> On 08/01/2024 14:48, Vincent Guittot wrote:
>>> Aggregate the different pressures applied on the capacity of CPUs and
>>> create a new function that returns the actual capacity of the CPU:
>>>   get_actual_cpu_capacity()
>>
>>    function name                scaling
>>
>> (1) arch_scale_cpu_capacity() - uarch
>>
>> (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1)
>>
>> (3) capacity_of()             - rt (rt/dl/irq) of (2) (used by fair)
>>
>> Although (1) - (3) are very close to each other from the functional
> 
> I don't get your point as name of (1) and (3) have not been changed by the patch

That's true. But with capacity_orig_of() for (1), we had some coherence
in the naming scheme of those cpu_capacity related functions (1) - (3).
which helps when trying to understand the code.

I can see that actual_capacity_of() (2) sounds awful though.

>> standpoint, their names are not very coherent.
>>
>> I assume this makes it hard to understand all of this when reading the
>> code w/o knowing these patches before.
>>
>> Why is (2) tagged with 'actual'?
> 
> This is the actual max compute capacity of the cpu at now  i.e.
> possibly reduced because of temporary frequency capping

Will the actual max compute capacity also depend on 'user space system
pressure' later, i.e. on 'permanent' frequency capping?

> So (2) equals (1) minus temporary performance capping and (3)
> additionally subtracts the time used by other class to (2)

OK.

A coherent set of those tags even reflected in those getters would help
but can be done later too.

On Wed, 10 Jan 2024 at 14:51, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
>
> On 09/01/2024 15:30, Vincent Guittot wrote:
> > On Tue, 9 Jan 2024 at 12:22, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote:
> >>
> >> On 08/01/2024 14:48, Vincent Guittot wrote:
> >>> Aggregate the different pressures applied on the capacity of CPUs and
> >>> create a new function that returns the actual capacity of the CPU:
> >>>   get_actual_cpu_capacity()
> >>
> >>    function name                scaling
> >>
> >> (1) arch_scale_cpu_capacity() - uarch
> >>
> >> (2) get_actual_cpu_capacity() - hw + cpufreq/thermal of (1)
> >>
> >> (3) capacity_of()             - rt (rt/dl/irq) of (2) (used by fair)
> >>
> >> Although (1) - (3) are very close to each other from the functional
> >
> > I don't get your point as name of (1) and (3) have not been changed by the patch
>
> That's true. But with capacity_orig_of() for (1), we had some coherence
> in the naming scheme of those cpu_capacity related functions (1) - (3).
> which helps when trying to understand the code.
>
> I can see that actual_capacity_of() (2) sounds awful though.
>
> >> standpoint, their names are not very coherent.
> >>
> >> I assume this makes it hard to understand all of this when reading the
> >> code w/o knowing these patches before.
> >>
> >> Why is (2) tagged with 'actual'?
> >
> > This is the actual max compute capacity of the cpu at now  i.e.
> > possibly reduced because of temporary frequency capping
>
> Will the actual max compute capacity also depend on 'user space system
> pressure' later, i.e. on 'permanent' frequency capping?

yes it will


>
> > So (2) equals (1) minus temporary performance capping and (3)
> > additionally subtracts the time used by other class to (2)
>
> OK.
>
> A coherent set of those tags even reflected in those getters would help
> but can be done later too.