Re: [PATCH 2/4] sched: cpufreq: Fix apply_dvfs_headroom() escaping uclamp constraints

[Qais Yousef <qyousef@layalina.io>]

On 09/12/23 16:34, Dietmar Eggemann wrote:

> No that's not what I wanted to say here.
> 
> I wanted to highlight the different treatment of `(1) a task with
> (natural) util = 800` and `(2) a task with uclamp_max = 800`.
> 
> (1) util = 800
> 
> util = (1.25 * 800 * (1024 - irq) / 1024 + ...
> 
>         <-      ->
>         uclamped(cfs+rt+headroom(cfs+rt))
> 
> 
> (2) uclamp_max = 800
> 
> util = (800 * (1024 - irq) / 1024 + ...
> 
>         <->
>         uclamped(cfs+rt+headroom(cfs+rt))
> 
> So for (1) the scheduler would ask for more than in (2).

Yes, which is the intention IMHO. If they behave the same, then uclamp_max is
not doing anything. And really this is the critical region in the system as
this is where the expensive OPPs lie.

> 
> That's essentially the same question which was raised here:
> 
> https://lkml.kernel.org/r/CAKfTPtDY48jpO+b-2KXawzxh-ty+FMKX6YUXioNR7kpgO=ua6Q@mail.gmail.com
> 
> >>> Note that similar problem exist for uclamp_min. If util was 50, and
> >>> uclamp_min is 100. Since we apply_dvfs_headroom() after apply uclamp
> >>> constraints, we'll end up with util of 125 instead of 100. IOW, we get
> >>> boosted twice, first time by uclamp_min, and second time by dvfs
> >>> headroom.
> >>
> >> I see what you want to change here but:
> >>
> >> So far we have `util -> uclamp -> map_util_to_perf()`
> > 
> > :-O
> > 
> > So when I set the system uclamp_max to 800 it will still run at max; and this
> > is normal?!!
> 
> No that's an issue (A) as well. But the diff between (1) and (2) is IMHO a
> new issue introduced by this patch-set.

It is also the problem to fix :-)

> 
> >> which is fine when we see uclamp as an entity which constrains util, not
> >> the util after being mapped to a capacity constraint.
> > 
> > -ENOPARSE.
> 
> What I meant is 'clamping the util' before scheduler hands over to
> 'cpufreq' is fine:
> 
>     util -> uclamp -> map_util_to_perf()
>                
>     scheduler    -->|<-- cpufreq
> 
> I do understand that you guys are already discussing a new
> cpufreq_give_me_freq_for_this_utilization_ctx() between EM and CPUfreq
> in the other thread of this patch to maybe sort out (A).

This will help with abstraction. But behavior wise, I still think that if
a task (or a cgroup, the system) has uclamp_max to 800, then we should not run
faster than this because of dvfs headroom.

If we don't do this then max performance is effectively mapped at anything at
800 and above. And likely less than that.

For example if

	util@OPP[Fmax-1] = 950

then any util value at

	util = 950 * 0.8 = 760

will saturate the system and run at Fmax, because all values from above 950
will map to Fmax, and because of the headroom this becomes all values at 760 or
above.

This mean from user's perspective uclamp_min and uclamp_max is from 0-760 on
that system. But the abstraction we're providing is that the performance range
is from 0-1024.

If uclamp_min is set to 512 and the task has a util of 100, why should we run
at 512*1.25?

Similarly if uclamp_max wants to cap the task/cgroup/system from consuming the
top resources, why it needs the 1.25 headroom? If there are two tasks running
on the CPU and one of them is not capped, then yeah the headroom will be
applied. But if the rq->uclamp[UCLAMP_MAX] = 800, then the vote from freq from
this CPU should not exceed this. I don't see what the headroom is for in this
case.

As another example, do you expect a task that has a util of 1024 but uclamp_max
= 800 to run at max or at an OPP equivalent to 800 which would be 2 or 3 below
Fmax usually? The latter is the expectation from the interface. Users shouldn't
need to know there's a dvfs headroom and cater for that when they set
uclamp_max or uclamp_min.


Thanks!

--
Qais Yousef