Messages in this thread | | | Date | Tue, 29 Aug 2023 17:37:40 +0100 | From | Qais Yousef <> | Subject | Re: [PATCH 2/4] sched: cpufreq: Fix apply_dvfs_headroom() escaping uclamp constraints |
| |
On 08/29/23 16:35, Vincent Guittot wrote: > On Sun, 20 Aug 2023 at 23:08, Qais Yousef <qyousef@layalina.io> wrote: > > > > DVFS headroom is applied after we calculate the effective_cpu_util() > > which is where we honour uclamp constraints. It makes more sense to > > apply the headroom there once and let all users naturally get the right > > thing without having to sprinkle the call around in various places. > > You have to take care of not mixing scheduler and cpufreq constraint and policy. > > uclamp is a scheduler feature to highlight that the utilization > requirement of a task can't go above a level.
uclamp is a performance hint, which utilization is how we represent it internally. A task with uclamp of 800 is not the same as util being actually 800. In our previous discussions around util_fits_cpu() we had similar discussion on how the two can't be treated the same.
Same with uclamp_min; if it is set to 1024 but there is a task with util say 100, this task shouldn't cause overutilized as its actual utilization actually fits, but it just asked to run at a higher performance point. The actual utilization has to be in the over utilized range. Unless uclamp_max forces it to fit of course.
So uclamp_max sets a cap to the performance that the task is allowed to reach. And this translates into frequency selection and cpu selection (in case of HMP) by design.
I don't think we're mixing matters here. But the headroom should be applied to actual utilization, not uclamp. If the rq is capped to a specific performance level, we should honour this.
We do the same with iowait boost where it is capped by uclamp_max. A task capped by uclamp_max shouldn't be the trigger of running at a frequency higher than this point. Otherwise we'd need to expose all these internal details to the user, which I think we all agree isn't something to consider at all.
> > dvfs head room is a cpufreq decision to anticipate either hw > limitation and responsiveness problem or performance hints. > > they come from different sources and rational and this patch mixed > them which i'm not sure is a correct thing to do
I don't think I'm mixing them up to be honest.
The governor is driven by effective_cpu_util() to tell it what is the effective_cpu_util() when making frequency selection. This function takes into account all the factors that could impact frequency selection including all type of rq pressures (except thermal). I think it is appropriate to take headroom into account there and make sure we honour uclamp_max hint to cap the performance appropriately based on the effective uclamp_max value of the rq.
For example if actually util was 640, then after the headroom it'd be 800. And if uclamp_max is 800, then this task will still get the 1.25 headroom. We are not changing this behavior.
But if the task goes beyond that, then it'll stop at 800 because this what the request is all about. A headroom beyond this point is not required because the task (or rq to be more precise) is capped to this performance point and regardless how busy the cpu gets in terms of real utilization or headroom, it shouldn't go beyond this point. ie: if a task is a 1024 but uclamp_max of is 800 then it'll only get a performance equivalent to OPP@800 would give.
If we don't do that uclamp_max range effectively is from 0-819 (based on current headroom setting of 1.25). Which is not what we want or what we're telling userspace. Userspace sees the whole system performance levels abstracted from 0 - 1024. As it should. The only effect they would observe and there's no way around it is that OPPs are discrete points. So in reality our 0-1024 is a staircase where a range of util values will map to the same OPP and then we'll get a jump. So the user can end up requesting for example 700 and 720 and not notice any difference because they both map to the same OPP. I don't think we can fix that - but maybe I should add it to the uclamp doc as a caveat when setting uclamp.
> > > > > Before this fix running > > > > uclampset -M 800 cat /dev/zero > /dev/null > > > > Will cause the test system to run at max freq of 2.8GHz. After the fix > > it runs at 2.2GHz instead which is the correct value that matches the > > capacity of 800. > > So a task with an utilization of 800 will run at higher freq than a > task clamped to 800 by uclamp ? Why should they run at different freq > for the same utilization ?
Because uclamp sets an upper limit on the performance level the task should be able to achieve. Imagine a task is 1024 and capped to 800, it should not run at max frequency, right? What's the point of the uclamp_max hint if the headroom will cause it to run at max anyway? We lost the meaning of the hint. And if this headroom changes in the future, people will start observing different behavior for existing uclamp_max settings on the same system because of this this rightfully hidden and unaccounted for factor.
> > > > > 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. > > > > Signed-off-by: Qais Yousef (Google) <qyousef@layalina.io> > > --- > > include/linux/energy_model.h | 1 - > > kernel/sched/core.c | 11 ++++++++--- > > kernel/sched/cpufreq_schedutil.c | 5 ++--- > > 3 files changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h > > index 6ebde4e69e98..adec808b371a 100644 > > --- a/include/linux/energy_model.h > > +++ b/include/linux/energy_model.h > > @@ -243,7 +243,6 @@ static inline unsigned long em_cpu_energy(struct em_perf_domain *pd, > > scale_cpu = arch_scale_cpu_capacity(cpu); > > ps = &pd->table[pd->nr_perf_states - 1]; > > > > - max_util = apply_dvfs_headroom(max_util); > > max_util = min(max_util, allowed_cpu_cap); > > freq = map_util_freq(max_util, ps->frequency, scale_cpu); > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index efe3848978a0..441d433c83cd 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -7439,8 +7439,10 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs, > > * frequency will be gracefully reduced with the utilization decay. > > */ > > util = util_cfs + cpu_util_rt(rq); > > - if (type == FREQUENCY_UTIL) > > + if (type == FREQUENCY_UTIL) { > > + util = apply_dvfs_headroom(util); > > This is not the same as before because utilization has not being > scaled by irq steal time yet
We do the scaling below, no?
AFAICS, we had:
(util_cfs + util_rt + irq + ((max-irq)*(util_cfs + util_rt)/max)+ dl_bw) * scale
Using U = (util_cfs + util_rt) * scale
we can write this after the multiplication
U + irq * scale + ((max-irq)*U/max) + dl_bw * scale
> > > util = uclamp_rq_util_with(rq, util, p); > > + } > > > > dl_util = cpu_util_dl(rq); > > > > @@ -7471,9 +7473,12 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs, > > * max - irq > > * U' = irq + --------- * U > > * max > > + * > > + * We only need to apply dvfs headroom to irq part since the util part > > + * already had it applied. > > */ > > util = scale_irq_capacity(util, irq, max); > > - util += irq; > > + util += type == FREQUENCY_UTIL ? apply_dvfs_headroom(irq) : irq; > > > > /* > > * Bandwidth required by DEADLINE must always be granted while, for > > @@ -7486,7 +7491,7 @@ unsigned long effective_cpu_util(int cpu, unsigned long util_cfs, > > * an interface. So, we only do the latter for now. > > */ > > if (type == FREQUENCY_UTIL) > > - util += cpu_bw_dl(rq); > > + util += apply_dvfs_headroom(cpu_bw_dl(rq)); > > If we follow your reasoning with uclamp on the dl bandwidth, should we > not skip this as well ?
I do remove this in patch 4. Can fold that one into this one if you like. I opted to keep the current behavior in this patch and remove these later in patch 4.
I do think that both RT and DL shouldn't need DVFS headroom in general as they both 'disable' it by default.
Thanks!
-- Qais Yousef
| |