Re: [PATCH] [RFC] sched: restrict iowait boost for boosted task only

[Qais Yousef]

On 01/24/20 12:55, Wei Wang wrote:
> > > So I'm pretty sure we *do* want tasks with the default clamps to get iowait
> > > boost'd. What we don't want are background tasks driving up the frequency,
> > > and that should be via uclamp.max (as Quentin is suggesting) rather than
> > > uclamp.min (as is suggested in the patch).
> > >
> > > Now, whether that is overloading the usage of uclamp... I'm not sure.
> > > One of the argument for uclamp was actually frequency selection, so if
> > > we just make iowait boost respect that, IOW not boost further than
> > > uclamp.max (which is a bit better than a simple on/off switch), that
> > > wouldn't be too crazy I think.
> >
> > Capping iowait boost value in schedutil based on uclamp makes sense indeed.
> >
> > What didn't make sense to me is the use of uclamp as a switch to toggle iowait
> > boost on/off.
> 
> Sounds like we all agree on adding a new toggle, so will move forward
> with that then.

Looking more closely at iowait boost, it's not actually a generic cpufreq
attribute. Only schedutil and intel_pstate have it. Other governors might
implement something similar but under a different name.

So I'm not sure how easy it'd be to implement a generic toggle for something
that probably should be considered an implementation detail of a governor and
userspace shouldn't care much about.

Of course, the maintainers might have a different opinion. So don't let mine
discourage you from pursuing this further! :-)

> For capping iowait boost, it should be a seperate patch. I am not sure
> if we want to apply what's the current max clamp on the rq but I do
> see the per-task iowait boost makes sense.

It is true the 2 patches are orthogonal, but if you already cap the max
frequencies the background task can use, by ensuring the iowait_boost in
schedutil respects the uclamp restrictions then this should solve your problem
too, no?

The patch below only compile tested.


	background/cpu.uclamp.max = 200 # Cap background tasks max frequencies

---

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index 9b8916fd00a2..a76c02eecdaf 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -421,7 +421,8 @@ static unsigned long sugov_iowait_apply(struct sugov_cpu *sg_cpu, u64 time,
         * into the same scale so we can compare.
         */
        boost = (sg_cpu->iowait_boost * max) >> SCHED_CAPACITY_SHIFT;
-       return max(boost, util);
+       boost = max(boost, util);
+       return uclamp_util_with(cpu_rq(sg_cpu->cpu), boost, NULL);
 }

 #ifdef CONFIG_NO_HZ_COMMON

--
Qais Yousef