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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 25 Jan 2020 23:59:37 +0000
From:   Qais Yousef <qais.yousef@....com>
To:     Wei Wang <wvw@...gle.com>
Cc:     Valentin Schneider <valentin.schneider@....com>,
        Quentin Perret <qperret@...gle.com>,
        Wei Wang <wei.vince.wang@...il.com>, dietmar.eggemann@....com,
        chris.redpath@....com, Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] [RFC] sched: restrict iowait boost for boosted task only

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