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]

Message-ID: <849cc9f0-f4ae-f2b6-8449-f55697928cf5@arm.com>
Date:   Fri, 24 Jan 2020 11:01:45 +0000
From:   Valentin Schneider <valentin.schneider@....com>
To:     Quentin Perret <qperret@...gle.com>,
        Qais Yousef <qais.yousef@....com>
Cc:     Wei Wang <wvw@...gle.com>, 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>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [RFC] sched: restrict iowait boost for boosted task only

On 24/01/2020 09:51, Quentin Perret wrote:
>>> +static inline bool iowait_boosted(struct task_struct *p)
>>> +{
>>> +	return p->in_iowait && uclamp_eff_value(p, UCLAMP_MIN) > 0;
>>
>> I think this is overloading the usage of util clamp. You're basically using
>> cpu.uclamp.min to temporarily switch iowait boost on/off.
>>
>> Isn't it better to add a new cgroup attribute to toggle this feature?
>>
>> The problem does seem generic enough and could benefit other battery-powered
>> devices outside of the Android world. I don't think the dependency on uclamp &&
>> energy model are necessary to solve this.
> 
> I think using uclamp is not a bad idea here, but perhaps we could do
> things differently. As of today the iowait boost escapes the clamping
> mechanism, so one option would be to change that. That would let us set
> a low max clamp in the 'background' cgroup, which in turns would limit
> the frequency request for those tasks even if they're IO-intensive.
> 

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.

> That'll have to be done at the RQ level, but figuring out what's the
> current max clamp on the rq should be doable from sugov I think.
> 
> Wei: would that work for your use case ?
> 
> Also, the iowait boost really should be per-task and not per-cpu, so it
> can be taken into account during wake-up balance on big.LITTLE. That
> might also help on SMP if a task doing a lot of IO migrates, as the
> boost would migrate with it. But that's perhaps for later ...
> 
> Thanks,
> Quentin
>