Re: [PATCH] sched/fair: Prevent cpu_busy_time from exceeding actual_cpu_capacity

[Qais Yousef <qyousef@layalina.io>]

On 06/12/24 16:11, Xuewen Yan wrote:
> Hi Qais
> 
> On Mon, Jun 10, 2024 at 6:55 AM Qais Yousef <qyousef@layalina.io> wrote:
> >
> > On 06/06/24 15:06, Xuewen Yan wrote:
> > > Because the effective_cpu_util() would return a util which
> > > maybe bigger than the actual_cpu_capacity, this could cause
> > > the pd_busy_time calculation errors.
> > > So clamp the cpu_busy_time with the eenv->cpu_cap, which is
> > > the actual_cpu_capacity.
> >
> > I actually think capping by pd_cap is something we should remove. Saturated
> > systems aren't calculated properly especially when uclamp_max is used.
> >
> > But this might a bigger change and out of scope of what you're proposing..
> 
> I agree, there are other things to consider before doing this.
> 
> >
> > Did this 'wrong' calculation cause an actual problem for task placement?
> > I assume the pd looked 'busier' because some CPUs were too busy.
> 
> This will not only affect calculations in scenarios with high temperatures.
> Sometimes, users will set scalimg_max_freq to actively limit the CPU frequency,
> so that even if the CPU load is large, the CPU frequency will not be very high.
> At this time, even if tasks are placed on other CPUs in the same PD,
> the energy increment may not be too large, thus affecting core selection.
> 
> >
> > Was the system in overutilzied state? Usually if one CPU is that busy
> > overutilized should be set and we'd skip EAS - unless uclamp_max was used.
> 
> As Christian said, This case occurs not only in the overutil scenario,
> this scenario holds true as long as the actual-cpu-capacity caused by
> the reduction in max cpu frequency is smaller than the cpu util.

Hmm. Sorry I might be thick here, but shouldn't fits_capacity() use
capacity_of() which should return capacity based on get_actual_cpu_capacity()
to compare if we are overutilized? If we are higher than this value that you
need to cap, then the CPU must be overutilized and we shouldn't be in feec() in
the first place, no? Unless the rq is capped with uclamp_max that is.

I generally think our current definition of overutilized is wrong and the use
case you're talking about should hold true if it's just a single CPU that is
overutilized. But I can't see how you end up in feec() if the util is higher
than the CPU in our current code base.

What did I miss?

And should effective_cpu_util() return a value higher than
get_actual_cpu_capacity()?

diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
index 8b1194c39161..91acc0f92ae4 100644
--- a/kernel/sched/syscalls.c
+++ b/kernel/sched/syscalls.c
@@ -286,7 +286,7 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs,
        unsigned long util, irq, scale;
        struct rq *rq = cpu_rq(cpu);

-       scale = arch_scale_cpu_capacity(cpu);
+       scale = get_actual_cpu_capacity(cpu);

        /*
         * Early check to see if IRQ/steal time saturates the CPU, can be