* 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
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Peter Zijlstra @ 2019-01-22 17:13 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-9-patrick.bellasi@arm.com>
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.
> + * 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.
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Patrick Bellasi @ 2019-01-22 15:45 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: <20190122152132.GR27931@hirez.programming.kicks-ass.net>
On 22-Jan 16:21, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:05AM +0000, Patrick Bellasi wrote:
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -218,8 +218,15 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> > * CFS tasks and we use the same metric to track the effective
> > * utilization (PELT windows are synchronized) we can directly add them
> > * to obtain the CPU's actual utilization.
> > + *
> > + * CFS utilization can be boosted or capped, depending on utilization
> > + * clamp constraints requested by currently RUNNABLE tasks.
> > + * When there are no CFS RUNNABLE tasks, clamps are released and
> > + * frequency will be gracefully reduced with the utilization decay.
> > */
> > - util = util_cfs;
> > + util = (type == ENERGY_UTIL)
> > + ? util_cfs
> > + : uclamp_util(rq, util_cfs);
>
> That's pretty horrible; what's wrong with:
>
> util = util_cfs;
> if (type == FREQUENCY_UTIL)
> util = uclamp_util(rq, util);
>
> That should generate the same code, but is (IMO) far easier to read.
Yes, right... and that's also the pattern we end up with the
following patch on RT integration.
However, as suggested by Rafael, I'll squash these two patches
together and we will get rid of the above for free ;)
> > util += cpu_util_rt(rq);
> >
> > dl_util = cpu_util_dl(rq);
--
#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-22 15:41 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: <20190122151317.GH13777@hirez.programming.kicks-ass.net>
On 22-Jan 16:13, Peter Zijlstra wrote:
> On Tue, Jan 22, 2019 at 02:43:29PM +0000, Patrick Bellasi wrote:
> > On 22-Jan 14:56, Peter Zijlstra wrote:
> > > On Tue, Jan 15, 2019 at 10:15:04AM +0000, Patrick Bellasi wrote:
> > >
> > > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > > index 84294925d006..c8f391d1cdc5 100644
> > > > --- a/include/linux/sched.h
> > > > +++ b/include/linux/sched.h
> > > > @@ -625,6 +625,11 @@ struct uclamp_se {
> > > > unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > > > unsigned int mapped : 1;
> > > > unsigned int active : 1;
> > > > + /* Clamp bucket and value actually used by a RUNNABLE task */
> > > > + struct {
> > > > + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> > > > + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > > > + } effective;
> > >
> > > I am confuzled by this thing.. so uclamp_se already has a value,bucket,
> > > which per the prior code is the effective one.
> > >
> > > Now; I think I see why you want another value; you need the second to
> > > store the original value for when the system limits change and we must
> > > re-evaluate.
> >
> > Yes, that's one reason, the other one being to properly support
> > CGroup when we add them in the following patches.
> >
> > Effective will always track the value/bucket in which the task has
> > been refcounted at enqueue time and it depends on the aggregated
> > value.
>
> > > Should you not update all tasks?
> >
> > That's true, but that's also an expensive operation, that's why now
> > I'm doing only lazy updates at next enqueue time.
>
> Aaah, so you refcount on the original value, which allows you to skip
> fixing up all tasks. I missed that bit.
Right, effective is always tracking the bucket we refcounted at
enqueue time.
We can still argue that, the moment we change a clamp, a task should
be updated without waiting for a dequeue/enqueue cycle.
IMO, that could be a limitation only for tasks which never sleep, but
that's a very special case.
Instead, as you'll see, in the cgroup integration we force update all
RUNNABLE tasks. Although that's expensive, since we are in the domain
of the "delegation model" and "containers resources control", there
it's probably more worth to pay than here.
> > 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.
--
#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-22 15: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: <20190122145742.GQ27931@hirez.programming.kicks-ass.net>
On 22-Jan 15:57, Peter Zijlstra wrote:
> On Tue, Jan 22, 2019 at 02:01:15PM +0000, Patrick Bellasi wrote:
> > On 22-Jan 14:28, Peter Zijlstra wrote:
> > > On Tue, Jan 22, 2019 at 10:43:05AM +0000, Patrick Bellasi wrote:
> > > > On 22-Jan 10:37, Peter Zijlstra wrote:
> > >
> > > > > Sure, I get that. What I don't get is why you're adding that (2) here.
> > > > > Like said, __sched_setscheduler() already does a dequeue/enqueue under
> > > > > rq->lock, which should already take care of that.
> > > >
> > > > Oh, ok... got it what you mean now.
> > > >
> > > > With:
> > > >
> > > > [PATCH v6 01/16] sched/core: Allow sched_setattr() to use the current policy
> > > > <20190115101513.2822-2-patrick.bellasi@arm.com>
> > > >
> > > > we can call __sched_setscheduler() with:
> > > >
> > > > attr->sched_flags & SCHED_FLAG_KEEP_POLICY
> > > >
> > > > whenever we want just to change the clamp values of a task without
> > > > changing its class. Thus, we can end up returning from
> > > > __sched_setscheduler() without doing an actual dequeue/enqueue.
> > >
> > > I don't see that happening.. when KEEP_POLICY we set attr.sched_policy =
> > > SETPARAM_POLICY. That is then checked again in __setscheduler_param(),
> > > which is in the middle of that dequeue/enqueue.
> >
> > Yes, I think I've forgot a check before we actually dequeue the task.
> >
> > The current code does:
> >
> > ---8<---
> > SYSCALL_DEFINE3(sched_setattr)
> >
> > // A) request to keep the same policy
> > if (attr.sched_flags & SCHED_FLAG_KEEP_POLICY)
> > attr.sched_policy = SETPARAM_POLICY;
> >
> > sched_setattr()
> > // B) actually enforce the same policy
> > if (policy < 0)
> > policy = oldpolicy = p->policy;
> >
> > // C) tune the clamp values
> > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
> > retval = __setscheduler_uclamp(p, attr);
> >
> > // D) tune attributes if policy is the same
> > if (unlikely(policy == p->policy))
> > if (fair_policy(policy) && attr->sched_nice != task_nice(p))
> > goto change;
> > if (rt_policy(policy) && attr->sched_priority != p->rt_priority)
> > goto change;
> > if (dl_policy(policy) && dl_param_changed(p, attr))
> > goto change;
>
> if (util_changed)
> goto change;
>
> ?
>
> > return 0;
> > change:
> >
> > // E) dequeue/enqueue task
> > ---8<---
> >
> > So, probably in D) I've missed a check on SCHED_FLAG_KEEP_POLICY to
> > enforce a return in that case...
> >
> > > Also, and this might be 'broken', SETPARAM_POLICY _does_ reset all the
> > > other attributes, it only preserves policy, but it will (re)set nice
> > > level for example (see that same function).
> >
> > Mmm... right... my bad! :/
> >
> > > So maybe we want to introduce another (few?) FLAG_KEEP flag(s) that
> > > preserve the other bits; I'm thinking at least KEEP_PARAM and KEEP_UTIL
> > > or something.
> >
> > 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.
> 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.
> Also, NICE/PRIO/DL* is all the same thing and depends on the policy,
> KEEP_PARAM should cover the lot
Right, that makes sense.
> 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 ?
> > 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? Perhaps we can also end up with some wired
side-effects like the task being preempted ?
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.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Peter Zijlstra @ 2019-01-22 15:21 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-9-patrick.bellasi@arm.com>
On Tue, Jan 15, 2019 at 10:15:05AM +0000, Patrick Bellasi wrote:
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -218,8 +218,15 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> * CFS tasks and we use the same metric to track the effective
> * utilization (PELT windows are synchronized) we can directly add them
> * to obtain the CPU's actual utilization.
> + *
> + * CFS utilization can be boosted or capped, depending on utilization
> + * clamp constraints requested by currently RUNNABLE tasks.
> + * When there are no CFS RUNNABLE tasks, clamps are released and
> + * frequency will be gracefully reduced with the utilization decay.
> */
> - util = util_cfs;
> + util = (type == ENERGY_UTIL)
> + ? util_cfs
> + : uclamp_util(rq, util_cfs);
That's pretty horrible; what's wrong with:
util = util_cfs;
if (type == FREQUENCY_UTIL)
util = uclamp_util(rq, util);
That should generate the same code, but is (IMO) far easier to read.
> util += cpu_util_rt(rq);
>
> dl_util = cpu_util_dl(rq);
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Quentin Perret @ 2019-01-22 15:14 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122150137.fp4g4kdng2qpy6qx@e110439-lin>
On Tuesday 22 Jan 2019 at 15:01:37 (+0000), Patrick Bellasi wrote:
> > I'm not saying it's useful, I'm saying userspace can decide to do that
> > if it thinks it is a good idea. The default should be min_cap = 1024 for
> > RT, no questions. But you _can_ change it at runtime if you want to.
> > That's my point. And doing that basically provides the same behaviour as
> > what we have right now in terms of EAS calculation (but it changes the
> > freq selection obviously) which is why I'm not fundamentally opposed to
> > your patch.
>
> Well, I think it's tricky to say whether the current or new approach
> is better... it probably depends on the use-case.
Agreed.
> > So in short, I'm fine with the behavioural change, but please at least
> > mention it somewhere :-)
>
> Anyway... agree, it's just that to add some documentation I need to
> get what you are pointing out ;)
>
> Will come up with some additional text to be added to the changelog
Sounds good.
> Maybe we can add a more detailed explanation of the different
> behaviors you can get in the EAS documentation which is coming to
> mainline ?
Yeah, if you feel like it, I guess that won't hurt :-)
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH v6 07/16] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-01-22 15:13 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: <20190122144329.ziimv6fejwvky7yb@e110439-lin>
On Tue, Jan 22, 2019 at 02:43:29PM +0000, Patrick Bellasi wrote:
> On 22-Jan 14:56, Peter Zijlstra wrote:
> > On Tue, Jan 15, 2019 at 10:15:04AM +0000, Patrick Bellasi wrote:
> >
> > > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > > index 84294925d006..c8f391d1cdc5 100644
> > > --- a/include/linux/sched.h
> > > +++ b/include/linux/sched.h
> > > @@ -625,6 +625,11 @@ struct uclamp_se {
> > > unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > > unsigned int mapped : 1;
> > > unsigned int active : 1;
> > > + /* Clamp bucket and value actually used by a RUNNABLE task */
> > > + struct {
> > > + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> > > + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > > + } effective;
> >
> > I am confuzled by this thing.. so uclamp_se already has a value,bucket,
> > which per the prior code is the effective one.
> >
> > Now; I think I see why you want another value; you need the second to
> > store the original value for when the system limits change and we must
> > re-evaluate.
>
> Yes, that's one reason, the other one being to properly support
> CGroup when we add them in the following patches.
>
> Effective will always track the value/bucket in which the task has
> been refcounted at enqueue time and it depends on the aggregated
> value.
> > Should you not update all tasks?
>
> That's true, but that's also an expensive operation, that's why now
> I'm doing only lazy updates at next enqueue time.
Aaah, so you refcount on the original value, which allows you to skip
fixing up all tasks. I missed that bit.
> Do you think that could be acceptable?
Think so, it's a sysctl poke, 'nobody' ever does that.
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Patrick Bellasi @ 2019-01-22 15:01 UTC (permalink / raw)
To: Quentin Perret
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122143909.pmlqyhcjshyomrbw@queper01-lin>
On 22-Jan 14:39, Quentin Perret wrote:
> On Tuesday 22 Jan 2019 at 14:26:06 (+0000), Patrick Bellasi wrote:
> > On 22-Jan 13:29, Quentin Perret wrote:
> > > On Tuesday 22 Jan 2019 at 12:45:46 (+0000), Patrick Bellasi wrote:
> > > > On 22-Jan 12:13, Quentin Perret wrote:
> > > > > On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> > > > > > The Energy Aware Scheduler (AES) estimates the energy impact of waking
[...]
> > > Ah, sorry, I guess my message was misleading. I'm saying this is
> > > changing the way _EAS_ deals with RT tasks. Right now we don't actually
> > > consider the RT-go-to-max thing at all in the EAS prediction. Your
> > > patch is changing that AFAICT. It actually changes the way EAS sees RT
> > > tasks even without uclamp ...
> >
> > Lemme see if I get it right.
> >
> > Currently, whenever we look at CPU utilization for ENERGY_UTIL, we
> > always use cpu_util_rt() for RT tasks:
> >
> > ---8<---
> > util = util_cfs;
> > util += cpu_util_rt(rq);
> > util += dl_util;
> > ---8<---
> >
> > Thus, even when RT tasks are RUNNABLE, we don't always assume the CPU
> > running at the max capacity but just whatever is the aggregated
> > utilization across all the classes.
> >
> > With uclamp, we have:
> >
> > ---8<---
> > util = cpu_util_rt(rq) + util_cfs;
> > if (type == FREQUENCY_UTIL)
> > util = uclamp_util_with(rq, util, p);
> > dl_util = cpu_util_dl(rq);
> > if (type == ENERGY_UTIL)
> > util += dl_util;
> > ---8<---
> >
> > So, I would say that, in terms of ENERGY_UTIL we do the same both
> > w/ and w/o uclamp. Isn't it?
>
> Yes but now you use FREQUENCY_UTIL for computing 'max_util' in the EAS
> prediction.
Right, I overlook that "little" detail... :/
> Let's take an example. You have a perf domain with two CPUs. One CPU is
> busy running a RT task, the other CPU runs a CFS task. Right now in
> compute_energy() we only use ENERGY_UTIL, so 'max_util' ends up being
> the max between the utilization of the two tasks. So we don't predict
> we're going to max freq.
+1
> With your patch, we use FREQUENCY_UTIL to compute 'max_util', so we
> _will_ predict that we're going to max freq.
Right, with the default conf yes.
> And we will do that even if CONFIG_UCLAMP_TASK=n.
While this should not happen, as I wrote in the RT integration patch,
that's happening because I'm missing some compilation guard or
similar. In this configurations we should always go to max... will
look into that.
> The default EAS calculation will be different with your patch when there
> are runnable RT tasks in the system. This needs to be documented, I
> think.
Sure...
> > > But I'm not hostile to the idea since it's possible to enable uclamp and
> > > set the min cap to 0 for RT if you want. So there is a story there.
> > > However, I think this needs be documented somewhere, at the very least.
> >
> > The only difference I see is that the actual frequency could be
> > different (lower then max) when a clamped RT task is RUNNABLE.
> >
> > Are you worried that running RT on a lower freq could have side
> > effects on the estimated busy time the CPU ?
> >
> > I also still don't completely get why you say it could be useful to
> > "set the min cap to 0 for RT if you want"
>
> I'm not saying it's useful, I'm saying userspace can decide to do that
> if it thinks it is a good idea. The default should be min_cap = 1024 for
> RT, no questions. But you _can_ change it at runtime if you want to.
> That's my point. And doing that basically provides the same behaviour as
> what we have right now in terms of EAS calculation (but it changes the
> freq selection obviously) which is why I'm not fundamentally opposed to
> your patch.
Well, I think it's tricky to say whether the current or new approach
is better... it probably depends on the use-case.
> So in short, I'm fine with the behavioural change, but please at least
> mention it somewhere :-)
Anyway... agree, it's just that to add some documentation I need to
get what you are pointing out ;)
Will come up with some additional text to be added to the changelog
Maybe we can add a more detailed explanation of the different
behaviors you can get in the EAS documentation which is coming to
mainline ?
> Thanks,
> Quentin
--
#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: Peter Zijlstra @ 2019-01-22 14:57 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: <20190122140115.twtx646vewm757ca@e110439-lin>
On Tue, Jan 22, 2019 at 02:01:15PM +0000, Patrick Bellasi wrote:
> On 22-Jan 14:28, Peter Zijlstra wrote:
> > On Tue, Jan 22, 2019 at 10:43:05AM +0000, Patrick Bellasi wrote:
> > > On 22-Jan 10:37, Peter Zijlstra wrote:
> >
> > > > Sure, I get that. What I don't get is why you're adding that (2) here.
> > > > Like said, __sched_setscheduler() already does a dequeue/enqueue under
> > > > rq->lock, which should already take care of that.
> > >
> > > Oh, ok... got it what you mean now.
> > >
> > > With:
> > >
> > > [PATCH v6 01/16] sched/core: Allow sched_setattr() to use the current policy
> > > <20190115101513.2822-2-patrick.bellasi@arm.com>
> > >
> > > we can call __sched_setscheduler() with:
> > >
> > > attr->sched_flags & SCHED_FLAG_KEEP_POLICY
> > >
> > > whenever we want just to change the clamp values of a task without
> > > changing its class. Thus, we can end up returning from
> > > __sched_setscheduler() without doing an actual dequeue/enqueue.
> >
> > I don't see that happening.. when KEEP_POLICY we set attr.sched_policy =
> > SETPARAM_POLICY. That is then checked again in __setscheduler_param(),
> > which is in the middle of that dequeue/enqueue.
>
> Yes, I think I've forgot a check before we actually dequeue the task.
>
> The current code does:
>
> ---8<---
> SYSCALL_DEFINE3(sched_setattr)
>
> // A) request to keep the same policy
> if (attr.sched_flags & SCHED_FLAG_KEEP_POLICY)
> attr.sched_policy = SETPARAM_POLICY;
>
> sched_setattr()
> // B) actually enforce the same policy
> if (policy < 0)
> policy = oldpolicy = p->policy;
>
> // C) tune the clamp values
> if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
> retval = __setscheduler_uclamp(p, attr);
>
> // D) tune attributes if policy is the same
> if (unlikely(policy == p->policy))
> if (fair_policy(policy) && attr->sched_nice != task_nice(p))
> goto change;
> if (rt_policy(policy) && attr->sched_priority != p->rt_priority)
> goto change;
> if (dl_policy(policy) && dl_param_changed(p, attr))
> goto change;
if (util_changed)
goto change;
?
> return 0;
> change:
>
> // E) dequeue/enqueue task
> ---8<---
>
> So, probably in D) I've missed a check on SCHED_FLAG_KEEP_POLICY to
> enforce a return in that case...
>
> > Also, and this might be 'broken', SETPARAM_POLICY _does_ reset all the
> > other attributes, it only preserves policy, but it will (re)set nice
> > level for example (see that same function).
>
> Mmm... right... my bad! :/
>
> > So maybe we want to introduce another (few?) FLAG_KEEP flag(s) that
> > preserve the other bits; I'm thinking at least KEEP_PARAM and KEEP_UTIL
> > or something.
>
> 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
given we have that as a syscall, that is supposedly a useful
functionality.
Also, NICE/PRIO/DL* is all the same thing and depends on the policy,
KEEP_PARAM should cover the lot
And I suppose the UTIL_CLAMP is !KEEP_UTIL; we could go either way
around with that flag.
> 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?
^ permalink raw reply
* Re: [PATCH v6 07/16] sched/core: uclamp: Add system default clamps
From: Patrick Bellasi @ 2019-01-22 14:43 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: <20190122135644.GP27931@hirez.programming.kicks-ass.net>
On 22-Jan 14:56, Peter Zijlstra wrote:
> On Tue, Jan 15, 2019 at 10:15:04AM +0000, Patrick Bellasi wrote:
>
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index 84294925d006..c8f391d1cdc5 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -625,6 +625,11 @@ struct uclamp_se {
> > unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > unsigned int mapped : 1;
> > unsigned int active : 1;
> > + /* Clamp bucket and value actually used by a RUNNABLE task */
> > + struct {
> > + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> > + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> > + } effective;
>
> I am confuzled by this thing.. so uclamp_se already has a value,bucket,
> which per the prior code is the effective one.
>
> Now; I think I see why you want another value; you need the second to
> store the original value for when the system limits change and we must
> re-evaluate.
Yes, that's one reason, the other one being to properly support
CGroup when we add them in the following patches.
Effective will always track the value/bucket in which the task has
been refcounted at enqueue time and it depends on the aggregated
value.
> So why are you not adding something like:
>
> unsigned int orig_value : bits_per(SCHED_CAPACITY_SCALE);
Would say that can be enough if we decide to ditch the mapping and use
a linear mapping. In that case the value will always be enough to find
in which bucket a task has been accounted.
> > +unsigned int sysctl_sched_uclamp_util_min;
>
> > +unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
>
> > +static inline void
> > +uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> > + unsigned int *clamp_value, unsigned int *bucket_id)
> > +{
> > + /* Task specific clamp value */
> > + *clamp_value = p->uclamp[clamp_id].value;
> > + *bucket_id = p->uclamp[clamp_id].bucket_id;
> > +
> > + /* System default restriction */
> > + if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> > + *clamp_value > uclamp_default[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;
> > + }
> > +}
>
> That would then turn into something like:
>
> unsigned int high = READ_ONCE(sysctl_sched_uclamp_util_max);
> unsigned int low = READ_ONCE(sysctl_sched_uclamp_util_min);
>
> uclamp_se->orig_value = value;
> uclamp_se->value = clamp(value, low, high);
>
> And then determine bucket_id based on value.
Right... if I ditch the mapping that should work.
> > +int sched_uclamp_handler(struct ctl_table *table, int write,
> > + void __user *buffer, size_t *lenp,
> > + loff_t *ppos)
> > +{
> > + int old_min, old_max;
> > + int result = 0;
> > +
> > + mutex_lock(&uclamp_mutex);
> > +
> > + old_min = sysctl_sched_uclamp_util_min;
> > + old_max = sysctl_sched_uclamp_util_max;
> > +
> > + result = proc_dointvec(table, write, buffer, lenp, ppos);
> > + if (result)
> > + goto undo;
> > + if (!write)
> > + goto done;
> > +
> > + if (sysctl_sched_uclamp_util_min > sysctl_sched_uclamp_util_max ||
> > + sysctl_sched_uclamp_util_max > SCHED_CAPACITY_SCALE) {
> > + result = -EINVAL;
> > + goto undo;
> > + }
> > +
> > + if (old_min != sysctl_sched_uclamp_util_min) {
> > + uclamp_bucket_inc(NULL, &uclamp_default[UCLAMP_MIN],
> > + UCLAMP_MIN, sysctl_sched_uclamp_util_min);
> > + }
> > + if (old_max != sysctl_sched_uclamp_util_max) {
> > + uclamp_bucket_inc(NULL, &uclamp_default[UCLAMP_MAX],
> > + UCLAMP_MAX, sysctl_sched_uclamp_util_max);
> > + }
>
> Should you not update all tasks?
That's true, but that's also an expensive operation, that's why now
I'm doing only lazy updates at next enqueue time.
Do you think that could be acceptable?
Perhaps I can sanity check all the CPU to ensure that they all have a
current clamp value within the new enforced range. This kind-of
anticipate the idea to have an in-kernel API which has higher priority
and allows to set clamp values across all the CPUs...
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Quentin Perret @ 2019-01-22 14:39 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122142606.gc5hnc5pzefblegw@e110439-lin>
On Tuesday 22 Jan 2019 at 14:26:06 (+0000), Patrick Bellasi wrote:
> On 22-Jan 13:29, Quentin Perret wrote:
> > On Tuesday 22 Jan 2019 at 12:45:46 (+0000), Patrick Bellasi wrote:
> > > On 22-Jan 12:13, Quentin Perret wrote:
> > > > On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> > > > > The Energy Aware Scheduler (AES) estimates the energy impact of waking
> > >
> > > [...]
> > >
> > > > > + for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
> > > > > + cfs_util = cpu_util_next(cpu, p, dst_cpu);
> > > > > +
> > > > > + /*
> > > > > + * Busy time computation: utilization clamping is not
> > > > > + * required since the ratio (sum_util / cpu_capacity)
> > > > > + * is already enough to scale the EM reported power
> > > > > + * consumption at the (eventually clamped) cpu_capacity.
> > > > > + */
> > > >
> > > > Right.
> > > >
> > > > > + sum_util += schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> > > > > + ENERGY_UTIL, NULL);
> > > > > +
> > > > > + /*
> > > > > + * Performance domain frequency: utilization clamping
> > > > > + * must be considered since it affects the selection
> > > > > + * of the performance domain frequency.
> > > > > + */
> > > >
> > > > So that actually affects the way we deal with RT I think. I assume the
> > > > idea is to say if you don't want to reflect the RT-go-to-max-freq thing
> > > > in EAS (which is what we do now) you should set the min cap for RT to 0.
> > > > Is that correct ?
> > >
> > > By default configuration, RT tasks still go to max when uclamp is
> > > enabled, since they get a util_min=1024.
> > >
> > > If we want to save power on RT tasks, we can set a smaller util_min...
> > > but not necessarily 0. A util_min=0 for RT tasks means to use just
> > > cpu_util_rt() for that class.
> >
> > Ah, sorry, I guess my message was misleading. I'm saying this is
> > changing the way _EAS_ deals with RT tasks. Right now we don't actually
> > consider the RT-go-to-max thing at all in the EAS prediction. Your
> > patch is changing that AFAICT. It actually changes the way EAS sees RT
> > tasks even without uclamp ...
>
> Lemme see if I get it right.
>
> Currently, whenever we look at CPU utilization for ENERGY_UTIL, we
> always use cpu_util_rt() for RT tasks:
>
> ---8<---
> util = util_cfs;
> util += cpu_util_rt(rq);
> util += dl_util;
> ---8<---
>
> Thus, even when RT tasks are RUNNABLE, we don't always assume the CPU
> running at the max capacity but just whatever is the aggregated
> utilization across all the classes.
>
> With uclamp, we have:
>
> ---8<---
> util = cpu_util_rt(rq) + util_cfs;
> if (type == FREQUENCY_UTIL)
> util = uclamp_util_with(rq, util, p);
> dl_util = cpu_util_dl(rq);
> if (type == ENERGY_UTIL)
> util += dl_util;
> ---8<---
>
> So, I would say that, in terms of ENERGY_UTIL we do the same both
> w/ and w/o uclamp. Isn't it?
Yes but now you use FREQUENCY_UTIL for computing 'max_util' in the EAS
prediction.
Let's take an example. You have a perf domain with two CPUs. One CPU is
busy running a RT task, the other CPU runs a CFS task. Right now in
compute_energy() we only use ENERGY_UTIL, so 'max_util' ends up being
the max between the utilization of the two tasks. So we don't predict
we're going to max freq.
With your patch, we use FREQUENCY_UTIL to compute 'max_util', so we
_will_ predict that we're going to max freq. And we will do that even if
CONFIG_UCLAMP_TASK=n.
The default EAS calculation will be different with your patch when there
are runnable RT tasks in the system. This needs to be documented, I
think.
> > But I'm not hostile to the idea since it's possible to enable uclamp and
> > set the min cap to 0 for RT if you want. So there is a story there.
> > However, I think this needs be documented somewhere, at the very least.
>
> The only difference I see is that the actual frequency could be
> different (lower then max) when a clamped RT task is RUNNABLE.
>
> Are you worried that running RT on a lower freq could have side
> effects on the estimated busy time the CPU ?
>
> I also still don't completely get why you say it could be useful to
> "set the min cap to 0 for RT if you want"
I'm not saying it's useful, I'm saying userspace can decide to do that
if it thinks it is a good idea. The default should be min_cap = 1024 for
RT, no questions. But you _can_ change it at runtime if you want to.
That's my point. And doing that basically provides the same behaviour as
what we have right now in terms of EAS calculation (but it changes the
freq selection obviously) which is why I'm not fundamentally opposed to
your patch.
So in short, I'm fine with the behavioural change, but please at least
mention it somewhere :-)
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Patrick Bellasi @ 2019-01-22 14:26 UTC (permalink / raw)
To: Quentin Perret
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122132944.sfxlnpc3xeft4rqd@queper01-lin>
On 22-Jan 13:29, Quentin Perret wrote:
> On Tuesday 22 Jan 2019 at 12:45:46 (+0000), Patrick Bellasi wrote:
> > On 22-Jan 12:13, Quentin Perret wrote:
> > > On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> > > > The Energy Aware Scheduler (AES) estimates the energy impact of waking
> >
> > [...]
> >
> > > > + for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
> > > > + cfs_util = cpu_util_next(cpu, p, dst_cpu);
> > > > +
> > > > + /*
> > > > + * Busy time computation: utilization clamping is not
> > > > + * required since the ratio (sum_util / cpu_capacity)
> > > > + * is already enough to scale the EM reported power
> > > > + * consumption at the (eventually clamped) cpu_capacity.
> > > > + */
> > >
> > > Right.
> > >
> > > > + sum_util += schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> > > > + ENERGY_UTIL, NULL);
> > > > +
> > > > + /*
> > > > + * Performance domain frequency: utilization clamping
> > > > + * must be considered since it affects the selection
> > > > + * of the performance domain frequency.
> > > > + */
> > >
> > > So that actually affects the way we deal with RT I think. I assume the
> > > idea is to say if you don't want to reflect the RT-go-to-max-freq thing
> > > in EAS (which is what we do now) you should set the min cap for RT to 0.
> > > Is that correct ?
> >
> > By default configuration, RT tasks still go to max when uclamp is
> > enabled, since they get a util_min=1024.
> >
> > If we want to save power on RT tasks, we can set a smaller util_min...
> > but not necessarily 0. A util_min=0 for RT tasks means to use just
> > cpu_util_rt() for that class.
>
> Ah, sorry, I guess my message was misleading. I'm saying this is
> changing the way _EAS_ deals with RT tasks. Right now we don't actually
> consider the RT-go-to-max thing at all in the EAS prediction. Your
> patch is changing that AFAICT. It actually changes the way EAS sees RT
> tasks even without uclamp ...
Lemme see if I get it right.
Currently, whenever we look at CPU utilization for ENERGY_UTIL, we
always use cpu_util_rt() for RT tasks:
---8<---
util = util_cfs;
util += cpu_util_rt(rq);
util += dl_util;
---8<---
Thus, even when RT tasks are RUNNABLE, we don't always assume the CPU
running at the max capacity but just whatever is the aggregated
utilization across all the classes.
With uclamp, we have:
---8<---
util = cpu_util_rt(rq) + util_cfs;
if (type == FREQUENCY_UTIL)
util = uclamp_util_with(rq, util, p);
dl_util = cpu_util_dl(rq);
if (type == ENERGY_UTIL)
util += dl_util;
---8<---
So, I would say that, in terms of ENERGY_UTIL we do the same both
w/ and w/o uclamp. Isn't it?
> But I'm not hostile to the idea since it's possible to enable uclamp and
> set the min cap to 0 for RT if you want. So there is a story there.
> However, I think this needs be documented somewhere, at the very least.
The only difference I see is that the actual frequency could be
different (lower then max) when a clamped RT task is RUNNABLE.
Are you worried that running RT on a lower freq could have side
effects on the estimated busy time the CPU ?
I also still don't completely get why you say it could be useful to
"set the min cap to 0 for RT if you want"
IMO this will be an even bigger difference wrt mainline, since the RT
tasks will never have a granted minimum freq but just whatever
utilization we measure for them.
--
#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-22 14:01 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: <20190122132817.GG13777@hirez.programming.kicks-ass.net>
On 22-Jan 14:28, Peter Zijlstra wrote:
> On Tue, Jan 22, 2019 at 10:43:05AM +0000, Patrick Bellasi wrote:
> > On 22-Jan 10:37, Peter Zijlstra wrote:
>
> > > Sure, I get that. What I don't get is why you're adding that (2) here.
> > > Like said, __sched_setscheduler() already does a dequeue/enqueue under
> > > rq->lock, which should already take care of that.
> >
> > Oh, ok... got it what you mean now.
> >
> > With:
> >
> > [PATCH v6 01/16] sched/core: Allow sched_setattr() to use the current policy
> > <20190115101513.2822-2-patrick.bellasi@arm.com>
> >
> > we can call __sched_setscheduler() with:
> >
> > attr->sched_flags & SCHED_FLAG_KEEP_POLICY
> >
> > whenever we want just to change the clamp values of a task without
> > changing its class. Thus, we can end up returning from
> > __sched_setscheduler() without doing an actual dequeue/enqueue.
>
> I don't see that happening.. when KEEP_POLICY we set attr.sched_policy =
> SETPARAM_POLICY. That is then checked again in __setscheduler_param(),
> which is in the middle of that dequeue/enqueue.
Yes, I think I've forgot a check before we actually dequeue the task.
The current code does:
---8<---
SYSCALL_DEFINE3(sched_setattr)
// A) request to keep the same policy
if (attr.sched_flags & SCHED_FLAG_KEEP_POLICY)
attr.sched_policy = SETPARAM_POLICY;
sched_setattr()
// B) actually enforce the same policy
if (policy < 0)
policy = oldpolicy = p->policy;
// C) tune the clamp values
if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
retval = __setscheduler_uclamp(p, attr);
// D) tune attributes if policy is the same
if (unlikely(policy == p->policy))
if (fair_policy(policy) && attr->sched_nice != task_nice(p))
goto change;
if (rt_policy(policy) && attr->sched_priority != p->rt_priority)
goto change;
if (dl_policy(policy) && dl_param_changed(p, attr))
goto change;
return 0;
change:
// E) dequeue/enqueue task
---8<---
So, probably in D) I've missed a check on SCHED_FLAG_KEEP_POLICY to
enforce a return in that case...
> Also, and this might be 'broken', SETPARAM_POLICY _does_ reset all the
> other attributes, it only preserves policy, but it will (re)set nice
> level for example (see that same function).
Mmm... right... my bad! :/
> So maybe we want to introduce another (few?) FLAG_KEEP flag(s) that
> preserve the other bits; I'm thinking at least KEEP_PARAM and KEEP_UTIL
> or something.
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)
In both cases the goal should be to return from code block D).
What do you prefer?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 07/16] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-01-22 13:56 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-8-patrick.bellasi@arm.com>
On Tue, Jan 15, 2019 at 10:15:04AM +0000, Patrick Bellasi wrote:
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 84294925d006..c8f391d1cdc5 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -625,6 +625,11 @@ struct uclamp_se {
> unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> unsigned int mapped : 1;
> unsigned int active : 1;
> + /* Clamp bucket and value actually used by a RUNNABLE task */
> + struct {
> + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> + } effective;
I am confuzled by this thing.. so uclamp_se already has a value,bucket,
which per the prior code is the effective one.
Now; I think I see why you want another value; you need the second to
store the original value for when the system limits change and we must
re-evaluate.
So why are you not adding something like:
unsigned int orig_value : bits_per(SCHED_CAPACITY_SCALE);
> +unsigned int sysctl_sched_uclamp_util_min;
> +unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
> +static inline void
> +uclamp_effective_get(struct task_struct *p, unsigned int clamp_id,
> + unsigned int *clamp_value, unsigned int *bucket_id)
> +{
> + /* Task specific clamp value */
> + *clamp_value = p->uclamp[clamp_id].value;
> + *bucket_id = p->uclamp[clamp_id].bucket_id;
> +
> + /* System default restriction */
> + if (unlikely(*clamp_value < uclamp_default[UCLAMP_MIN].value ||
> + *clamp_value > uclamp_default[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;
> + }
> +}
That would then turn into something like:
unsigned int high = READ_ONCE(sysctl_sched_uclamp_util_max);
unsigned int low = READ_ONCE(sysctl_sched_uclamp_util_min);
uclamp_se->orig_value = value;
uclamp_se->value = clamp(value, low, high);
And then determine bucket_id based on value.
> +int sched_uclamp_handler(struct ctl_table *table, int write,
> + void __user *buffer, size_t *lenp,
> + loff_t *ppos)
> +{
> + int old_min, old_max;
> + int result = 0;
> +
> + mutex_lock(&uclamp_mutex);
> +
> + old_min = sysctl_sched_uclamp_util_min;
> + old_max = sysctl_sched_uclamp_util_max;
> +
> + result = proc_dointvec(table, write, buffer, lenp, ppos);
> + if (result)
> + goto undo;
> + if (!write)
> + goto done;
> +
> + if (sysctl_sched_uclamp_util_min > sysctl_sched_uclamp_util_max ||
> + sysctl_sched_uclamp_util_max > SCHED_CAPACITY_SCALE) {
> + result = -EINVAL;
> + goto undo;
> + }
> +
> + if (old_min != sysctl_sched_uclamp_util_min) {
> + uclamp_bucket_inc(NULL, &uclamp_default[UCLAMP_MIN],
> + UCLAMP_MIN, sysctl_sched_uclamp_util_min);
> + }
> + if (old_max != sysctl_sched_uclamp_util_max) {
> + uclamp_bucket_inc(NULL, &uclamp_default[UCLAMP_MAX],
> + UCLAMP_MAX, sysctl_sched_uclamp_util_max);
> + }
Should you not update all tasks?
^ permalink raw reply
* Re: Can we drop upstream Linux x32 support?
From: Enrico Weigelt, metux IT consult @ 2019-01-22 13:34 UTC (permalink / raw)
To: Thomas Schöbel-Theuer, Andy Lutomirski, X86 ML, LKML,
Linux API, H. Peter Anvin, Peter Zijlstra, Borislav Petkov,
Florian Weimer, Mike Frysinger, H. J. Lu, Rich Felker, x32,
Arnd Bergmann, Will Deacon, Catalin Marinas, Linus Torvalds
In-Reply-To: <6577ac4f-524c-37f4-a4d0-6eb94ec7d9a5@schoebel-theuer.de>
On 14.12.18 22:16, Thomas Schöbel-Theuer wrote:
Hi,
> Currently, we have a few thousands of servers relying on 32bit ABIs in> some thousands of VMs and/or containers of various types (LXC,
OpenVZ,> etc).
Similar w/ my clients, but in Industrie/Embedded/i4.0 world. We have
thousands of devices in the field, that need support for at least a
decade.
Right now, I'm working on a new device, that - even having a 64bit cpu -
will still run 32bit userland for various reasons (memory footprint
is one of them).
Even on the data center side, where we have *a lot* of containers
(eg. container-based microservices on minimal distros like alpine
or ptxdist - as well as many different, often highly specialized,
legacy applications), 32bit userland indeed is a major factor.
Dropping it would have huge economic consequences:
* massive costs for re-deployment / migration of thousands of different
applications
* field roll of thousands of different embedded / industrial devices,
as a major cpu arch
* huge increase in memory consumption and io load in container-
based microservice environments
* loosing x86 for a huge portion of the embedded / small devices world
I don't even dare to estimate the economic damage. (not even speaking
about reputation loss of the Linux community)
> Here is my private opinion, not speaking for 1&1: at some point the> future, 32bit userspace support needs to be dropped anyway, somewhen
in> future. This is inevitable in the very long term.
Very, very, long term. Just the devices I'm coping w/ will have
remaining lifetime of at least 10..15 years. That's a really long time
in IT world. Nobody knows whether x86 at all will play a big role then.
> 1) please release / declare a new LTS kernel, with upstream support for
> at least 5 years (as usual). Currently, only 4.4 and 4.9 are marked as
> LTS. Either mark another existing stable kernel as LTS, or a future one.
That might be okay in enterprise world (even though, here I'm regularily
stumbling across much older code in production), but in industrial
world, the product lifetimes are much longer - 20+yrs years
usualstandard.are pretty common.
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Quentin Perret @ 2019-01-22 13:29 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122124546.njrpmykzbjpztd6u@e110439-lin>
On Tuesday 22 Jan 2019 at 12:45:46 (+0000), Patrick Bellasi wrote:
> On 22-Jan 12:13, Quentin Perret wrote:
> > On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> > > The Energy Aware Scheduler (AES) estimates the energy impact of waking
>
> [...]
>
> > > + for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
> > > + cfs_util = cpu_util_next(cpu, p, dst_cpu);
> > > +
> > > + /*
> > > + * Busy time computation: utilization clamping is not
> > > + * required since the ratio (sum_util / cpu_capacity)
> > > + * is already enough to scale the EM reported power
> > > + * consumption at the (eventually clamped) cpu_capacity.
> > > + */
> >
> > Right.
> >
> > > + sum_util += schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> > > + ENERGY_UTIL, NULL);
> > > +
> > > + /*
> > > + * Performance domain frequency: utilization clamping
> > > + * must be considered since it affects the selection
> > > + * of the performance domain frequency.
> > > + */
> >
> > So that actually affects the way we deal with RT I think. I assume the
> > idea is to say if you don't want to reflect the RT-go-to-max-freq thing
> > in EAS (which is what we do now) you should set the min cap for RT to 0.
> > Is that correct ?
>
> By default configuration, RT tasks still go to max when uclamp is
> enabled, since they get a util_min=1024.
>
> If we want to save power on RT tasks, we can set a smaller util_min...
> but not necessarily 0. A util_min=0 for RT tasks means to use just
> cpu_util_rt() for that class.
Ah, sorry, I guess my message was misleading. I'm saying this is
changing the way _EAS_ deals with RT tasks. Right now we don't actually
consider the RT-go-to-max thing at all in the EAS prediction. Your
patch is changing that AFAICT. It actually changes the way EAS sees RT
tasks even without uclamp ...
But I'm not hostile to the idea since it's possible to enable uclamp and
set the min cap to 0 for RT if you want. So there is a story there.
However, I think this needs be documented somewhere, at the very least.
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH v6 05/16] sched/core: uclamp: Update CPU's refcount on clamp changes
From: Peter Zijlstra @ 2019-01-22 13: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: <20190122104305.6vjx37muqsxm536t@e110439-lin>
On Tue, Jan 22, 2019 at 10:43:05AM +0000, Patrick Bellasi wrote:
> On 22-Jan 10:37, Peter Zijlstra wrote:
> > Sure, I get that. What I don't get is why you're adding that (2) here.
> > Like said, __sched_setscheduler() already does a dequeue/enqueue under
> > rq->lock, which should already take care of that.
>
> Oh, ok... got it what you mean now.
>
> With:
>
> [PATCH v6 01/16] sched/core: Allow sched_setattr() to use the current policy
> <20190115101513.2822-2-patrick.bellasi@arm.com>
>
> we can call __sched_setscheduler() with:
>
> attr->sched_flags & SCHED_FLAG_KEEP_POLICY
>
> whenever we want just to change the clamp values of a task without
> changing its class. Thus, we can end up returning from
> __sched_setscheduler() without doing an actual dequeue/enqueue.
I don't see that happening.. when KEEP_POLICY we set attr.sched_policy =
SETPARAM_POLICY. That is then checked again in __setscheduler_param(),
which is in the middle of that dequeue/enqueue.
Also, and this might be 'broken', SETPARAM_POLICY _does_ reset all the
other attributes, it only preserves policy, but it will (re)set nice
level for example (see that same function).
So maybe we want to introduce another (few?) FLAG_KEEP flag(s) that
preserve the other bits; I'm thinking at least KEEP_PARAM and KEEP_UTIL
or something.
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Patrick Bellasi @ 2019-01-22 12:45 UTC (permalink / raw)
To: Quentin Perret
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122121321.r6mv23ao57uut3t7@queper01-lin>
On 22-Jan 12:13, Quentin Perret wrote:
> On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> > The Energy Aware Scheduler (AES) estimates the energy impact of waking
[...]
> > + for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
> > + cfs_util = cpu_util_next(cpu, p, dst_cpu);
> > +
> > + /*
> > + * Busy time computation: utilization clamping is not
> > + * required since the ratio (sum_util / cpu_capacity)
> > + * is already enough to scale the EM reported power
> > + * consumption at the (eventually clamped) cpu_capacity.
> > + */
>
> Right.
>
> > + sum_util += schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> > + ENERGY_UTIL, NULL);
> > +
> > + /*
> > + * Performance domain frequency: utilization clamping
> > + * must be considered since it affects the selection
> > + * of the performance domain frequency.
> > + */
>
> So that actually affects the way we deal with RT I think. I assume the
> idea is to say if you don't want to reflect the RT-go-to-max-freq thing
> in EAS (which is what we do now) you should set the min cap for RT to 0.
> Is that correct ?
By default configuration, RT tasks still go to max when uclamp is
enabled, since they get a util_min=1024.
If we want to save power on RT tasks, we can set a smaller util_min...
but not necessarily 0. A util_min=0 for RT tasks means to use just
cpu_util_rt() for that class.
> I'm fine with this conceptually but maybe the specific case of RT should
> be mentioned somewhere in the commit message or so ? I think it's
> important to say that clearly since this patch changes the default
> behaviour.
Default behavior for RT should not be affected. While a capping is
possible for those tasks... where do you see issues ?
Here we are just figuring out what's the capacity the task will run
at, if we will have clamped RT tasks will not be the max but: is that
a problem ?
> > + cpu_util = schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> > + FREQUENCY_UTIL,
> > + cpu == dst_cpu ? p : NULL);
> > + max_util = max(max_util, cpu_util);
> > }
> >
> > energy += em_pd_energy(pd->em_pd, max_util, sum_util);
>
> Thanks,
> Quentin
--
#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-22 12:37 UTC (permalink / raw)
To: Quentin Perret
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190122123007.dkavjh73w223pa6y@queper01-lin>
On 22-Jan 12:30, Quentin Perret wrote:
> On Tuesday 15 Jan 2019 at 10:15:06 (+0000), Patrick Bellasi wrote:
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 520ee2b785e7..38a05a4f78cc 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -201,9 +201,6 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> > unsigned long dl_util, util, irq;
> > struct rq *rq = cpu_rq(cpu);
> >
> > - if (type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt))
> > - return max;
> > -
> > /*
> > * Early check to see if IRQ/steal time saturates the CPU, can be
> > * because of inaccuracies in how we track these -- see
> > @@ -219,15 +216,19 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> > * utilization (PELT windows are synchronized) we can directly add them
> > * to obtain the CPU's actual utilization.
> > *
> > - * CFS utilization can be boosted or capped, depending on utilization
> > - * clamp constraints requested by currently RUNNABLE tasks.
> > + * CFS and RT utilization can be boosted or capped, depending on
> > + * utilization clamp constraints requested by currently RUNNABLE
> > + * tasks.
> > * When there are no CFS RUNNABLE tasks, clamps are released and
> > * frequency will be gracefully reduced with the utilization decay.
> > */
> > - 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);
>
> So with this we don't go to max to anymore for CONFIG_UCLAMP_TASK=n no ?
Mmm... good point!
I need to guard this chagen for the !CONFIG_UCLAMP_TASK case!
>
> Thanks,
> Quentin
Cheers Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 09/16] sched/cpufreq: uclamp: Add utilization clamping for RT tasks
From: Quentin Perret @ 2019-01-22 12:30 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, 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 Tuesday 15 Jan 2019 at 10:15:06 (+0000), Patrick Bellasi wrote:
> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 520ee2b785e7..38a05a4f78cc 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -201,9 +201,6 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> unsigned long dl_util, util, irq;
> struct rq *rq = cpu_rq(cpu);
>
> - if (type == FREQUENCY_UTIL && rt_rq_is_runnable(&rq->rt))
> - return max;
> -
> /*
> * Early check to see if IRQ/steal time saturates the CPU, can be
> * because of inaccuracies in how we track these -- see
> @@ -219,15 +216,19 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> * utilization (PELT windows are synchronized) we can directly add them
> * to obtain the CPU's actual utilization.
> *
> - * CFS utilization can be boosted or capped, depending on utilization
> - * clamp constraints requested by currently RUNNABLE tasks.
> + * CFS and RT utilization can be boosted or capped, depending on
> + * utilization clamp constraints requested by currently RUNNABLE
> + * tasks.
> * When there are no CFS RUNNABLE tasks, clamps are released and
> * frequency will be gracefully reduced with the utilization decay.
> */
> - 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);
So with this we don't go to max to anymore for CONFIG_UCLAMP_TASK=n no ?
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH v6 11/16] sched/fair: Add uclamp support to energy_compute()
From: Quentin Perret @ 2019-01-22 12:13 UTC (permalink / raw)
To: Patrick Bellasi
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190115101513.2822-12-patrick.bellasi@arm.com>
On Tuesday 15 Jan 2019 at 10:15:08 (+0000), Patrick Bellasi wrote:
> The Energy Aware Scheduler (AES) estimates the energy impact of waking
s/AES/EAS :-)
[...]
> + for_each_cpu_and(cpu, pd_mask, cpu_online_mask) {
> + cfs_util = cpu_util_next(cpu, p, dst_cpu);
> +
> + /*
> + * Busy time computation: utilization clamping is not
> + * required since the ratio (sum_util / cpu_capacity)
> + * is already enough to scale the EM reported power
> + * consumption at the (eventually clamped) cpu_capacity.
> + */
Right.
> + sum_util += schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> + ENERGY_UTIL, NULL);
> +
> + /*
> + * Performance domain frequency: utilization clamping
> + * must be considered since it affects the selection
> + * of the performance domain frequency.
> + */
So that actually affects the way we deal with RT I think. I assume the
idea is to say if you don't want to reflect the RT-go-to-max-freq thing
in EAS (which is what we do now) you should set the min cap for RT to 0.
Is that correct ?
I'm fine with this conceptually but maybe the specific case of RT should
be mentioned somewhere in the commit message or so ? I think it's
important to say that clearly since this patch changes the default
behaviour.
> + cpu_util = schedutil_cpu_util(cpu, cfs_util, cpu_cap,
> + FREQUENCY_UTIL,
> + cpu == dst_cpu ? p : NULL);
> + max_util = max(max_util, cpu_util);
> }
>
> energy += em_pd_energy(pd->em_pd, max_util, sum_util);
Thanks,
Quentin
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Patrick Bellasi @ 2019-01-22 11:27 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux Kernel Mailing List, Linux PM, Linux API,
Ingo Molnar, Peter Zijlstra, 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: <CAJZ5v0j2NQY_gKJOAy=rP5_1Dk9TODKNhW0vuvsynTN3BUmYaQ@mail.gmail.com>
On 22-Jan 12:04, Rafael J. Wysocki wrote:
> On Tue, Jan 22, 2019 at 12:02 PM Patrick Bellasi
> <patrick.bellasi@arm.com> wrote:
> >
> > On 22-Jan 11:37, Rafael J. Wysocki wrote:
> > > On Tuesday, January 15, 2019 11:15:05 AM CET Patrick Bellasi wrote:
>
> Merge the two together, please.
Ok, will do in v7, thanks.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Rafael J. Wysocki @ 2019-01-22 11:04 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Rafael J. Wysocki, Linux Kernel Mailing List, Linux PM, Linux API,
Ingo Molnar, Peter Zijlstra, 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: <20190122110205.lpk6o2jdfe3mttjr@e110439-lin>
On Tue, Jan 22, 2019 at 12:02 PM Patrick Bellasi
<patrick.bellasi@arm.com> wrote:
>
> On 22-Jan 11:37, Rafael J. Wysocki wrote:
> > On Tuesday, January 15, 2019 11:15:05 AM CET Patrick Bellasi wrote:
[cut]
> >
> > IMO it would be better to combine this patch with the next one.
>
> Main reason was to better document in the changelog what we do for the
> two different classes...
>
> > At least some things in it I was about to ask about would go away
> > then. :-)
>
> ... but if it creates confusion I can certainly merge them.
>
> Or maybe clarify better in this patch what's not clear: may I ask what
> were your questions ?
>
> > Besides, I don't really see a reason for the split here.
>
> Was mainly to make the changes required for RT more self-contained.
>
> For that class only, not for FAIR, we have additional code in the
> following patch which add uclamp_default_perf which are system
> defaults used to track/account tasks requesting the maximum frequency.
>
> Again, I can either better clarify the above patch or just merge the
> two together: what do you prefer ?
Merge the two together, please.
^ permalink raw reply
* Re: [PATCH v6 08/16] sched/cpufreq: uclamp: Add utilization clamping for FAIR tasks
From: Patrick Bellasi @ 2019-01-22 11:02 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
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: <3006911.57lVBuUGX3@aspire.rjw.lan>
On 22-Jan 11:37, Rafael J. Wysocki wrote:
> On Tuesday, January 15, 2019 11:15:05 AM CET Patrick Bellasi wrote:
> > Each time a frequency update is required via schedutil, a frequency is
> > selected to (possibly) satisfy the utilization reported by each
> > scheduling class. However, when utilization clamping is in use, the
> > frequency selection should consider userspace utilization clamping
> > hints. This will allow, for example, to:
> >
> > - boost tasks which are directly affecting the user experience
> > by running them at least at a minimum "requested" frequency
> >
> > - cap low priority tasks not directly affecting the user experience
> > by running them only up to a maximum "allowed" frequency
> >
> > These constraints are meant to support a per-task based tuning of the
> > frequency selection thus supporting a fine grained definition of
> > performance boosting vs energy saving strategies in kernel space.
> >
> > Add support to clamp the utilization and IOWait boost of RUNNABLE FAIR
> > tasks within the boundaries defined by their aggregated utilization
> > clamp constraints.
> > Based on the max(min_util, max_util) of each task, max-aggregated the
> > CPU clamp value in a way to give the boosted tasks the performance they
> > need when they happen to be co-scheduled with other capped tasks.
> >
> > Signed-off-by: Patrick Bellasi <patrick.bellasi@arm.com>
> > Cc: Ingo Molnar <mingo@redhat.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > ---
> > Changes in v6:
> > Message-ID: <20181107113849.GC14309@e110439-lin>
> > - sanity check util_max >= util_min
> > Others:
> > - wholesale s/group/bucket/
> > - wholesale s/_{get,put}/_{inc,dec}/ to match refcount APIs
> > ---
> > kernel/sched/cpufreq_schedutil.c | 27 ++++++++++++++++++++++++---
> > kernel/sched/sched.h | 23 +++++++++++++++++++++++
> > 2 files changed, 47 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 033ec7c45f13..520ee2b785e7 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -218,8 +218,15 @@ unsigned long schedutil_freq_util(int cpu, unsigned long util_cfs,
> > * CFS tasks and we use the same metric to track the effective
> > * utilization (PELT windows are synchronized) we can directly add them
> > * to obtain the CPU's actual utilization.
> > + *
> > + * CFS utilization can be boosted or capped, depending on utilization
> > + * clamp constraints requested by currently RUNNABLE tasks.
> > + * When there are no CFS RUNNABLE tasks, clamps are released and
> > + * frequency will be gracefully reduced with the utilization decay.
> > */
> > - util = util_cfs;
> > + util = (type == ENERGY_UTIL)
> > + ? util_cfs
> > + : uclamp_util(rq, util_cfs);
> > util += cpu_util_rt(rq);
> >
> > dl_util = cpu_util_dl(rq);
> > @@ -327,6 +334,7 @@ static void sugov_iowait_boost(struct sugov_cpu *sg_cpu, u64 time,
> > unsigned int flags)
> > {
> > bool set_iowait_boost = flags & SCHED_CPUFREQ_IOWAIT;
> > + unsigned int max_boost;
> >
> > /* Reset boost if the CPU appears to have been idle enough */
> > if (sg_cpu->iowait_boost &&
> > @@ -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.
> > + * 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;
> > }
> >
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index b7f3ee8ba164..95d62a2a0b44 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -2267,6 +2267,29 @@ static inline unsigned int uclamp_none(int clamp_id)
> > return SCHED_CAPACITY_SCALE;
> > }
> >
> > +#ifdef CONFIG_UCLAMP_TASK
> > +static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> > +{
> > + unsigned int min_util = READ_ONCE(rq->uclamp[UCLAMP_MIN].value);
> > + unsigned int max_util = READ_ONCE(rq->uclamp[UCLAMP_MAX].value);
> > +
> > + /*
> > + * Since CPU's {min,max}_util clamps are MAX aggregated considering
> > + * RUNNABLE tasks with _different_ clamps, we can end up with an
> > + * invertion, which we can fix at usage time.
> > + */
> > + if (unlikely(min_util >= max_util))
> > + return min_util;
> > +
> > + return clamp(util, min_util, max_util);
> > +}
> > +#else /* CONFIG_UCLAMP_TASK */
> > +static inline unsigned int uclamp_util(struct rq *rq, unsigned int util)
> > +{
> > + return util;
> > +}
> > +#endif /* CONFIG_UCLAMP_TASK */
> > +
> > #ifdef arch_scale_freq_capacity
> > # ifndef arch_scale_freq_invariant
> > # define arch_scale_freq_invariant() true
> >
>
> IMO it would be better to combine this patch with the next one.
Main reason was to better document in the changelog what we do for the
two different classes...
> At least some things in it I was about to ask about would go away
> then. :-)
... but if it creates confusion I can certainly merge them.
Or maybe clarify better in this patch what's not clear: may I ask what
were your questions ?
> Besides, I don't really see a reason for the split here.
Was mainly to make the changes required for RT more self-contained.
For that class only, not for FAIR, we have additional code in the
following patch which add uclamp_default_perf which are system
defaults used to track/account tasks requesting the maximum frequency.
Again, I can either better clarify the above patch or just merge the
two together: what do you prefer ?
--
#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