* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-24 12:30 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123201146.GH17749@hirez.programming.kicks-ass.net>
On 23-Jan 21:11, Peter Zijlstra wrote:
> On Wed, Jan 23, 2019 at 02:40:11PM +0000, Patrick Bellasi wrote:
> > On 23-Jan 11:49, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> > > > @@ -858,16 +859,23 @@ static inline void
> > > > uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > > > unsigned int *clamp_value, unsigned int *bucket_id)
> > > > {
> > > > + struct uclamp_se *default_clamp;
> > > > +
> > > > /* Task specific clamp value */
> > > > *clamp_value = p->uclamp[clamp_id].value;
> > > > *bucket_id = p->uclamp[clamp_id].bucket_id;
> > > >
> > > > + /* RT tasks have different default values */
> > > > + default_clamp = task_has_rt_policy(p)
> > > > + ? uclamp_default_perf
> > > > + : uclamp_default;
> > > > +
> > > > /* System default restriction */
> > > > - if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> > > > - *clamp_value > uclamp_default[UCLAMP_MAX].value)) {
> > > > + if (unlikely(*clamp_value < default_clamp[UCLAMP_MIN].value ||
> > > > + *clamp_value > default_clamp[UCLAMP_MAX].value)) {
> > > > /* Keep it simple: unconditionally enforce system defaults */
> > > > - *clamp_value = uclamp_default[clamp_id].value;
> > > > - *bucket_id = uclamp_default[clamp_id].bucket_id;
> > > > + *clamp_value = default_clamp[clamp_id].value;
> > > > + *bucket_id = default_clamp[clamp_id].bucket_id;
> > > > }
> > > > }
> > >
> > > So I still don't much like the whole effective thing;
> >
> > :/
> >
> > I find back-annotation useful in many cases since we have different
> > sources for possible clamp values:
> >
> > 1. task specific
> > 2. cgroup defined
> > 3. system defaults
> > 4. system power default
>
> (I'm not sure I've seen 4 happen yet, what's that?)
Typo: that's "system s/power/perf/ default", i.e. uclamp_default_perf
introduced by this patch.
> Anyway, once you get range composition defined; that should be something
> like:
>
> R_p \Compose_g R_g
>
> Where R_p is the range of task-p, and R_g is the range of the g'th
> cgroup of p (where you can make an identity between the root cgroup and
> the system default).
>
> Now; as per the other email; I think the straight forward composition:
>
> struct range compose(struct range a, struct range b)
> {
> return (range){.min = clamp(a.min, b.min, b.max),
> .max = clamp(a.max, b.min, b.max), };
> }
This composition is done in uclamp_effective_get() but it's
slightly different, since we want to support a "nice policy" where
tasks can always ask less then what they have got assigned.
Thus, from an abstract standpoint, if a task is in a cgroup:
task.min <= R_g.min
task.max <= R_g.max
While, for tasks in the root cgroup system default applies and we
enforece:
task.min >= R_0.min
task.max <= R_0.max
... where the "nice policy" is currently not more supported, but
perhaps we can/should use the same for system defaults too.
> (note that this is non-commutative, so we have to pay attention to
> get the order 'right')
>
> Works in this case; unlike the cpu/rq conposition where we resort to a
> pure max function for non-interference.
Right.
> > I don't think we can avoid to somehow back annotate on which bucket a
> > task has been refcounted... it makes dequeue so much easier, it helps
> > in ensuring that the refcouning is consistent and enable lazy updates.
>
> So I'll have to go over the code again, but I'm wondering why you're
> changing uclamp_se::bucket_id on a runnable task.
We change only the "requested" value, not the "effective" one.
> Ideally you keep bucket_id invariant between enqueue and dequeue; then
> dequeue knows where we put it.
Right, that's what we do for the "effective" value.
> Now I suppose actually determining bucket_id is 'expensive' (it
> certainly is with the whole mapping scheme, but even that integer
> division is not nice), so we'd like to precompute the bucket_id.
Yes, although the complexity is mostly in the composition logic
described above not on mapping at all. We have "mapping" overheads
only when we change a "request" value and that's from slow-paths.
The "effective" value is computed at each enqueue time by composing
precomputed bucket_id representing the current "requested" task's,
cgroup's and system default's values.
> This then leads to the problem of how to change uclamp_se::value while
> runnable (since per the other thread you don't want to always update all
> runnable tasks).
>
> So far so right?
Yes.
> I'm thikning that if we haz a single bit, say:
>
> struct uclamp_se {
> ...
> unsigned int changed : 1;
> };
>
> We can update uclamp_se::value and set uclamp_se::changed, and then the
> next enqueue will (unlikely) test-and-clear changed and recompute the
> bucket_id.
This mean will lazy update the "requested" bucket_id by deferring its
computation at enqueue time. Which saves us a copy of the bucket_id,
i.e. we will have only the "effective" value updated at enqueue time.
But...
> Would that not be simpler?
... although being simpler it does not fully exploit the slow-path,
a syscall which is usually running from a different process context
(system management software).
It also fits better for lazy updates but, in the cgroup case, where we
wanna enforce an update ASAP for RUNNABLE tasks, we will still have to
do the updates from the slow-path.
Will look better into this simplification while working on v7, perhaps
the linear mapping can really help in that too.
Cheers Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Patrick Bellasi @ 2019-01-24 11:21 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123185940.GF17749@hirez.programming.kicks-ass.net>
On 23-Jan 19:59, Peter Zijlstra wrote:
> On Wed, Jan 23, 2019 at 02:14:26PM +0000, Patrick Bellasi wrote:
>
> > > > Consider also that the uclamp_task_update_active() added by this patch
> > > > not only has lower overhead but it will be use also by cgroups where
> > > > we want to force update all the tasks on a cgroup's clamp change.
> > >
> > > I haven't gotten that far; but I would prefer not to have two different
> > > 'change' paths in __sched_setscheduler().
> >
> > Yes, I agree that two paths in __sched_setscheduler() could be
> > confusing. Still we have to consider that here we are adding
> > "not class specific" attributes.
>
> But that change thing is not class specific; the whole:
>
>
> rq = task_rq_lock(p, &rf);
> queued = task_on_rq_queued(p);
> running = task_current(rq, p);
> if (queued)
> dequeue_task(rq, p, queue_flags);
> if (running)
> put_prev_task(rq, p);
>
>
> /* @p is in it's invariant state; frob it's state */
>
>
> if (queued)
> enqueue_task(rq, p, queue_flags);
> if (running)
> set_curr_task(rq, p);
> task_rq_unlock(rq, p, &rf);
>
>
> pattern is all over the place; it is just because C sucks that that
Yes, understand, don't want to enter a language war :)
> isn't more explicitly shared (do_set_cpus_allowed(), rt_mutex_setprio(),
> set_user_nice(), __sched_setscheduler(), sched_setnuma(),
> sched_move_task()).
>
> This is _the_ pattern for changing state and is not class specific at
> all.
Right, that pattern is not "class specific" true and I should have not
used that term to begin with.
What I was trying to point out is that all the calls above directly
affect the current scheduling decision and "requires" a
dequeue/enqueue pattern.
When a task-specific uclamp value is changed for a task, instead, a
dequeue/enqueue is not needed. As long as we are doing a lazy update,
that sounds just like not necessary overhead.
However, there could still be value in keeping code consistent and if
you prefer it this way what should I do?
---8<---
__sched_setscheduler()
...
if (policy < 0)
policy = oldpolicy = p->policy;
...
if (unlikely(policy == p->policy)) {
...
if (uclamp_changed()) // Force dequeue/enqueue
goto change;
}
change:
...
if (queued)
dequeue_task(rq, p, queue_flags);
if (running)
put_prev_task(rq, p);
__setscheduler_uclamp();
__setscheduler(rq, p, attr, pi);
if (queued)
enqueue_task(rq, p, queue_flags);
if (running)
set_curr_task(rq, p);
...
---8<---
Could be something like that ok with you?
Not sure about side-effects on p->prio(): for CFS seems to be reset to
NORMAL in this case :/
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Dominique Martinet @ 2019-01-24 0:24 UTC (permalink / raw)
To: Linus Torvalds, Jiri Kosina
Cc: Andy Lutomirski, Josh Snyder, Dave Chinner, Matthew Wilcox,
Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra, Michal Hocko,
Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wg+C65FJHB=Jx1OvuJP4kvpWdw+5G=XOXB6X_KB2XuofA@mail.gmail.com>
Linus Torvalds wrote on Thu, Jan 24, 2019:
> I've reverted the 'let's try to just remove the code' part in my tree.
> But I didn't apply the two other patches yet. Any final comments
> before that should happen?
I mentionned when sending the updated version that just checking file
permission might not be enough, e.g. a git tree is full of read-only
objects that someone might want to preload and think we might really
want to check both despite the overhead in the denied case.
Josh agreed and I meant to send a new version since nothing was
happening but work priorities got the better of me, and I was kind of
waiting for the ltp testcases[1] as well because aside from the few
tests I ran by hand I'm not sure the few hours of ltp/xfstests Jiri ran
did much but this is probably going to be a chicken-or-egg problem..
[1] https://github.com/linux-test-project/ltp/issues/461
Jiri Kosina wrote on Thu, Jan 24, 2019:
> On Thu, 24 Jan 2019, Linus Torvalds wrote:
>
> > Side note: the inode_permission() addition to can_do_mincore() in that
> > patch 0002, seems to be questionable. We do
> >
> > +static inline bool can_do_mincore(struct vm_area_struct *vma)
> > +{
> > + return vma_is_anonymous(vma)
> > + || (vma->vm_file && (vma->vm_file->f_mode & FMODE_WRITE))
> > + || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
> > +}
> >
> > note how it tests whether vma->vm_file is NULL for the FMODE_WRITE
> > test, but not for the inode_permission() test.
> >
> > So either we test unnecessarily in the second line, or we don't
> > properly test it in the third one.
> >
> > I think the "test vm_file" thing may be unnecessary, because a
> > non-anonymous mapping should always have a file pointer and an inode.
> > But I could imagine some odd case (vdso mapping, anyone?) that
> > doesn't have a vm_file, but also isn't anonymous.
>
> Hmm, good point.
>
> So dropping the 'vma->vm_file' test and checking whether given vma is
> special mapping should hopefully provide the desired semantics, shouldn't
> it?
I think it's probably better to keep this simple, if we're going to
check something before accessing vm_file we might as well directly check
it.
I was thinking of something along the lines of:
return vma_is_anonymous(vma) || (vma->vm_file &&
(inode_owner_or_capable(file_inode(vma->vm_file))
|| inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0));
I dropped the first f_mode check because none of the known mincore users
open the files read-write, and the check is redundant with
inode_permission() so while it would probably be an optimisation in some
cases I do not think it is useful in practice.
On the other hand, I have no idea how expensive the inode_permission and
owner checks really are - do they try to refresh attributes on a
networked filesystem or would it trust the cache or is it fs dependant?
Honestly this is more a case of "the people who's be interested in
seeing this have no idea what they're doing" than lack of interest.. I
wouldn't mind if there were tests doing mincore on a bunch of special
files/mappings but I just tried on a few regular files by hand, this
isn't proper coverage; I'll try to take more time to test various
mappings today (JST).
Thanks,
--
Dominique
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-24 0:20 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dominique Martinet, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901240009560.6626@cbobk.fhfr.pm>
On Thu, Jan 24, 2019 at 12:12 PM Jiri Kosina <jikos@kernel.org> wrote:
>
> >
> > I think the "test vm_file" thing may be unnecessary, because a
> > non-anonymous mapping should always have a file pointer and an inode.
> > But I could imagine some odd case (vdso mapping, anyone?) that
> > doesn't have a vm_file, but also isn't anonymous.
>
> Hmm, good point.
>
> So dropping the 'vma->vm_file' test and checking whether given vma is
> special mapping should hopefully provide the desired semantics, shouldn't
> it?
Maybe. But on the whole I think it would be simpler and more
straightforward to just instead add a vm_file test for the
inode_permission() case. That way you at least know that you aren't
following a NULL pointer.
If the file then turns out to be some special thing, it doesn't really
_matter_, I think. It won't have anything in the page cache etc, but
the code should "work".
Linus
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Jiri Kosina @ 2019-01-23 23:12 UTC (permalink / raw)
To: Linus Torvalds
Cc: Dominique Martinet, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wgy+1YT-Rhj5qWb_aCuBADhcq42GDKHB74sqrnOVPKzPg@mail.gmail.com>
On Thu, 24 Jan 2019, Linus Torvalds wrote:
> Side note: the inode_permission() addition to can_do_mincore() in that
> patch 0002, seems to be questionable. We do
>
> +static inline bool can_do_mincore(struct vm_area_struct *vma)
> +{
> + return vma_is_anonymous(vma)
> + || (vma->vm_file && (vma->vm_file->f_mode & FMODE_WRITE))
> + || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
> +}
>
> note how it tests whether vma->vm_file is NULL for the FMODE_WRITE
> test, but not for the inode_permission() test.
>
> So either we test unnecessarily in the second line, or we don't
> properly test it in the third one.
>
> I think the "test vm_file" thing may be unnecessary, because a
> non-anonymous mapping should always have a file pointer and an inode.
> But I could imagine some odd case (vdso mapping, anyone?) that
> doesn't have a vm_file, but also isn't anonymous.
Hmm, good point.
So dropping the 'vma->vm_file' test and checking whether given vma is
special mapping should hopefully provide the desired semantics, shouldn't
it?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-23 20:35 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dominique Martinet, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <CAHk-=wg+C65FJHB=Jx1OvuJP4kvpWdw+5G=XOXB6X_KB2XuofA@mail.gmail.com>
On Thu, Jan 24, 2019 at 9:27 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> I've reverted the 'let's try to just remove the code' part in my tree.
> But I didn't apply the two other patches yet. Any final comments
> before that should happen?
Side note: the inode_permission() addition to can_do_mincore() in that
patch 0002, seems to be questionable. We do
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+ return vma_is_anonymous(vma)
+ || (vma->vm_file && (vma->vm_file->f_mode & FMODE_WRITE))
+ || inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
+}
note how it tests whether vma->vm_file is NULL for the FMODE_WRITE
test, but not for the inode_permission() test.
So either we test unnecessarily in the second line, or we don't
properly test it in the third one.
I think the "test vm_file" thing may be unnecessary, because a
non-anonymous mapping should always have a file pointer and an inode.
But I could imagine some odd case (vdso mapping, anyone?) that
doesn't have a vm_file, but also isn't anonymous.
Anybody?
Anyway, it's one reason why I didn't actually apply those other two
patches yet. This may be a 5.1 issue..
Linus
^ permalink raw reply
* Re: [PATCH] mm/mincore: allow for making sys_mincore() privileged
From: Linus Torvalds @ 2019-01-23 20:27 UTC (permalink / raw)
To: Jiri Kosina
Cc: Dominique Martinet, Andy Lutomirski, Josh Snyder, Dave Chinner,
Matthew Wilcox, Jann Horn, Andrew Morton, Greg KH, Peter Zijlstra,
Michal Hocko, Linux-MM, kernel list, Linux API
In-Reply-To: <nycvar.YFH.7.76.1901162120000.6626@cbobk.fhfr.pm>
On Thu, Jan 17, 2019 at 9:23 AM Jiri Kosina <jikos@kernel.org> wrote:
>
> So I've done some basic smoke testing (~2 hours of LTP+xfstests) on the
> kernel with the three topmost patches from
>
> https://git.kernel.org/pub/scm/linux/kernel/git/jikos/jikos.git/log/?h=pagecache-sidechannel
>
> applied (also attaching to this mail), and no obvious breakage popped up.
>
> So if noone sees any principal problem there, I'll happily submit it with
> proper attribution etc.
So this seems to have died down, and a week later we seem to not have
a lot of noise here any more. I think it means people either weren't
testing it, or just didn't find any real problems.
I've reverted the 'let's try to just remove the code' part in my tree.
But I didn't apply the two other patches yet. Any final comments
before that should happen?
Linus
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-23 20:11 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123144011.iid3avb63r5v4r2c@e110439-lin>
On Wed, Jan 23, 2019 at 02:40:11PM +0000, Patrick Bellasi wrote:
> On 23-Jan 11:49, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> > > @@ -858,16 +859,23 @@ static inline void
> > > uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > > unsigned int *clamp_value, unsigned int *bucket_id)
> > > {
> > > + struct uclamp_se *default_clamp;
> > > +
> > > /* Task specific clamp value */
> > > *clamp_value = p->uclamp[clamp_id].value;
> > > *bucket_id = p->uclamp[clamp_id].bucket_id;
> > >
> > > + /* RT tasks have different default values */
> > > + default_clamp = task_has_rt_policy(p)
> > > + ? uclamp_default_perf
> > > + : uclamp_default;
> > > +
> > > /* System default restriction */
> > > - if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> > > - *clamp_value > uclamp_default[UCLAMP_MAX].value)) {
> > > + if (unlikely(*clamp_value < default_clamp[UCLAMP_MIN].value ||
> > > + *clamp_value > default_clamp[UCLAMP_MAX].value)) {
> > > /* Keep it simple: unconditionally enforce system defaults */
> > > - *clamp_value = uclamp_default[clamp_id].value;
> > > - *bucket_id = uclamp_default[clamp_id].bucket_id;
> > > + *clamp_value = default_clamp[clamp_id].value;
> > > + *bucket_id = default_clamp[clamp_id].bucket_id;
> > > }
> > > }
> >
> > So I still don't much like the whole effective thing;
>
> :/
>
> I find back-annotation useful in many cases since we have different
> sources for possible clamp values:
>
> 1. task specific
> 2. cgroup defined
> 3. system defaults
> 4. system power default
(I'm not sure I've seen 4 happen yet, what's that?)
Anyway, once you get range composition defined; that should be something
like:
R_p \Compose_g R_g
Where R_p is the range of task-p, and R_g is the range of the g'th
cgroup of p (where you can make an identity between the root cgroup and
the system default).
Now; as per the other email; I think the straight forward composition:
struct range compose(struct range a, struct range b)
{
return (range){.min = clamp(a.min, b.min, b.max),
.max = clamp(a.max, b.min, b.max), };
}
(note that this is non-commutative, so we have to pay attention to
get the order 'right')
Works in this case; unlike the cpu/rq conposition where we resort to a
pure max function for non-interference.
> I don't think we can avoid to somehow back annotate on which bucket a
> task has been refcounted... it makes dequeue so much easier, it helps
> in ensuring that the refcouning is consistent and enable lazy updates.
So I'll have to go over the code again, but I'm wondering why you're
changing uclamp_se::bucket_id on a runnable task.
Ideally you keep bucket_id invariant between enqueue and dequeue; then
dequeue knows where we put it.
Now I suppose actually determining bucket_id is 'expensive' (it
certainly is with the whole mapping scheme, but even that integer
division is not nice), so we'd like to precompute the bucket_id.
This then leads to the problem of how to change uclamp_se::value while
runnable (since per the other thread you don't want to always update all
runnable tasks).
So far so right?
I'm thikning that if we haz a single bit, say:
struct uclamp_se {
...
unsigned int changed : 1;
};
We can update uclamp_se::value and set uclamp_se::changed, and then the
next enqueue will (unlikely) test-and-clear changed and recompute the
bucket_id.
Would that not be simpler?
^ permalink raw reply
* Re: [PATCH v6 10/16] sched/core: Add uclamp_util_with()
From: Peter Zijlstra @ 2019-01-23 19:22 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123145106.zaqb3d6l65rs5lg6@e110439-lin>
On Wed, Jan 23, 2019 at 02:51:06PM +0000, Patrick Bellasi wrote:
> On 23-Jan 14:33, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:07AM +0000, Patrick Bellasi wrote:
> > > +static __always_inline
> > > +unsigned int uclamp_util_with(struct rq *rq, unsigned int util,
> > > + struct task_struct *p)
> > > {
> > > unsigned int min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > > unsigned int max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> > >
> > > + if (p) {
> > > + min_util = max(min_util, uclamp_effective_value(p, UCLAMP_MIN));
> > > + max_util = max(max_util, uclamp_effective_value(p, UCLAMP_MAX));
> > > + }
> > > +
> >
> > Like I think you mentioned earlier; this doesn't look right at all.
>
> What we wanna do here is to compute what _will_ be the clamp values of
> a CPU if we enqueue *p on it.
>
> The code above starts from the current CPU clamp value and mimics what
> uclamp will do in case we move the task there... which is always a max
> aggregation.
Ah, then I misunderstood the purpose of this function.
> > Should that not be something like:
> >
> > lo = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > hi = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> >
> > min_util = clamp(uclamp_effective(p, UCLAMP_MIN), lo, hi);
> > max_util = clamp(uclamp_effective(p, UCLAMP_MAX), lo, hi);
>
> Here you end up with a restriction of the task clamp (effective)
> clamps values considering the CPU clamps... which is different.
>
> Why do you think we should do that?... perhaps I'm missing something.
I was left with the impression from patch 7 that we don't compose clamps
right and throught that was what this code was supposed to do.
I'll have another look at this patch.
^ permalink raw reply
* Re: [PATCH v6 07/16] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-01-23 19:10 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123141924.x36iqxh42lkssrxl@e110439-lin>
On Wed, Jan 23, 2019 at 02:19:24PM +0000, Patrick Bellasi wrote:
> On 23-Jan 10:22, Peter Zijlstra wrote:
> > On Tue, Jan 22, 2019 at 03:41:29PM +0000, Patrick Bellasi wrote:
> > > On 22-Jan 16:13, Peter Zijlstra wrote:
> > > > On Tue, Jan 22, 2019 at 02:43:29PM +0000, Patrick Bellasi wrote:
> >
> > > > > Do you think that could be acceptable?
> > > >
> > > > Think so, it's a sysctl poke, 'nobody' ever does that.
> > >
> > > Cool, so... I'll keep lazy update for system default.
> >
> > Ah, I think I misunderstood. I meant to say that since nobody ever pokes
> > at sysctl's it doesn't matter if its a little more expensive and iterate
> > everything.
>
> Here I was more worried about the code complexity/overhead... for
> something actually not very used/useful.
>
> > Also; if you always keep everything up-to-date, you can avoid doing that
> > duplicate accounting.
>
> To update everything we will have to walk all the CPUs and update all
> the RUNNABLE tasks currently enqueued, which are either RT or CFS.
>
> That's way more expensive both in code and time then what we do for
> cgroups, where at least we have a limited scope since the cgroup
> already provides a (usually limited) list of tasks to consider.
>
> Do you think it's really worth to have ?
Dunno; the whole double bucket thing seems a bit weird to me; but maybe
it will all look better without the mapping stuff.
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Peter Zijlstra @ 2019-01-23 18:59 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123141426.5samtr4hl6okdypu@e110439-lin>
On Wed, Jan 23, 2019 at 02:14:26PM +0000, Patrick Bellasi wrote:
> > > Consider also that the uclamp_task_update_active() added by this patch
> > > not only has lower overhead but it will be use also by cgroups where
> > > we want to force update all the tasks on a cgroup's clamp change.
> >
> > I haven't gotten that far; but I would prefer not to have two different
> > 'change' paths in __sched_setscheduler().
>
> Yes, I agree that two paths in __sched_setscheduler() could be
> confusing. Still we have to consider that here we are adding
> "not class specific" attributes.
But that change thing is not class specific; the whole:
rq = task_rq_lock(p, &rf);
queued = task_on_rq_queued(p);
running = task_current(rq, p);
if (queued)
dequeue_task(rq, p, queue_flags);
if (running)
put_prev_task(rq, p);
/* @p is in it's invariant state; frob it's state */
if (queued)
enqueue_task(rq, p, queue_flags);
if (running)
set_curr_task(rq, p);
task_rq_unlock(rq, p, &rf);
pattern is all over the place; it is just because C sucks that that
isn't more explicitly shared (do_set_cpus_allowed(), rt_mutex_setprio(),
set_user_nice(), __sched_setscheduler(), sched_setnuma(),
sched_move_task()).
This is _the_ pattern for changing state and is not class specific at
all.
^ permalink raw reply
* Re: [PATCH v4 3/3] fs: let filldir_t return bool instead of an error code
From: Jann Horn @ 2019-01-23 15:07 UTC (permalink / raw)
To: Dave Chinner
Cc: Richard Henderson, Ivan Kokshaysky, Matt Turner, Alexander Viro,
linux-fsdevel, Arnd Bergmann, Eric W. Biederman,
Theodore Ts'o, Andreas Dilger, linux-alpha, kernel list,
Pavel Machek, linux-arch, Linux API
In-Reply-To: <20190121222411.GD4205@dastard>
On Mon, Jan 21, 2019 at 11:24 PM Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Jan 21, 2019 at 04:49:45PM +0100, Jann Horn wrote:
> > On Sun, Jan 20, 2019 at 11:41 PM Dave Chinner <david@fromorbit.com> wrote:
> > > On Fri, Jan 18, 2019 at 05:14:40PM +0100, Jann Horn wrote:
> > > > As Al Viro pointed out, many filldir_t functions return error codes, but
> > > > all callers of filldir_t functions just check whether the return value is
> > > > non-zero (to determine whether to continue reading the directory); more
> > > > precise errors have to be signalled via struct dir_context.
> > > > Change all filldir_t functions to return bool instead of int.
> > > >
> > > > Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > > > ---
> > > > arch/alpha/kernel/osf_sys.c | 12 +++----
> > > > fs/afs/dir.c | 30 +++++++++--------
> > > > fs/ecryptfs/file.c | 13 ++++----
> > > > fs/exportfs/expfs.c | 8 ++---
> > > > fs/fat/dir.c | 8 ++---
> > > > fs/gfs2/export.c | 6 ++--
> > > > fs/nfsd/nfs4recover.c | 8 ++---
> > > > fs/nfsd/vfs.c | 6 ++--
> > > > fs/ocfs2/dir.c | 10 +++---
> > > > fs/ocfs2/journal.c | 14 ++++----
> > > > fs/overlayfs/readdir.c | 24 +++++++-------
> > > > fs/readdir.c | 64 ++++++++++++++++++-------------------
> > > > fs/reiserfs/xattr.c | 20 ++++++------
> > > > fs/xfs/scrub/dir.c | 8 ++---
> > > > fs/xfs/scrub/parent.c | 4 +--
> > > > include/linux/fs.h | 10 +++---
> > > > 16 files changed, 125 insertions(+), 120 deletions(-)
> > > >
> > > > diff --git a/arch/alpha/kernel/osf_sys.c b/arch/alpha/kernel/osf_sys.c
> > > > index db1c2144d477..14e5ae0dac50 100644
> > > > --- a/arch/alpha/kernel/osf_sys.c
> > > > +++ b/arch/alpha/kernel/osf_sys.c
> > > > @@ -108,7 +108,7 @@ struct osf_dirent_callback {
> > > > int error;
> > > > };
> > > >
> > > > -static int
> > > > +static bool
> > > > osf_filldir(struct dir_context *ctx, const char *name, int namlen,
> > > > loff_t offset, u64 ino, unsigned int d_type)
> > > > {
> > > > @@ -120,14 +120,14 @@ osf_filldir(struct dir_context *ctx, const char *name, int namlen,
> > > >
> > > > buf->error = check_dirent_name(name, namlen);
> > > > if (unlikely(buf->error))
> > > > - return -EFSCORRUPTED;
> > > > + return false;
> > > > buf->error = -EINVAL; /* only used if we fail */
> > > > if (reclen > buf->count)
> > > > - return -EINVAL;
> > > > + return false;
> > >
> > > Oh, it's because the error being returned is being squashed by
> > > dir_emit():
> >
> > Yeah.
> >
> > > > struct dir_context {
> > > > @@ -3469,17 +3471,17 @@ static inline bool dir_emit(struct dir_context *ctx,
> > > > const char *name, int namelen,
> > > > u64 ino, unsigned type)
> > > > {
> > > > - return ctx->actor(ctx, name, namelen, ctx->pos, ino, type) == 0;
> > > > + return ctx->actor(ctx, name, namelen, ctx->pos, ino, type);
> > > > }
> > >
> > > /me wonders if it would be cleaner to do:
> > >
> > > static inline bool dir_emit(...)
> > > {
> > > buf->error = ctx->actor(....)
> > > if (buf->error)
> > > return false;
> > > return true;
> > > }
> > >
> > > And clean up all filldir actors just to return the error state
> > > rather than have to jump through hoops to stash the error state in
> > > the context buffer and return the error state?
> >
> > One negative thing about that, IMO, is that it mixes up the request
> > for termination of the loop and the presence of an error.
>
> Doesn't the code already do that, only worse?
The current code does that, yes. But with this patch, I think that's
not really the case anymore?
> > > That then allows callers who want/need the full error info can
> > > continue to call ctx->actor directly,
> >
> > "continue to call ctx->actor directly"? I don't remember any code that
> > calls ctx->actor directly.
>
> ovl_fill_real().
Ah, right.
> And the XFS directory scrubber could probably make better use of the
> error return from ctx->actor when validating the directory contents
> rather than just calling dir_emit() and aborting the scan at the
> first error encountered. We eventually want to know exactly what
> error was encountered here to determine if it is safe to continue,
> not just a "stop processing" flag. e.g. a bad name length will need
> to stop traversal because we can't trust the underlying structure,
> but an invalid file type isn't a structural flaw that prevents us
> from continuing to traverse and check the rest of the directory....
Sorry, maybe I'm a bit dense right now, I don't get your point. Are
you talking about filesystem errors detected in the actor? If so,
doesn't it make *more* sense for non-fatal errors to put a note that
an error happened into the xchk_dir_ctx (if that information should be
kept around), then return a value that says "please continue"?
Or are you talking about filesystem errors detected in the readdir
implementation? In that case, you're AFAICS going to need special-case
logic gated on ctx->actor==xchk_dir_actor anyway if you want the
scrubber to continue while readdir() stops.
(But as I've said, I don't really care about this patch. If Al takes
patches 1 and 2 from this series, I'm happy; this patch is just in
response to <https://lore.kernel.org/lkml/20180731165112.GJ30522@ZenIV.linux.org.uk/>.)
^ permalink raw reply
* Re: [PATCH v6 10/16] sched/core: Add uclamp_util_with()
From: Patrick Bellasi @ 2019-01-23 14:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123133324.GY27931@hirez.programming.kicks-ass.net>
On 23-Jan 14:33, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:07AM +0000, Patrick Bellasi wrote:
> > +static __always_inline
> > +unsigned int uclamp_util_with(struct rq *rq, unsigned int util,
> > + struct task_struct *p)
> > {
> > unsigned int min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > unsigned int max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> >
> > + if (p) {
> > + min_util = max(min_util, uclamp_effective_value(p, UCLAMP_MIN));
> > + max_util = max(max_util, uclamp_effective_value(p, UCLAMP_MAX));
> > + }
> > +
>
> Like I think you mentioned earlier; this doesn't look right at all.
What we wanna do here is to compute what _will_ be the clamp values of
a CPU if we enqueue *p on it.
The code above starts from the current CPU clamp value and mimics what
uclamp will do in case we move the task there... which is always a max
aggregation.
> Should that not be something like:
>
> lo = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> hi = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
>
> min_util = clamp(uclamp_effective(p, UCLAMP_MIN), lo, hi);
> max_util = clamp(uclamp_effective(p, UCLAMP_MAX), lo, hi);
Here you end up with a restriction of the task clamp (effective)
clamps values considering the CPU clamps... which is different.
Why do you think we should do that?... perhaps I'm missing something.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-23 14:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123104944.GX27931@hirez.programming.kicks-ass.net>
On 23-Jan 11:49, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> > @@ -858,16 +859,23 @@ static inline void
> > uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > unsigned int *clamp_value, unsigned int *bucket_id)
> > {
> > + struct uclamp_se *default_clamp;
> > +
> > /* Task specific clamp value */
> > *clamp_value = p->uclamp[clamp_id].value;
> > *bucket_id = p->uclamp[clamp_id].bucket_id;
> >
> > + /* RT tasks have different default values */
> > + default_clamp = task_has_rt_policy(p)
> > + ? uclamp_default_perf
> > + : uclamp_default;
> > +
> > /* System default restriction */
> > - if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> > - *clamp_value > uclamp_default[UCLAMP_MAX].value)) {
> > + if (unlikely(*clamp_value < default_clamp[UCLAMP_MIN].value ||
> > + *clamp_value > default_clamp[UCLAMP_MAX].value)) {
> > /* Keep it simple: unconditionally enforce system defaults */
> > - *clamp_value = uclamp_default[clamp_id].value;
> > - *bucket_id = uclamp_default[clamp_id].bucket_id;
> > + *clamp_value = default_clamp[clamp_id].value;
> > + *bucket_id = default_clamp[clamp_id].bucket_id;
> > }
> > }
>
> So I still don't much like the whole effective thing;
:/
I find back-annotation useful in many cases since we have different
sources for possible clamp values:
1. task specific
2. cgroup defined
3. system defaults
4. system power default
I don't think we can avoid to somehow back annotate on which bucket a
task has been refcounted... it makes dequeue so much easier, it helps
in ensuring that the refcouning is consistent and enable lazy updates.
> but I think you should use rt_task() instead of
> task_has_rt_policy().
Right... will do.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Patrick Bellasi @ 2019-01-23 14:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123102821.GW27931@hirez.programming.kicks-ass.net>
On 23-Jan 11:28, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> > - util = (type == ENERGY_UTIL)
> > - ? util_cfs
> > - : uclamp_util(rq, util_cfs);
> > - util += cpu_util_rt(rq);
> > + util = cpu_util_rt(rq);
> > + if (type == FREQUENCY_UTIL) {
> > + util += cpu_util_cfs(rq);
> > + util = uclamp_util(rq, util);
> > + } else {
> > + util += util_cfs;
> > + }
>
> Or the combined thing:
>
> - util = util_cfs;
> - util += cpu_util_rt(rq);
> + util = cpu_util_rt(rq);
> + if (type == FREQUENCY_UTIL) {
> + util += cpu_util_cfs(rq);
> + util = uclamp_util(rq, util);
> + } else {
> + util += util_cfs;
> + }
>
>
> Leaves me confused.
>
> When type == FREQ, util_cfs should already be cpu_util_cfs(), per
> sugov_get_util().
>
> So should that not end up like:
>
> util = util_cfs;
> util += cpu_util_rt(rq);
> + if (type == FREQUENCY_UTIL)
> + util = uclamp_util(rq, util);
>
> instead?
You right, I get to that core after the patches which integrate
compute_energy(). The chuck above was the version before the EM got
merged but I missed to backport the change once I rebased on
tip/sched/core with the EM in.
Sorry for the confusion, will fix in v7.
Cheers
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Patrick Bellasi @ 2019-01-23 14:24 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123095219.GV27931@hirez.programming.kicks-ass.net>
On 23-Jan 10:52, Peter Zijlstra wrote:
> On Tue, Jan 22, 2019 at 06:18:31PM +0000, Patrick Bellasi wrote:
> > On 22-Jan 18:13, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:05AM +0000, Patrick Bellasi wrote:
[...]
> > If a task is not clamped we execute it at its required utilization or
> > even max frequency in case of wakeup from IO.
> >
> > When a task is util_max clamped instead, we are saying that we don't
> > care to run it above the specified clamp value and, if possible, we
> > should run it below that capacity level.
> >
> > If that's the case, why this clamping hints should not be enforced on
> > IO wakeups too?
> >
> > At the end it's still a user-space decision, we basically allow
> > userspace to defined what's the max IO boost they like to get.
>
> Because it is the wrong knob for it.
>
> Ideally we'd extend the IO-wait state to include the device-busy state
> at the time of sleep. At the very least double state io_schedule() state
> space from 1 to 2 bits, where we not only indicate: yes this is an
> IO-sleep, but also can indicate device saturation. When the device is
> saturated, we don't need to boost further.
>
> (this binary state will ofcourse cause oscilations where we drop the
> freq, drop device saturation, then ramp the freq, regain device
> saturation etc..)
>
> However, doing this is going to require fairly massive surgery on our
> whole IO stack.
>
> Also; how big of a problem is 'supriouos' boosting really? Joel tried to
> introduce a boost_max tunable, but the grandual boosting thing was good
> enough at the time.
Ok then, I'll drop the clamp on IOBoost... you right and moreover we
can always investigate for a better solution in the future with a
real use-case on hand.
Cheers.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 07/16] sched/core: uclamp: Add system default clamps
From: Patrick Bellasi @ 2019-01-23 14:19 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123092210.GU27931@hirez.programming.kicks-ass.net>
On 23-Jan 10:22, Peter Zijlstra wrote:
> On Tue, Jan 22, 2019 at 03:41:29PM +0000, Patrick Bellasi wrote:
> > On 22-Jan 16:13, Peter Zijlstra wrote:
> > > On Tue, Jan 22, 2019 at 02:43:29PM +0000, Patrick Bellasi wrote:
>
> > > > Do you think that could be acceptable?
> > >
> > > Think so, it's a sysctl poke, 'nobody' ever does that.
> >
> > Cool, so... I'll keep lazy update for system default.
>
> Ah, I think I misunderstood. I meant to say that since nobody ever pokes
> at sysctl's it doesn't matter if its a little more expensive and iterate
> everything.
Here I was more worried about the code complexity/overhead... for
something actually not very used/useful.
> Also; if you always keep everything up-to-date, you can avoid doing that
> duplicate accounting.
To update everything we will have to walk all the CPUs and update all
the RUNNABLE tasks currently enqueued, which are either RT or CFS.
That's way more expensive both in code and time then what we do for
cgroups, where at least we have a limited scope since the cgroup
already provides a (usually limited) list of tasks to consider.
Do you think it's really worth to have ?
Perhaps we can add it in a second step, once we have the core bits in
and we really see a need for a specific use-case.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Patrick Bellasi @ 2019-01-23 14:14 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190123091634.GT27931@hirez.programming.kicks-ass.net>
On 23-Jan 10:16, Peter Zijlstra wrote:
> On Tue, Jan 22, 2019 at 03:33:15PM +0000, Patrick Bellasi wrote:
> > On 22-Jan 15:57, Peter Zijlstra wrote:
> > > On Tue, Jan 22, 2019 at 02:01:15PM +0000, Patrick Bellasi wrote:
>
> > > > Yes, I would say we have two options:
> > > >
> > > > 1) SCHED_FLAG_KEEP_POLICY enforces all the scheduling class specific
> > > > attributes, but cross class attributes (e.g. uclamp)
> > > >
> > > > 2) add SCHED_KEEP_NICE, SCHED_KEEP_PRIO, and SCED_KEEP_PARAMS
> > > > and use them in the if conditions in D)
> > >
> > > So the current KEEP_POLICY basically provides sched_setparam(), and
> >
> > But it's not exposed user-space.
>
> Correct; not until your first patch indeed.
>
> > > given we have that as a syscall, that is supposedly a useful
> > > functionality.
> >
> > For uclamp is definitively useful to change clamps without the need to
> > read beforehand the current policy params and use them in a following
> > set syscall... which is racy pattern.
>
> Right; but my argument was mostly that if sched_setparam() is a useful
> interface, a 'pure' KEEP_POLICY would be too and your (1) looses that.
Ok, that's an argument in favour of option (2).
> > > And I suppose the UTIL_CLAMP is !KEEP_UTIL; we could go either way
> > > around with that flag.
> >
> > What about getting rid of the racy case above by exposing userspace
> > only the new UTIL_CLAMP and, on:
> >
> > sched_setscheduler(flags: UTIL_CLAMP)
> >
> > we enforce the other two flags from the syscall:
> >
> > ---8<---
> > SYSCALL_DEFINE3(sched_setattr)
> > if (attr.sched_flags & SCHED_FLAG_KEEP_POLICY) {
> > attr.sched_policy = SETPARAM_POLICY;
> > attr.sched_flags |= (KEEP_POLICY|KEEP_PARAMS);
> > }
> > ---8<---
> >
> > This will not make possible to change class and set flags in one go,
> > but honestly that's likely a very limited use-case, isn't it ?
>
> So I must admit to not seeing much use for sched_setparam() (and its
> equivalents) myself, but given it is an existing interface, I also think
> it would be nice to cover that functionality in the sched_setattr()
> call.
Which will make them sort-of equivalent... meaning: both the POSIX
sched_setparam() and the !POSIX sched_setattr() will allow to change
params/attributes without changing the policy.
> That is; I know of userspace priority-ceiling implementations using
> sched_setparam(), but I don't see any reason why that wouldn't also work
> with sched_setscheduler() (IOW always also set the policy).
The sched_setscheduler() requires a policy to be explicitely defined,
it's a mandatory parameter and has to be specified.
Unless a RT task could be blocked by a FAIR one and you need
sched_setscheduler() to boost both prio and class (which looks like a
poor RT design to begin with) why would you use sched_setscheduler()
instead of sched_setparam()?
They are both POSIX calls and, AFAIU, sched_setparam() seems to be
designed exactly for those kind on use cases.
> > > > In both cases the goal should be to return from code block D).
> > >
> > > I don't think so; we really do want to 'goto change' for util changes
> > > too I think. Why duplicate part of that logic?
> >
> > But that will force a dequeue/enqueue... isn't too much overhead just
> > to change a clamp value?
>
> These syscalls aren't what I consider fast paths anyway. However, there
> are people that rely on the scheduler syscalls not to schedule
> themselves, or rather be non-blocking (see for example that prio-ceiling
> implementation).
>
> And in that respect the newly introduced uclamp_mutex does appear to be
> a problem.
Mmm... could be... I'll look better into it. Could be that that mutex
is not really required. We don't need to serialize task specific
clamp changes and anyway the protected code never sleeps and uses
atomic instruction.
> Also; do you expect these clamp values to be changed often?
Not really, the most common use cases are:
a) a resource manager (e.g. the Android run-time) set clamps for a
bunch of tasks whenever you switch for example from one app to
antoher... but that will be done via cgroups (i.e. different path)
b) a task can relax his constraints to save energy (something
conceptually similar to use a deferrable timers)
In both cases I expect a limited call frequency.
> > Perhaps we can also end up with some wired
>
> s/wired/weird/, right?
Right :)
> > side-effects like the task being preempted ?
>
> Nothing worse than any other random reschedule would cause.
>
> > Consider also that the uclamp_task_update_active() added by this patch
> > not only has lower overhead but it will be use also by cgroups where
> > we want to force update all the tasks on a cgroup's clamp change.
>
> I haven't gotten that far; but I would prefer not to have two different
> 'change' paths in __sched_setscheduler().
Yes, I agree that two paths in __sched_setscheduler() could be
confusing. Still we have to consider that here we are adding
"not class specific" attributes.
What if we keep "not class specific" code completely outside of
__sched_setscheduler() and do something like:
---8<---
int sched_setattr(struct task_struct *p, const struct sched_attr *attr)
{
retval = __sched_setattr(p, attr);
if (retval)
return retval;
return __sched_setscheduler(p, attr, true, true);
}
EXPORT_SYMBOL_GPL(sched_setattr);
---8<---
where __sched_setattr() will collect all the tunings which do not
require an enqueue/dequeue, so far only the new uclamp settings, while
the rest remains under __sched_setscheduler().
Thoughts ?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 10/16] sched/core: Add uclamp_util_with()
From: Peter Zijlstra @ 2019-01-23 13:33 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-11-patrick.bellasi@arm.com>
On Tue, Jan 15, 2019 at 10:15:07AM +0000, Patrick Bellasi wrote:
> +static __always_inline
> +unsigned int uclamp_util_with(struct rq *rq, unsigned int util,
> + struct task_struct *p)
> {
> unsigned int min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> unsigned int max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
>
> + if (p) {
> + min_util = max(min_util, uclamp_effective_value(p, UCLAMP_MIN));
> + max_util = max(max_util, uclamp_effective_value(p, UCLAMP_MAX));
> + }
> +
Like I think you mentioned earlier; this doesn't look right at all.
Should that not be something like:
lo = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
hi = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
min_util = clamp(uclamp_effective(p, UCLAMP_MIN), lo, hi);
max_util = clamp(uclamp_effective(p, UCLAMP_MAX), lo, hi);
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-23 10:49 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-10-patrick.bellasi@arm.com>
On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> @@ -858,16 +859,23 @@ static inline void
> uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> unsigned int *clamp_value, unsigned int *bucket_id)
> {
> + struct uclamp_se *default_clamp;
> +
> /* Task specific clamp value */
> *clamp_value = p->uclamp[clamp_id].value;
> *bucket_id = p->uclamp[clamp_id].bucket_id;
>
> + /* RT tasks have different default values */
> + default_clamp = task_has_rt_policy(p)
> + ? uclamp_default_perf
> + : uclamp_default;
> +
> /* System default restriction */
> - if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> - *clamp_value > uclamp_default[UCLAMP_MAX].value)) {
> + if (unlikely(*clamp_value < default_clamp[UCLAMP_MIN].value ||
> + *clamp_value > default_clamp[UCLAMP_MAX].value)) {
> /* Keep it simple: unconditionally enforce system defaults */
> - *clamp_value = uclamp_default[clamp_id].value;
> - *bucket_id = uclamp_default[clamp_id].bucket_id;
> + *clamp_value = default_clamp[clamp_id].value;
> + *bucket_id = default_clamp[clamp_id].bucket_id;
> }
> }
So I still don't much like the whole effective thing; but I think you
should use rt_task() instead of task_has_rt_policy().
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Peter Zijlstra @ 2019-01-23 10:28 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-10-patrick.bellasi@arm.com>
On Tue, Jan 15, 2019 at 10:15:06AM +0000, Patrick Bellasi wrote:
> - util = (type == ENERGY_UTIL)
> - ? util_cfs
> - : uclamp_util(rq, util_cfs);
> - util += cpu_util_rt(rq);
> + util = cpu_util_rt(rq);
> + if (type == FREQUENCY_UTIL) {
> + util += cpu_util_cfs(rq);
> + util = uclamp_util(rq, util);
> + } else {
> + util += util_cfs;
> + }
Or the combined thing:
- util = util_cfs;
- util += cpu_util_rt(rq);
+ util = cpu_util_rt(rq);
+ if (type == FREQUENCY_UTIL) {
+ util += cpu_util_cfs(rq);
+ util = uclamp_util(rq, util);
+ } else {
+ util += util_cfs;
+ }
Leaves me confused.
When type == FREQ, util_cfs should already be cpu_util_cfs(), per
sugov_get_util().
So should that not end up like:
util = util_cfs;
util += cpu_util_rt(rq);
+ if (type == FREQUENCY_UTIL)
+ util = uclamp_util(rq, util);
instead?
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Peter Zijlstra @ 2019-01-23 9:52 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122181831.a4w65qcivx4hua6d@e110439-lin>
On Tue, Jan 22, 2019 at 06:18:31PM +0000, Patrick Bellasi wrote:
> On 22-Jan 18:13, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:05AM +0000, Patrick Bellasi wrote:
> > > @@ -342,11 +350,24 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> > > return;
> > > sg_cpu->iowait_boost_pending = true;
> > >
> > > + /*
> > > + * Boost FAIR tasks only up to the CPU clamped utilization.
> > > + *
> > > + * Since DL tasks have a much more advanced bandwidth control, it's
> > > + * safe to assume that IO boost does not apply to those tasks.
> >
> > I'm not buying that argument. IO-boost isn't related to b/w management.
> >
> > IO-boot is more about compensating for hidden dependencies, and those
> > don't get less hidden for using a different scheduling class.
> >
> > Now, arguably DL should not be doing IO in the first place, but that's a
> > whole different discussion.
>
> My understanding is that IOBoost is there to help tasks doing many
> and _frequent_ IO operations, which are relatively _not so much_
> computational intensive on the CPU.
>
> Those tasks generate a small utilization and, without IOBoost, will be
> executed at a lower frequency and will add undesired latency on
> triggering the next IO operation.
>
> Isn't mainly that the reason for it?
http://lkml.kernel.org/r/20170522082154.f57cqovterd2qajv@hirez.programming.kicks-ass.net
Using a lower frequency will allow the IO device to go idle while we try
and get the next request going.
The connection between IO device and task/freq selection is hidden/lost.
We could potentially do better here, but fundamentally a completion
doesn't have an 'owner', there can be multiple waiters etc.
We loose (through our software architecture, and this we could possibly
improve, although it would be fairly invasive) the device busy state,
and it would be the device that raises the CPU frequency (to the point
where request submission is no longer the bottle neck to staying busy).
Currently all we do is mark a task as sleeping on IO and loose any
and all device relations/metrics.
So I don't think the task clamping should affect the IO boosting, as
that is meant to represent the device state, not the task utilization.
> IMHO, it makes perfectly sense to use DL for these kind of operations
> but I would expect that, since you care about latency we should come
> up with a proper description of the required bandwidth... eventually
> accounting for an additional headroom to compensate for "hidden
> dependencies"... without relaying on a quite dummy policy like
> IOBoost to get our DL tasks working.
Deadline is about determinsm, (file/disk) IO is typically the
anti-thesis of that.
> At the end, DL is now quite good in driving the freq as high has it
> needs... and by closing userspace feedback loops it can also
> compensate for all sort of fluctuations and noise... as demonstrated
> by Alessio during last OSPM:
>
> http://retis.sssup.it/luca/ospm-summit/2018/Downloads/OSPM_deadline_audio.pdf
Audio is a special in that it is indeed a deterministic device, also, I
don't think ALSA touches the IO-wait code, that is typically all
filesystem stuff.
> > > + * Instead, since RT tasks are not utilization clamped, we don't want
> > > + * to apply clamping on IO boost while there is blocked RT
> > > + * utilization.
> > > + */
> > > + max_boost = sg_cpu->iowait_boost_max;
> > > + if (!cpu_util_rt(cpu_rq(sg_cpu->cpu)))
> > > + max_boost = uclamp_util(cpu_rq(sg_cpu->cpu), max_boost);
> > > +
> > > /* Double the boost at each request */
> > > if (sg_cpu->iowait_boost) {
> > > sg_cpu->iowait_boost <<= 1;
> > > - if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
> > > - sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> > > + if (sg_cpu->iowait_boost > max_boost)
> > > + sg_cpu->iowait_boost = max_boost;
> > > return;
> > > }
> >
> > Hurmph... so I'm not sold on this bit.
>
> If a task is not clamped we execute it at its required utilization or
> even max frequency in case of wakeup from IO.
>
> When a task is util_max clamped instead, we are saying that we don't
> care to run it above the specified clamp value and, if possible, we
> should run it below that capacity level.
>
> If that's the case, why this clamping hints should not be enforced on
> IO wakeups too?
>
> At the end it's still a user-space decision, we basically allow
> userspace to defined what's the max IO boost they like to get.
Because it is the wrong knob for it.
Ideally we'd extend the IO-wait state to include the device-busy state
at the time of sleep. At the very least double state io_schedule() state
space from 1 to 2 bits, where we not only indicate: yes this is an
IO-sleep, but also can indicate device saturation. When the device is
saturated, we don't need to boost further.
(this binary state will ofcourse cause oscilations where we drop the
freq, drop device saturation, then ramp the freq, regain device
saturation etc..)
However, doing this is going to require fairly massive surgery on our
whole IO stack.
Also; how big of a problem is 'supriouos' boosting really? Joel tried to
introduce a boost_max tunable, but the grandual boosting thing was good
enough at the time.
^ permalink raw reply
* Re: [PATCH v6 07/16] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-01-23 9:22 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122154129.mxnpgaoxnccbjbch@e110439-lin>
On Tue, Jan 22, 2019 at 03:41:29PM +0000, Patrick Bellasi wrote:
> On 22-Jan 16:13, Peter Zijlstra wrote:
> > On Tue, Jan 22, 2019 at 02:43:29PM +0000, Patrick Bellasi wrote:
> > > Do you think that could be acceptable?
> >
> > Think so, it's a sysctl poke, 'nobody' ever does that.
>
> Cool, so... I'll keep lazy update for system default.
Ah, I think I misunderstood. I meant to say that since nobody ever pokes
at sysctl's it doesn't matter if its a little more expensive and iterate
everything.
Also; if you always keep everything up-to-date, you can avoid doing that
duplicate accounting.
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Peter Zijlstra @ 2019-01-23 9:16 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122153315.dhjl67sgpu74hmqv@e110439-lin>
On Tue, Jan 22, 2019 at 03:33:15PM +0000, Patrick Bellasi wrote:
> On 22-Jan 15:57, Peter Zijlstra wrote:
> > On Tue, Jan 22, 2019 at 02:01:15PM +0000, Patrick Bellasi wrote:
> > > Yes, I would say we have two options:
> > >
> > > 1) SCHED_FLAG_KEEP_POLICY enforces all the scheduling class specific
> > > attributes, but cross class attributes (e.g. uclamp)
> > >
> > > 2) add SCHED_KEEP_NICE, SCHED_KEEP_PRIO, and SCED_KEEP_PARAMS
> > > and use them in the if conditions in D)
> >
> > So the current KEEP_POLICY basically provides sched_setparam(), and
>
> But it's not exposed user-space.
Correct; not until your first patch indeed.
> > given we have that as a syscall, that is supposedly a useful
> > functionality.
>
> For uclamp is definitively useful to change clamps without the need to
> read beforehand the current policy params and use them in a following
> set syscall... which is racy pattern.
Right; but my argument was mostly that if sched_setparam() is a useful
interface, a 'pure' KEEP_POLICY would be too and your (1) looses that.
> > And I suppose the UTIL_CLAMP is !KEEP_UTIL; we could go either way
> > around with that flag.
>
> What about getting rid of the racy case above by exposing userspace
> only the new UTIL_CLAMP and, on:
>
> sched_setscheduler(flags: UTIL_CLAMP)
>
> we enforce the other two flags from the syscall:
>
> ---8<---
> SYSCALL_DEFINE3(sched_setattr)
> if (attr.sched_flags & SCHED_FLAG_KEEP_POLICY) {
> attr.sched_policy = SETPARAM_POLICY;
> attr.sched_flags |= (KEEP_POLICY|KEEP_PARAMS);
> }
> ---8<---
>
> This will not make possible to change class and set flags in one go,
> but honestly that's likely a very limited use-case, isn't it ?
So I must admit to not seeing much use for sched_setparam() (and its
equivalents) myself, but given it is an existing interface, I also think
it would be nice to cover that functionality in the sched_setattr()
call.
That is; I know of userspace priority-ceiling implementations using
sched_setparam(), but I don't see any reason why that wouldn't also work
with sched_setscheduler() (IOW always also set the policy).
> > > In both cases the goal should be to return from code block D).
> >
> > I don't think so; we really do want to 'goto change' for util changes
> > too I think. Why duplicate part of that logic?
>
> But that will force a dequeue/enqueue... isn't too much overhead just
> to change a clamp value?
These syscalls aren't what I consider fast paths anyway. However, there
are people that rely on the scheduler syscalls not to schedule
themselves, or rather be non-blocking (see for example that prio-ceiling
implementation).
And in that respect the newly introduced uclamp_mutex does appear to be
a problem.
Also; do you expect these clamp values to be changed often?
> Perhaps we can also end up with some wired
s/wired/weird/, right?
> side-effects like the task being preempted ?
Nothing worse than any other random reschedule would cause.
> Consider also that the uclamp_task_update_active() added by this patch
> not only has lower overhead but it will be use also by cgroups where
> we want to force update all the tasks on a cgroup's clamp change.
I haven't gotten that far; but I would prefer not to have two different
'change' paths in __sched_setscheduler().
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Patrick Bellasi @ 2019-01-22 18:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Tejun Heo,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122171314.GS27931@hirez.programming.kicks-ass.net>
On 22-Jan 18:13, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:05AM +0000, Patrick Bellasi wrote:
> > @@ -342,11 +350,24 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> > return;
> > sg_cpu->iowait_boost_pending = true;
> >
> > + /*
> > + * Boost FAIR tasks only up to the CPU clamped utilization.
> > + *
> > + * Since DL tasks have a much more advanced bandwidth control, it's
> > + * safe to assume that IO boost does not apply to those tasks.
>
> I'm not buying that argument. IO-boost isn't related to b/w management.
>
> IO-boot is more about compensating for hidden dependencies, and those
> don't get less hidden for using a different scheduling class.
>
> Now, arguably DL should not be doing IO in the first place, but that's a
> whole different discussion.
My understanding is that IOBoost is there to help tasks doing many
and _frequent_ IO operations, which are relatively _not so much_
computational intensive on the CPU.
Those tasks generate a small utilization and, without IOBoost, will be
executed at a lower frequency and will add undesired latency on
triggering the next IO operation.
Isn't mainly that the reason for it?
IO operations have also to be _frequent_ since we don't got to max OPP
at the very first wakeup from IO. We double frequency and get to max
only if we have a stable stream of IO operations.
IMHO, it makes perfectly sense to use DL for these kind of operations
but I would expect that, since you care about latency we should come
up with a proper description of the required bandwidth... eventually
accounting for an additional headroom to compensate for "hidden
dependencies"... without relaying on a quite dummy policy like
IOBoost to get our DL tasks working.
At the end, DL is now quite good in driving the freq as high has it
needs... and by closing userspace feedback loops it can also
compensate for all sort of fluctuations and noise... as demonstrated
by Alessio during last OSPM:
http://retis.sssup.it/luca/ospm-summit/2018/Downloads/OSPM_deadline_audio.pdf
> > + * Instead, since RT tasks are not utilization clamped, we don't want
> > + * to apply clamping on IO boost while there is blocked RT
> > + * utilization.
> > + */
> > + max_boost = sg_cpu->iowait_boost_max;
> > + if (!cpu_util_rt(cpu_rq(sg_cpu->cpu)))
> > + max_boost = uclamp_util(cpu_rq(sg_cpu->cpu), max_boost);
> > +
> > /* Double the boost at each request */
> > if (sg_cpu->iowait_boost) {
> > sg_cpu->iowait_boost <<= 1;
> > - if (sg_cpu->iowait_boost > sg_cpu->iowait_boost_max)
> > - sg_cpu->iowait_boost = sg_cpu->iowait_boost_max;
> > + if (sg_cpu->iowait_boost > max_boost)
> > + sg_cpu->iowait_boost = max_boost;
> > return;
> > }
>
> Hurmph... so I'm not sold on this bit.
If a task is not clamped we execute it at its required utilization or
even max frequency in case of wakeup from IO.
When a task is util_max clamped instead, we are saying that we don't
care to run it above the specified clamp value and, if possible, we
should run it below that capacity level.
If that's the case, why this clamping hints should not be enforced on
IO wakeups too?
At the end it's still a user-space decision, we basically allow
userspace to defined what's the max IO boost they like to get.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox