* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 16:12 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: <20190313134022.GB5922@hirez.programming.kicks-ass.net>
On 13-Mar 14:40, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
> > +{
> > + return clamp_value / UCLAMP_BUCKET_DELTA;
> > +}
> > +
> > +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> > +{
> > + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
>
> return clamp_value - (clamp_value % UCLAMP_BUCKET_DELTA);
>
> might generate better code; just a single division, instead of a div and
> mult.
Wondering if compilers cannot do these optimizations... but yes, looks
cool and will do it in v8, thanks.
> > +}
> > +
> > +static inline unsigned int uclamp_none(int clamp_id)
> > +{
> > + if (clamp_id == UCLAMP_MIN)
> > + return 0;
> > + return SCHED_CAPACITY_SCALE;
> > +}
> > +
> > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > +{
> > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > + unsigned int max_value = uclamp_none(clamp_id);
> > + unsigned int bucket_id;
> > +
> > + /*
> > + * Both min and max clamps are MAX aggregated, thus the topmost
> > + * bucket with some tasks defines the rq's clamp value.
> > + */
> > + bucket_id = UCLAMP_BUCKETS;
> > + do {
> > + --bucket_id;
> > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> > + continue;
> > + max_value = bucket[bucket_id].value;
> > + break;
>
> If you flip the if condition the code will be nicer.
>
> > + } while (bucket_id);
>
> But you can also use a for loop:
>
> for (i = UCLAMP_BUCKETS-1; i>=0; i--) {
> if (rq->uclamp[clamp_id].bucket[i].tasks) {
> max_value = bucket[i].value;
> break;
> }
> }
Yes, the for looks better, but perhaps like that:
unsigned int bucket_id = UCLAMP_BUCKETS;
/*
* Both min and max clamps are MAX aggregated, thus the topmost
* bucket with some tasks defines the rq's clamp value.
*/
for (; bucket_id >= 0; --bucket_id) {
if (!bucket[bucket_id].tasks)
continue;
max_value = bucket[bucket_id].value;
break;
}
... just to save a {} block.
> > + WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> > +}
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 15:59 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: <20190313135238.GC5922@hirez.programming.kicks-ass.net>
On 13-Mar 14:52, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +/*
> > + * When a task is enqueued on a rq, the clamp bucket currently defined by the
> > + * task's uclamp::bucket_id is reference counted on that rq. This also
> > + * immediately updates the rq's clamp value if required.
> > + *
> > + * Since tasks know their specific value requested from user-space, we track
> > + * within each bucket the maximum value for tasks refcounted in that bucket.
> > + * This provide a further aggregation (local clamping) which allows to track
> > + * within each bucket the exact "requested" clamp value whenever all tasks
> > + * RUNNABLE in that bucket require the same clamp.
> > + */
> > +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> > + unsigned int clamp_id)
> > +{
> > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
> > +
> > + rq->uclamp[clamp_id].bucket[bucket_id].tasks++;
> > +
> > + /*
> > + * Local clamping: rq's buckets always track the max "requested"
> > + * clamp value from all RUNNABLE tasks in that bucket.
> > + */
> > + tsk_clamp = p->uclamp[clamp_id].value;
> > + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> > + rq->uclamp[clamp_id].bucket[bucket_id].value = max(bkt_clamp, tsk_clamp);
>
> So, if I read this correct:
>
> - here we track a max value in a bucket,
>
> > + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> > + WRITE_ONCE(rq->uclamp[clamp_id].value, max(rq_clamp, tsk_clamp));
> > +}
> > +
> > +/*
> > + * When a task is dequeued from a rq, the clamp bucket reference counted by
> > + * the task is released. If this is the last task reference counting the rq's
> > + * max active clamp value, then the rq's clamp value is updated.
> > + * Both the tasks reference counter and the rq's cached clamp values are
> > + * expected to be always valid, if we detect they are not we skip the updates,
> > + * enforce a consistent state and warn.
> > + */
> > +static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq,
> > + unsigned int clamp_id)
> > +{
> > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > + unsigned int rq_clamp, bkt_clamp;
> > +
> > + SCHED_WARN_ON(!rq->uclamp[clamp_id].bucket[bucket_id].tasks);
> > + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> > + rq->uclamp[clamp_id].bucket[bucket_id].tasks--;
> > +
> > + /*
> > + * Keep "local clamping" simple and accept to (possibly) overboost
> > + * still RUNNABLE tasks in the same bucket.
> > + */
> > + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> > + return;
>
> (Oh man, I hope that generates semi sane code; long live CSE passes I
> suppose)
What do you mean ?
> But we never decrement that bkt_clamp value on dequeue.
We decrement the bkt_clamp value only when the bucket becomes empty
and thus we pass the condition above. That's what the comment above is
there to call out.
> > + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> > +
> > + /* The rq's clamp value is expected to always track the max */
> > + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> > + SCHED_WARN_ON(bkt_clamp > rq_clamp);
> > + if (bkt_clamp >= rq_clamp) {
>
> head hurts, this reads ==, how can this ever not be so?
Never, given the current code, that's just defensive programming.
If in the future the accounting should be accidentally broken by some
refactoring we warn and fix the corrupted data structures at first
chance.
Is that so bad?
> > + /*
> > + * Reset rq's clamp bucket value to its nominal value whenever
> > + * there are anymore RUNNABLE tasks refcounting it.
>
> -ENOPARSE
That's related to the comment above, when you say we don't decrement
the bkt_clamp.
Because of backetization, we potentially end up tracking tasks with
different requested clamp values in the same bucket.
For example, with 20% bucket size, we can have:
Task1: util_min=25%
Task2: util_min=35%
accounted in the same bucket.
This ensure that while they are both running we boost 35%. If Task1
should run longer than Task2, Task1 will be "overboosted" until its
end. The bucket value will be reset to 20% (nominal value) when both
tasks are idle.
> > + */
> > + rq->uclamp[clamp_id].bucket[bucket_id].value =
> > + uclamp_bucket_value(rq_clamp);
>
> But basically you decrement the bucket value to the nominal value.
Yes, at this point we know there are no more tasks in this bucket and
we reset its value.
>
> > + uclamp_rq_update(rq, clamp_id);
> > + }
> > +}
>
> Given all that, what is to stop the bucket value to climbing to
> uclamp_bucket_value(+1)-1 and staying there (provided there's someone
> runnable)?
Nothing... but that's an expected consequence of bucketization.
> Why are we doing this... ?
You can either decide to:
a) always boost tasks to just the bucket nominal value
thus always penalizing both Task1 and Task2 of the example above
b) always boost tasks to the bucket "max" value
thus always overboosting both Task1 and Task2 of the example above
The solution above instead has a very good property: in systems
where you have only few and well defined clamp values we always
provide the exact boost.
For example, if your system requires only 23% and 47% boost values
(totally random numbers), then you can always get the exact boost
required using just 3 bucksts or ~33% size each.
In systems where you don't know which boost values you will have, you
can still defined the maximum overboost granularity you accept for
each task by just tuning the number of clamp groups. For example, with
20 groups you can have a 5% max overboost.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 15:28 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: <20190313140603.GD5922@hirez.programming.kicks-ass.net>
On 13-Mar 15:06, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +static void __init init_uclamp(void)
> > +{
> > + unsigned int clamp_id;
> > + int cpu;
> > +
> > + for_each_possible_cpu(cpu)
> > + memset(&cpu_rq(cpu)->uclamp, 0, sizeof(struct uclamp_rq));
> > +
>
> Is that really needed? Doesn't rq come from .bss ?
Will check better... I've just assumed not since in sched_init() we
already have tons of rq's related zero initializations.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 15:23 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: <20190313140925.GE5922@hirez.programming.kicks-ass.net>
On 13-Mar 15:09, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +static inline unsigned int uclamp_none(int clamp_id)
> > +{
> > + if (clamp_id == UCLAMP_MIN)
> > + return 0;
> > + return SCHED_CAPACITY_SCALE;
> > +}
> > +
> > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > +{
> > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > + unsigned int max_value = uclamp_none(clamp_id);
>
> That's 1024 for uclamp_max
>
> > + unsigned int bucket_id;
> > +
> > + /*
> > + * Both min and max clamps are MAX aggregated, thus the topmost
> > + * bucket with some tasks defines the rq's clamp value.
> > + */
> > + bucket_id = UCLAMP_BUCKETS;
> > + do {
> > + --bucket_id;
> > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> > + continue;
> > + max_value = bucket[bucket_id].value;
>
> but this will then _lower_ it. That's not a MAX aggregate.
For uclamp_max we want max_value=1024 when there are no active tasks,
which means: no max clamp enforced on CFS/RT "idle" cpus.
If instead there are active RT/CFS tasks then we want the clamp value
of the max group, which means: MAX aggregate active clamps.
That's what the code above does and the comment says.
> > + break;
> > + } while (bucket_id);
> > +
> > + WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> > +}
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 15:15 UTC (permalink / raw)
To: Dietmar Eggemann
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Tejun Heo, Rafael J . Wysocki, Vincent Guittot, Viresh Kumar,
Paul Turner, Quentin Perret, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <21171fa0-7fd5-ebbf-dd48-d6668ed563af@arm.com>
On 12-Mar 13:52, Dietmar Eggemann wrote:
> On 2/8/19 11:05 AM, Patrick Bellasi wrote:
>
> [...]
>
> > +config UCLAMP_BUCKETS_COUNT
> > + int "Number of supported utilization clamp buckets"
> > + range 5 20
> > + default 5
> > + depends on UCLAMP_TASK
> > + help
> > + Defines the number of clamp buckets to use. The range of each bucket
> > + will be SCHED_CAPACITY_SCALE/UCLAMP_BUCKETS_COUNT. The higher the
> > + number of clamp buckets the finer their granularity and the higher
> > + the precision of clamping aggregation and tracking at run-time.
> > +
> > + For example, with the default configuration we will have 5 clamp
> > + buckets tracking 20% utilization each. A 25% boosted tasks will be
> > + refcounted in the [20..39]% bucket and will set the bucket clamp
> > + effective value to 25%.
> > + If a second 30% boosted task should be co-scheduled on the same CPU,
> > + that task will be refcounted in the same bucket of the first task and
> > + it will boost the bucket clamp effective value to 30%.
> > + The clamp effective value of a bucket is reset to its nominal value
> > + (20% in the example above) when there are anymore tasks refcounted in
>
> this sounds weird.
Why ?
>
> [...]
>
> > +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> > +{
> > + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
> > +}
>
> Soemthing like uclamp_bucket_nominal_value() should be clearer.
Maybe... can update it in v8
> > +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> > +{
> > + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> > + unsigned int max_value = uclamp_none(clamp_id);
> > + unsigned int bucket_id;
>
> unsigned int bucket_id = UCLAMP_BUCKETS;
>
> > +
> > + /*
> > + * Both min and max clamps are MAX aggregated, thus the topmost
> > + * bucket with some tasks defines the rq's clamp value.
> > + */
> > + bucket_id = UCLAMP_BUCKETS;
>
> to get rid of this line?
I put it on a different line as a justfication for the loop variable
initialization described in the comment above.
>
> > + do {
> > + --bucket_id;
> > + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
>
> if (!bucket[bucket_id].tasks)
Right... that's some leftover from the last refactoring!
[...]
> > + * within each bucket the exact "requested" clamp value whenever all tasks
> > + * RUNNABLE in that bucket require the same clamp.
> > + */
> > +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> > + unsigned int clamp_id)
> > +{
> > + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> > + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
>
> Wouldn't it be easier to have a pointer to the task's and rq's uclamp
> structure as well to the bucket?
>
> - unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> + struct uclamp_se *uc_se = &p->uclamp[clamp_id];
> + struct uclamp_rq *uc_rq = &rq->uclamp[clamp_id];
> + struct uclamp_bucket *bucket = &uc_rq->bucket[uc_se->bucket_id];
I think I went back/forth a couple of times in using pointer or the
extended version, which both have pros and cons.
I personally prefer the pointers as you suggest but I've got the
impression in the past that since everybody cleared "basic C trainings"
it's not so difficult to read the code above too.
> The code in uclamp_rq_inc_id() and uclamp_rq_dec_id() for example becomes
> much more readable.
Agree... let's try to switch once again in v8 and see ;)
> [...]
>
> > struct sched_class {
> > const struct sched_class *next;
> > +#ifdef CONFIG_UCLAMP_TASK
> > + int uclamp_enabled;
> > +#endif
> > +
> > void (*enqueue_task) (struct rq *rq, struct task_struct *p, int flags);
> > void (*dequeue_task) (struct rq *rq, struct task_struct *p, int flags);
> > - void (*yield_task) (struct rq *rq);
> > - bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
> > void (*check_preempt_curr)(struct rq *rq, struct task_struct *p, int flags);
> > @@ -1685,7 +1734,6 @@ struct sched_class {
> > void (*set_curr_task)(struct rq *rq);
> > void (*task_tick)(struct rq *rq, struct task_struct *p, int queued);
> > void (*task_fork)(struct task_struct *p);
> > - void (*task_dead)(struct task_struct *p);
> > /*
> > * The switched_from() call is allowed to drop rq->lock, therefore we
> > @@ -1702,12 +1750,17 @@ struct sched_class {
> > void (*update_curr)(struct rq *rq);
> > + void (*yield_task) (struct rq *rq);
> > + bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
> > +
> > #define TASK_SET_GROUP 0
> > #define TASK_MOVE_GROUP 1
> > #ifdef CONFIG_FAIR_GROUP_SCHED
> > void (*task_change_group)(struct task_struct *p, int type);
> > #endif
> > +
> > + void (*task_dead)(struct task_struct *p);
>
> Why do you move yield_task, yield_to_task and task_dead here?
Since I'm adding a new field at the beginning of the struct, which is
used at enqueue/dequeue time, this is to ensure that all the
callbacks used in these paths are grouped together and don't fall
across a cache line... but yes, that's supposed to be a
micro-optimization which I can skip in this patch.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 03/15] sched/core: uclamp: Add system default clamps
From: Peter Zijlstra @ 2019-03-13 14:32 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: <20190208100554.32196-4-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:42AM +0000, Patrick Bellasi wrote:
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 45460e7a3eee..447261cd23ba 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -584,14 +584,32 @@ struct sched_dl_entity {
> * Utilization clamp for a scheduling entity
> * @value: clamp value "requested" by a se
> * @bucket_id: clamp bucket corresponding to the "requested" value
> + * @effective: clamp value and bucket actually "assigned" to the se
> + * @active: the se is currently refcounted in a rq's bucket
> *
> + * Both bucket_id and effective::bucket_id are the index of the clamp bucket
> + * matching the corresponding clamp value which are pre-computed and stored to
> + * avoid expensive integer divisions from the fast path.
> + *
> + * The active bit is set whenever a task has got an effective::value assigned,
> + * which can be different from the user requested clamp value. This allows to
> + * know a task is actually refcounting the rq's effective::bucket_id bucket.
> */
> struct uclamp_se {
> + /* Clamp value "requested" by a scheduling entity */
> unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> + unsigned int active : 1;
> + /*
> + * Clamp value "obtained" by a scheduling entity.
> + *
> + * This cache the actual clamp value, possibly enforced by system
> + * default clamps, a task is subject to while enqueued in a rq.
> + */
> + struct {
> + unsigned int value : bits_per(SCHED_CAPACITY_SCALE);
> + unsigned int bucket_id : bits_per(UCLAMP_BUCKETS);
> + } effective;
I still think that this effective thing is backwards.
The existing code already used @value and @bucket_id as 'effective' and
you're now changing all that again. This really doesn't make sense to
me.
Also; if you don't add it inside struct uclamp_se, but add a second
instance,
> };
> #endif /* CONFIG_UCLAMP_TASK */
>
> @@ -803,6 +811,70 @@ static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id,
> WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> }
>
> +/*
> + * The effective clamp bucket index of a task depends on, by increasing
> + * priority:
> + * - the task specific clamp value, when explicitly requested from userspace
> + * - the system default clamp value, defined by the sysadmin
> + *
> + * As a side effect, update the task's effective value:
> + * task_struct::uclamp::effective::value
> + * to represent the clamp value of the task effective bucket index.
> + */
> +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 */
> + *bucket_id = p->uclamp[clamp_id].bucket_id;
> + *clamp_value = p->uclamp[clamp_id].value;
> +
> + /* Always apply system default restrictions */
> + if (unlikely(*clamp_value > uclamp_default[clamp_id].value)) {
> + *clamp_value = uclamp_default[clamp_id].value;
> + *bucket_id = uclamp_default[clamp_id].bucket_id;
> + }
> +}
you can avoid horrors like this and simply return a struct uclamp_se by
value.
^ permalink raw reply
* Re: [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX
From: Peter Zijlstra @ 2019-03-13 14:12 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: <20190208100554.32196-3-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:
> +static inline void uclamp_idle_reset(struct rq *rq, unsigned int clamp_id,
> + unsigned int clamp_value)
> +{
> + /* Reset max-clamp retention only on idle exit */
> + if (!(rq->uclamp_flags & UCLAMP_FLAG_IDLE))
> + return;
> +
> + WRITE_ONCE(rq->uclamp[clamp_id].value, clamp_value);
> +
> + /*
> + * This function is called for both UCLAMP_MIN (before) and UCLAMP_MAX
> + * (after). The idle flag is reset only the second time, when we know
> + * that UCLAMP_MIN has been already updated.
Why do we care? That is, what is this comment trying to tell us.
> + */
> + if (clamp_id == UCLAMP_MAX)
> + rq->uclamp_flags &= ~UCLAMP_FLAG_IDLE;
> +}
^ permalink raw reply
* Re: [PATCH v7 02/15] sched/core: uclamp: Enforce last task UCLAMP_MAX
From: Peter Zijlstra @ 2019-03-13 14: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: <20190208100554.32196-3-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:41AM +0000, Patrick Bellasi wrote:
> +uclamp_idle_value(struct rq *rq, unsigned int clamp_id, unsigned int clamp_value)
> +{
> + /*
> + * Avoid blocked utilization pushing up the frequency when we go
> + * idle (which drops the max-clamp) by retaining the last known
> + * max-clamp.
> + */
> + if (clamp_id == UCLAMP_MAX) {
> + rq->uclamp_flags |= UCLAMP_FLAG_IDLE;
> + return clamp_value;
> + }
> +
> + return uclamp_none(UCLAMP_MIN);
That's a very complicated way or writing: return 0, right?
> +}
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 14:09 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: <20190208100554.32196-2-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> +static inline unsigned int uclamp_none(int clamp_id)
> +{
> + if (clamp_id == UCLAMP_MIN)
> + return 0;
> + return SCHED_CAPACITY_SCALE;
> +}
> +
> +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> +{
> + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> + unsigned int max_value = uclamp_none(clamp_id);
That's 1024 for uclamp_max
> + unsigned int bucket_id;
> +
> + /*
> + * Both min and max clamps are MAX aggregated, thus the topmost
> + * bucket with some tasks defines the rq's clamp value.
> + */
> + bucket_id = UCLAMP_BUCKETS;
> + do {
> + --bucket_id;
> + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> + continue;
> + max_value = bucket[bucket_id].value;
but this will then _lower_ it. That's not a MAX aggregate.
> + break;
> + } while (bucket_id);
> +
> + WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> +}
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 14:06 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: <20190208100554.32196-2-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> +static void __init init_uclamp(void)
> +{
> + unsigned int clamp_id;
> + int cpu;
> +
> + for_each_possible_cpu(cpu)
> + memset(&cpu_rq(cpu)->uclamp, 0, sizeof(struct uclamp_rq));
> +
Is that really needed? Doesn't rq come from .bss ?
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 13: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: <20190208100554.32196-2-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> +/*
> + * When a task is enqueued on a rq, the clamp bucket currently defined by the
> + * task's uclamp::bucket_id is reference counted on that rq. This also
> + * immediately updates the rq's clamp value if required.
> + *
> + * Since tasks know their specific value requested from user-space, we track
> + * within each bucket the maximum value for tasks refcounted in that bucket.
> + * This provide a further aggregation (local clamping) which allows to track
> + * within each bucket the exact "requested" clamp value whenever all tasks
> + * RUNNABLE in that bucket require the same clamp.
> + */
> +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> + unsigned int clamp_id)
> +{
> + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
> +
> + rq->uclamp[clamp_id].bucket[bucket_id].tasks++;
> +
> + /*
> + * Local clamping: rq's buckets always track the max "requested"
> + * clamp value from all RUNNABLE tasks in that bucket.
> + */
> + tsk_clamp = p->uclamp[clamp_id].value;
> + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> + rq->uclamp[clamp_id].bucket[bucket_id].value = max(bkt_clamp, tsk_clamp);
So, if I read this correct:
- here we track a max value in a bucket,
> + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> + WRITE_ONCE(rq->uclamp[clamp_id].value, max(rq_clamp, tsk_clamp));
> +}
> +
> +/*
> + * When a task is dequeued from a rq, the clamp bucket reference counted by
> + * the task is released. If this is the last task reference counting the rq's
> + * max active clamp value, then the rq's clamp value is updated.
> + * Both the tasks reference counter and the rq's cached clamp values are
> + * expected to be always valid, if we detect they are not we skip the updates,
> + * enforce a consistent state and warn.
> + */
> +static inline void uclamp_rq_dec_id(struct task_struct *p, struct rq *rq,
> + unsigned int clamp_id)
> +{
> + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> + unsigned int rq_clamp, bkt_clamp;
> +
> + SCHED_WARN_ON(!rq->uclamp[clamp_id].bucket[bucket_id].tasks);
> + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> + rq->uclamp[clamp_id].bucket[bucket_id].tasks--;
> +
> + /*
> + * Keep "local clamping" simple and accept to (possibly) overboost
> + * still RUNNABLE tasks in the same bucket.
> + */
> + if (likely(rq->uclamp[clamp_id].bucket[bucket_id].tasks))
> + return;
(Oh man, I hope that generates semi sane code; long live CSE passes I
suppose)
But we never decrement that bkt_clamp value on dequeue.
> + bkt_clamp = rq->uclamp[clamp_id].bucket[bucket_id].value;
> +
> + /* The rq's clamp value is expected to always track the max */
> + rq_clamp = READ_ONCE(rq->uclamp[clamp_id].value);
> + SCHED_WARN_ON(bkt_clamp > rq_clamp);
> + if (bkt_clamp >= rq_clamp) {
head hurts, this reads ==, how can this ever not be so?
> + /*
> + * Reset rq's clamp bucket value to its nominal value whenever
> + * there are anymore RUNNABLE tasks refcounting it.
-ENOPARSE
> + */
> + rq->uclamp[clamp_id].bucket[bucket_id].value =
> + uclamp_bucket_value(rq_clamp);
But basically you decrement the bucket value to the nominal value.
> + uclamp_rq_update(rq, clamp_id);
> + }
> +}
Given all that, what is to stop the bucket value to climbing to
uclamp_bucket_value(+1)-1 and staying there (provided there's someone
runnable)?
Why are we doing this... ?
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 13:40 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: <20190208100554.32196-2-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> +static inline unsigned int uclamp_bucket_id(unsigned int clamp_value)
> +{
> + return clamp_value / UCLAMP_BUCKET_DELTA;
> +}
> +
> +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> +{
> + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
return clamp_value - (clamp_value % UCLAMP_BUCKET_DELTA);
might generate better code; just a single division, instead of a div and
mult.
> +}
> +
> +static inline unsigned int uclamp_none(int clamp_id)
> +{
> + if (clamp_id == UCLAMP_MIN)
> + return 0;
> + return SCHED_CAPACITY_SCALE;
> +}
> +
> +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> +{
> + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> + unsigned int max_value = uclamp_none(clamp_id);
> + unsigned int bucket_id;
> +
> + /*
> + * Both min and max clamps are MAX aggregated, thus the topmost
> + * bucket with some tasks defines the rq's clamp value.
> + */
> + bucket_id = UCLAMP_BUCKETS;
> + do {
> + --bucket_id;
> + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
> + continue;
> + max_value = bucket[bucket_id].value;
> + break;
If you flip the if condition the code will be nicer.
> + } while (bucket_id);
But you can also use a for loop:
for (i = UCLAMP_BUCKETS-1; i>=0; i--) {
if (rq->uclamp[clamp_id].bucket[i].tasks) {
max_value = bucket[i].value;
break;
}
}
> + WRITE_ONCE(rq->uclamp[clamp_id].value, max_value);
> +}
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-13 11:37 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: <20190313081958.GA5996@hirez.programming.kicks-ass.net>
On 13-Mar 09:19, Peter Zijlstra wrote:
> On Tue, Mar 12, 2019 at 03:50:43PM +0000, Patrick Bellasi wrote:
> > On 12-Mar 16:20, Peter Zijlstra wrote:
> > > On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > > > +/* Integer ceil-rounded range for each bucket */
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> > > > +#define UCLAMP_BUCKET_DELTA ((SCHED_CAPACITY_SCALE / UCLAMP_BUCKETS) + 1)
>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> simply do not match.
Right, that don't match when UCLAMP_BUCKETS is a divider of
SCHED_CAPACITY_SCALE, i.e. when we use 8 or 16 buckets.
> > > Uhm, should that not me ((x+y-1)/y), aka. DIV_ROUND_UP(x,y) ?
> >
> > Well, there is certainly some rounding to be done...
> >
> > > The above would give 4 for 9/3, which is clearly buggered.
> >
> > .. still the math above should work fine within the boundaries we
> > define for UCLAMP_BUCKET_DELTA (5..20 groups) and considering that
> > SCHED_CAPACITY_SCALE will never be smaller then 1024.
>
> That's a very poor reason to write utter nonsense :-)
>
> > The above is designed to shrink the topmost bucket wrt all the others
> > but it will never be smaller than ~30%.
>
> 30% sounds like a lot, esp. for this range.
Well, that 30% is really just ~16 utiliation units on a scale of 1024
when buckets have a size of 52.
Still, yes, we can argue that's big but that's also the same error
generated by DIV_ROUND_UP() when UCLAMP_BUCKETS is not 8 or 16.
> > Here are the start values computed for each bucket using the math
> > above and the computed shrinking percentage for the topmost bucket:
>
> If you use a regular rounding, the error is _much_ smaller:
>
> $ for ((x=5;x<21;x++)) ; do let d=(1024+x/2)/x; let s=(x-1)*d; let e=1024-s; let p=100*(d-e)/d; echo $x $d $s $e $p%; done
^^^^^^^^^^^^^
> 5 205 820 204 0%
> 6 171 855 169 1%
> 7 146 876 148 -1%
> 8 128 896 128 0%
> 9 114 912 112 1%
> 10 102 918 106 -3%
> 11 93 930 94 -1%
> 12 85 935 89 -4%
> 13 79 948 76 3%
> 14 73 949 75 -2%
> 15 68 952 72 -5%
> 16 64 960 64 0%
> 17 60 960 64 -6%
> 18 57 969 55 3%
> 19 54 972 52 3%
> 20 51 969 55 -7%
>
> Funnily enough, we have a helper for that too: DIV_ROUND_CLOSEST().
^^^^^^^^^^^^^^^^^^^^
This is different than DIV_ROUND_UP() and actually better across the
full range.
> Now, if we go further, the error will obviously increase because we run
> out of precision, but even there, regular rounding will be better than
> either floor or ceil.
I don't think we will have to cover other values in the further but I
agree that this "closest rounding" is definitively better.
Thanks for spotting it, will update in v8.
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-13 8:19 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: <20190312155043.vj3fiutnsovjti2x@e110439-lin>
On Tue, Mar 12, 2019 at 03:50:43PM +0000, Patrick Bellasi wrote:
> On 12-Mar 16:20, Peter Zijlstra wrote:
> > On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > > +/* Integer ceil-rounded range for each bucket */
^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > +#define UCLAMP_BUCKET_DELTA ((SCHED_CAPACITY_SCALE / UCLAMP_BUCKETS) + 1)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
simply do not match.
> > Uhm, should that not me ((x+y-1)/y), aka. DIV_ROUND_UP(x,y) ?
>
> Well, there is certainly some rounding to be done...
>
> > The above would give 4 for 9/3, which is clearly buggered.
>
> .. still the math above should work fine within the boundaries we
> define for UCLAMP_BUCKET_DELTA (5..20 groups) and considering that
> SCHED_CAPACITY_SCALE will never be smaller then 1024.
That's a very poor reason to write utter nonsense :-)
> The above is designed to shrink the topmost bucket wrt all the others
> but it will never be smaller than ~30%.
30% sounds like a lot, esp. for this range.
> Here are the start values computed for each bucket using the math
> above and the computed shrinking percentage for the topmost bucket:
If you use a regular rounding, the error is _much_ smaller:
$ for ((x=5;x<21;x++)) ; do let d=(1024+x/2)/x; let s=(x-1)*d; let e=1024-s; let p=100*(d-e)/d; echo $x $d $s $e $p%; done
5 205 820 204 0%
6 171 855 169 1%
7 146 876 148 -1%
8 128 896 128 0%
9 114 912 112 1%
10 102 918 106 -3%
11 93 930 94 -1%
12 85 935 89 -4%
13 79 948 76 3%
14 73 949 75 -2%
15 68 952 72 -5%
16 64 960 64 0%
17 60 960 64 -6%
18 57 969 55 3%
19 54 972 52 3%
20 51 969 55 -7%
Funnily enough, we have a helper for that too: DIV_ROUND_CLOSEST().
Now, if we go further, the error will obviously increase because we run
out of precision, but even there, regular rounding will be better than
either floor or ceil.
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Patrick Bellasi @ 2019-03-12 15:50 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: <20190312152059.GA5922@hirez.programming.kicks-ass.net>
On 12-Mar 16:20, Peter Zijlstra wrote:
> On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> > +/* Integer ceil-rounded range for each bucket */
> > +#define UCLAMP_BUCKET_DELTA ((SCHED_CAPACITY_SCALE / UCLAMP_BUCKETS) + 1)
>
> Uhm, should that not me ((x+y-1)/y), aka. DIV_ROUND_UP(x,y) ?
Well, there is certainly some rounding to be done...
> The above would give 4 for 9/3, which is clearly buggered.
.. still the math above should work fine within the boundaries we
define for UCLAMP_BUCKET_DELTA (5..20 groups) and considering that
SCHED_CAPACITY_SCALE will never be smaller then 1024.
The above is designed to shrink the topmost bucket wrt all the others
but it will never be smaller than ~30%.
Here are the start values computed for each bucket using the math
above and the computed shrinking percentage for the topmost bucket:
bukets size: 205, top bucket start@820 (err: 0.49%), buckets: {0: 0, 1: 205, 2: 410, 3: 615, 4: 820}
bukets size: 171, top bucket start@855 (err: 1.17%), buckets: {0: 0, 1: 171, 2: 342, 3: 513, 4: 684, 5: 855}
bukets size: 147, top bucket start@882 (err: 3.40%), buckets: {0: 0, 1: 147, 2: 294, 3: 441, 4: 588, 5: 735, 6: 882}
bukets size: 129, top bucket start@903 (err: 6.20%), buckets: {0: 0, 1: 129, 2: 258, 3: 387, 4: 516, 5: 645, 6: 774, 7: 903}
bukets size: 114, top bucket start@912 (err: 1.75%), buckets: {0: 0, 1: 114, 2: 228, 3: 342, 4: 456, 5: 570, 6: 684, 7: 798, 8: 912}
bukets size: 103, top bucket start@927 (err: 5.83%), buckets: {0: 0, 1: 103, 2: 206, 3: 309, 4: 412, 5: 515, 6: 618, 7: 721, 8: 824, 9: 927}
bukets size: 94, top bucket start@940 (err: 10.64%), buckets: {0: 0, 1: 94, 2: 188, 3: 282, 4: 376, 5: 470, 6: 564, 7: 658, 8: 752, 9: 846, 10: 940}
bukets size: 86, top bucket start@946 (err: 9.30%), buckets: {0: 0, 1: 86, 2: 172, 3: 258, 4: 344, 5: 430, 6: 516, 7: 602, 8: 688, 9: 774, 10: 860, 11: 946}
bukets size: 79, top bucket start@948 (err: 3.80%), buckets: {0: 0, 1: 79, 2: 158, 3: 237, 4: 316, 5: 395, 6: 474, 7: 553, 8: 632, 9: 711, 10: 790, 11: 869, 12: 948}
bukets size: 74, top bucket start@962 (err: 16.22%), buckets: {0: 0, 1: 74, 2: 148, 3: 222, 4: 296, 5: 370, 6: 444, 7: 518, 8: 592, 9: 666, 10: 740, 11: 814, 12: 888, 13: 962}
bukets size: 69, top bucket start@966 (err: 15.94%), buckets: {0: 0, 1: 69, 2: 138, 3: 207, 4: 276, 5: 345, 6: 414, 7: 483, 8: 552, 9: 621, 10: 690, 11: 759, 12: 828, 13: 897, 14: 966}
bukets size: 65, top bucket start@975 (err: 24.62%), buckets: {0: 0, 1: 65, 2: 130, 3: 195, 4: 260, 5: 325, 6: 390, 7: 455, 8: 520, 9: 585, 10: 650, 11: 715, 12: 780, 13: 845, 14: 910, 15: 975}
bukets size: 61, top bucket start@976 (err: 21.31%), buckets: {0: 0, 1: 61, 2: 122, 3: 183, 4: 244, 5: 305, 6: 366, 7: 427, 8: 488, 9: 549, 10: 610, 11: 671, 12: 732, 13: 793, 14: 854, 15: 915, 16: 976}
bukets size: 57, top bucket start@969 (err: 3.51%), buckets: {0: 0, 1: 57, 2: 114, 3: 171, 4: 228, 5: 285, 6: 342, 7: 399, 8: 456, 9: 513, 10: 570, 11: 627, 12: 684, 13: 741, 14: 798, 15: 855, 16: 912, 17: 969}
bukets size: 54, top bucket start@972 (err: 3.70%), buckets: {0: 0, 1: 54, 2: 108, 3: 162, 4: 216, 5: 270, 6: 324, 7: 378, 8: 432, 9: 486, 10: 540, 11: 594, 12: 648, 13: 702, 14: 756, 15: 810, 16: 864, 17: 918, 18: 972}
bukets size: 52, top bucket start@988 (err: 30.77%), buckets: {0: 0, 1: 52, 2: 104, 3: 156, 4: 208, 5: 260, 6: 312, 7: 364, 8: 416, 9: 468, 10: 520, 11: 572, 12: 624, 13: 676, 14: 728, 15: 780, 16: 832, 17: 884, 18: 936, 19: 988}
Does that makes sense?
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Peter Zijlstra @ 2019-03-12 15:20 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: <20190208100554.32196-2-patrick.bellasi@arm.com>
On Fri, Feb 08, 2019 at 10:05:40AM +0000, Patrick Bellasi wrote:
> +/* Integer ceil-rounded range for each bucket */
> +#define UCLAMP_BUCKET_DELTA ((SCHED_CAPACITY_SCALE / UCLAMP_BUCKETS) + 1)
Uhm, should that not me ((x+y-1)/y), aka. DIV_ROUND_UP(x,y) ?
The above would give 4 for 9/3, which is clearly buggered.
^ permalink raw reply
* [PATCH v2 2/2] mm/mincore: provide mapped status when cached status is not allowed
From: Vlastimil Babka @ 2019-03-12 14:17 UTC (permalink / raw)
To: Andrew Morton
Cc: Linus Torvalds, Jann Horn, Michal Hocko, linux-mm, linux-kernel,
linux-api, Vlastimil Babka, Jiri Kosina, Dominique Martinet,
Andy Lutomirski, Dave Chinner, Kevin Easton, Matthew Wilcox,
Cyril Hrubis, Tejun Heo, Kirill A . Shutemov, Daniel Gruss,
Jiri Kosina, Josh Snyder, Michal Hocko
In-Reply-To: <20190312141708.6652-1-vbabka@suse.cz>
After "mm/mincore: make mincore() more conservative" we sometimes restrict the
information about page cache residency, which needs to be done without breaking
existing userspace, as much as possible. Instead of returning with error, we
thus fake the results. For that we return residency values as 1, which should
be safer than faking them as 0, as there might theoretically exist code that
would try to fault in the page(s) in a loop until mincore() returns 1 for them.
Faking 1 however means that such code would not fault in a page even if it was
not truly in page cache, with possibly unwanted performance implications. We
can improve the situation by revisting the approach of 574823bfab82 ("Change
mincore() to count "mapped" pages rather than "cached" pages"), later reverted
by 30bac164aca7 and replaced by restricting/faking the results. In this patch
we apply the approach only to cases where page cache residency check is
restricted. Thus mincore() will return 0 for an unmapped page (which may or may
not be resident in a pagecache), and 1 after the process faults it in.
One potential downside is that mincore() users will be now able to recognize
when a previously mapped page was reclaimed. While that might be useful for
some attack scenarios, it is not as crucial as recognizing that somebody else
faulted the page in, which is the main reason we are making mincore() more
conservative. For detecting that pages being reclaimed, there are also other
existing ways anyway.
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Kevin Easton <kevin@guarana.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Cyril Hrubis <chrubis@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Daniel Gruss <daniel@gruss.cc>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/mincore.c | 67 +++++++++++++++++++++++++++++++++++++++-------------
1 file changed, 51 insertions(+), 16 deletions(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index c3f058bd0faf..c9a265abc631 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -21,12 +21,23 @@
#include <linux/uaccess.h>
#include <asm/pgtable.h>
+/*
+ * mincore() page walk's private structure. Contains pointer to the array
+ * of return values to be set, and whether the current vma passed the
+ * can_do_mincore() check.
+ */
+struct mincore_walk_private {
+ unsigned char *vec;
+ bool can_check_pagecache;
+};
+
static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
unsigned long end, struct mm_walk *walk)
{
#ifdef CONFIG_HUGETLB_PAGE
unsigned char present;
- unsigned char *vec = walk->private;
+ struct mincore_walk_private *walk_private = walk->private;
+ unsigned char *vec = walk_private->vec;
/*
* Hugepages under user process are always in RAM and never
@@ -35,7 +46,7 @@ static int mincore_hugetlb(pte_t *pte, unsigned long hmask, unsigned long addr,
present = pte && !huge_pte_none(huge_ptep_get(pte));
for (; addr != end; vec++, addr += PAGE_SIZE)
*vec = present;
- walk->private = vec;
+ walk_private->vec = vec;
#else
BUG();
#endif
@@ -85,7 +96,8 @@ static unsigned char mincore_page(struct address_space *mapping, pgoff_t pgoff)
}
static int __mincore_unmapped_range(unsigned long addr, unsigned long end,
- struct vm_area_struct *vma, unsigned char *vec)
+ struct vm_area_struct *vma, unsigned char *vec,
+ bool can_check_pagecache)
{
unsigned long nr = (end - addr) >> PAGE_SHIFT;
int i;
@@ -95,7 +107,14 @@ static int __mincore_unmapped_range(unsigned long addr, unsigned long end,
pgoff = linear_page_index(vma, addr);
for (i = 0; i < nr; i++, pgoff++)
- vec[i] = mincore_page(vma->vm_file->f_mapping, pgoff);
+ /*
+ * Return page cache residency state if we are allowed
+ * to, otherwise return mapping state, which is 0 for
+ * an unmapped range.
+ */
+ vec[i] = can_check_pagecache ?
+ mincore_page(vma->vm_file->f_mapping, pgoff)
+ : 0;
} else {
for (i = 0; i < nr; i++)
vec[i] = 0;
@@ -106,8 +125,11 @@ static int __mincore_unmapped_range(unsigned long addr, unsigned long end,
static int mincore_unmapped_range(unsigned long addr, unsigned long end,
struct mm_walk *walk)
{
- walk->private += __mincore_unmapped_range(addr, end,
- walk->vma, walk->private);
+ struct mincore_walk_private *walk_private = walk->private;
+ unsigned char *vec = walk_private->vec;
+
+ walk_private->vec += __mincore_unmapped_range(addr, end, walk->vma,
+ vec, walk_private->can_check_pagecache);
return 0;
}
@@ -117,7 +139,8 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
spinlock_t *ptl;
struct vm_area_struct *vma = walk->vma;
pte_t *ptep;
- unsigned char *vec = walk->private;
+ struct mincore_walk_private *walk_private = walk->private;
+ unsigned char *vec = walk_private->vec;
int nr = (end - addr) >> PAGE_SHIFT;
ptl = pmd_trans_huge_lock(pmd, vma);
@@ -128,7 +151,8 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
}
if (pmd_trans_unstable(pmd)) {
- __mincore_unmapped_range(addr, end, vma, vec);
+ __mincore_unmapped_range(addr, end, vma, vec,
+ walk_private->can_check_pagecache);
goto out;
}
@@ -138,7 +162,7 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
if (pte_none(pte))
__mincore_unmapped_range(addr, addr + PAGE_SIZE,
- vma, vec);
+ vma, vec, walk_private->can_check_pagecache);
else if (pte_present(pte))
*vec = 1;
else { /* pte is a swap entry */
@@ -152,8 +176,20 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
*vec = 1;
} else {
#ifdef CONFIG_SWAP
- *vec = mincore_page(swap_address_space(entry),
+ /*
+ * If tmpfs pages are being swapped out, treat
+ * it with same restrictions on mincore() as
+ * the page cache so we don't expose that
+ * somebody else brought them back from swap.
+ * In the restricted case return 0 as swap
+ * entry means the page is not mapped.
+ */
+ if (walk_private->can_check_pagecache)
+ *vec = mincore_page(
+ swap_address_space(entry),
swp_offset(entry));
+ else
+ *vec = 0;
#else
WARN_ON(1);
*vec = 1;
@@ -195,22 +231,21 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
struct vm_area_struct *vma;
unsigned long end;
int err;
+ struct mincore_walk_private walk_private = {
+ .vec = vec
+ };
struct mm_walk mincore_walk = {
.pmd_entry = mincore_pte_range,
.pte_hole = mincore_unmapped_range,
.hugetlb_entry = mincore_hugetlb,
- .private = vec,
+ .private = &walk_private
};
vma = find_vma(current->mm, addr);
if (!vma || addr < vma->vm_start)
return -ENOMEM;
end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
- if (!can_do_mincore(vma)) {
- unsigned long pages = DIV_ROUND_UP(end - addr, PAGE_SIZE);
- memset(vec, 1, pages);
- return pages;
- }
+ walk_private.can_check_pagecache = can_do_mincore(vma);
mincore_walk.mm = vma->vm_mm;
err = walk_page_range(addr, end, &mincore_walk);
if (err < 0)
--
2.20.1
^ permalink raw reply related
* [PATCH v2 1/2] mm/mincore: make mincore() more conservative
From: Vlastimil Babka @ 2019-03-12 14:17 UTC (permalink / raw)
To: Andrew Morton
Cc: Linus Torvalds, Jann Horn, Michal Hocko, linux-mm, linux-kernel,
linux-api, Jiri Kosina, Dominique Martinet, Andy Lutomirski,
Dave Chinner, Kevin Easton, Matthew Wilcox, Cyril Hrubis,
Tejun Heo, Kirill A . Shutemov, Daniel Gruss, Vlastimil Babka,
Josh Snyder, Michal Hocko, Jiri Kosina
In-Reply-To: <20190312141708.6652-1-vbabka@suse.cz>
From: Jiri Kosina <jkosina@suse.cz>
The semantics of what mincore() considers to be resident is not completely
clear, but Linux has always (since 2.3.52, which is when mincore() was
initially done) treated it as "page is available in page cache".
That's potentially a problem, as that [in]directly exposes meta-information
about pagecache / memory mapping state even about memory not strictly belonging
to the process executing the syscall, opening possibilities for sidechannel
attacks.
Change the semantics of mincore() so that it only reveals pagecache information
for non-anonymous mappings that belog to files that the calling process could
(if it tried to) successfully open for writing; otherwise we'd be including
shared non-exclusive mappings, which
- is the sidechannel
- is not the usecase for mincore(), as that's primarily used for data, not
(shared) text
[mhocko@suse.com: restructure can_do_mincore() conditions]
Originally-by: Linus Torvalds <torvalds@linux-foundation.org>
Originally-by: Dominique Martinet <asmadeus@codewreck.org>
Cc: Dominique Martinet <asmadeus@codewreck.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Dave Chinner <david@fromorbit.com>
Cc: Kevin Easton <kevin@guarana.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Cyril Hrubis <chrubis@suse.cz>
Cc: Tejun Heo <tj@kernel.org>
Cc: Kirill A. Shutemov <kirill@shutemov.name>
Cc: Daniel Gruss <daniel@gruss.cc>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Josh Snyder <joshs@netflix.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
mm/mincore.c | 23 ++++++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/mm/mincore.c b/mm/mincore.c
index 218099b5ed31..c3f058bd0faf 100644
--- a/mm/mincore.c
+++ b/mm/mincore.c
@@ -169,6 +169,22 @@ static int mincore_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
return 0;
}
+static inline bool can_do_mincore(struct vm_area_struct *vma)
+{
+ if (vma_is_anonymous(vma))
+ return true;
+ if (!vma->vm_file)
+ return false;
+ /*
+ * Reveal pagecache information only for non-anonymous mappings that
+ * correspond to the files the calling process could (if tried) open
+ * for writing; otherwise we'd be including shared non-exclusive
+ * mappings, which opens a side channel.
+ */
+ return inode_owner_or_capable(file_inode(vma->vm_file)) ||
+ inode_permission(file_inode(vma->vm_file), MAY_WRITE) == 0;
+}
+
/*
* Do a chunk of "sys_mincore()". We've already checked
* all the arguments, we hold the mmap semaphore: we should
@@ -189,8 +205,13 @@ static long do_mincore(unsigned long addr, unsigned long pages, unsigned char *v
vma = find_vma(current->mm, addr);
if (!vma || addr < vma->vm_start)
return -ENOMEM;
- mincore_walk.mm = vma->vm_mm;
end = min(vma->vm_end, addr + (pages << PAGE_SHIFT));
+ if (!can_do_mincore(vma)) {
+ unsigned long pages = DIV_ROUND_UP(end - addr, PAGE_SIZE);
+ memset(vec, 1, pages);
+ return pages;
+ }
+ mincore_walk.mm = vma->vm_mm;
err = walk_page_range(addr, end, &mincore_walk);
if (err < 0)
return err;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 0/2] prevent mincore() page cache leaks
From: Vlastimil Babka @ 2019-03-12 14:17 UTC (permalink / raw)
To: Andrew Morton
Cc: Linus Torvalds, Jann Horn, Michal Hocko, linux-mm, linux-kernel,
linux-api, Vlastimil Babka, Andy Lutomirski, Cyril Hrubis,
Daniel Gruss, Dave Chinner, Dominique Martinet, Jiri Kosina,
Jiri Kosina, Josh Snyder, Kevin Easton, Kirill A. Shutemov,
Matthew Wilcox, Michal Hocko, Tejun Heo
In-Reply-To: <20190130124420.1834-1-vbabka@suse.cz>
Here's a new version of the mincore() patches, with feedback from Andrew Morton
applied. The IOCB_NOWAIT patch was dropped since David Chinner pointed out it's
incomplete. We definitely want the first patch, while for the second Linus
said:
I think that's fine, and probably the right thing to do, but I also
suspect that nobody actually cares ;(
Whether or not somebody cares, we should hear of no breakage. If somebody does
care after all, without second patch we might hear of breakage, so I would
suggest applying it. It's not that complicated after all (famous last words?)
Jiri Kosina (1):
mm/mincore: make mincore() more conservative
Vlastimil Babka (1):
mm/mincore: provide mapped status when cached status is not allowed
mm/mincore.c | 80 ++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 68 insertions(+), 12 deletions(-)
--
2.20.1
^ permalink raw reply
* Re: [PATCHv7 10/10] doc/mm: New documentation for memory performance
From: Jonathan Cameron @ 2019-03-12 13:37 UTC (permalink / raw)
To: Keith Busch
Cc: Busch, Keith, linux-kernel@vger.kernel.org,
linux-acpi@vger.kernel.org, linux-mm@kvack.org,
linux-api@vger.kernel.org, Greg Kroah-Hartman, Rafael Wysocki,
Hansen, Dave, Williams, Dan J
In-Reply-To: <20190311201632.GG10411@localhost.localdomain>
On Mon, 11 Mar 2019 14:16:33 -0600
Keith Busch <kbusch@kernel.org> wrote:
> On Mon, Mar 11, 2019 at 04:38:43AM -0700, Jonathan Cameron wrote:
> > On Wed, 27 Feb 2019 15:50:38 -0700
> > Keith Busch <keith.busch@intel.com> wrote:
> >
> > > Platforms may provide system memory where some physical address ranges
> > > perform differently than others, or is side cached by the system.
> > The magic 'side cached' term still here in the patch description, ideally
> > wants cleaning up.
> >
> > >
> > > Add documentation describing a high level overview of such systems and the
> > > perforamnce and caching attributes the kernel provides for applications
> > performance
> >
> > > wishing to query this information.
> > >
> > > Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
> > > Signed-off-by: Keith Busch <keith.busch@intel.com>
> >
> > A few comments inline. Mostly the weird corner cases that I miss understood
> > in one of the earlier versions of the code.
> >
> > Whilst I think perhaps that one section could be tweaked a tiny bit I'm basically
> > happy with this if you don't want to.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >
> > > ---
> > > Documentation/admin-guide/mm/numaperf.rst | 164 ++++++++++++++++++++++++++++++
> > > 1 file changed, 164 insertions(+)
> > > create mode 100644 Documentation/admin-guide/mm/numaperf.rst
> > >
> > > diff --git a/Documentation/admin-guide/mm/numaperf.rst b/Documentation/admin-guide/mm/numaperf.rst
> > > new file mode 100644
> > > index 000000000000..d32756b9be48
> > > --- /dev/null
> > > +++ b/Documentation/admin-guide/mm/numaperf.rst
> > > @@ -0,0 +1,164 @@
> > > +.. _numaperf:
> > > +
> > > +=============
> > > +NUMA Locality
> > > +=============
> > > +
> > > +Some platforms may have multiple types of memory attached to a compute
> > > +node. These disparate memory ranges may share some characteristics, such
> > > +as CPU cache coherence, but may have different performance. For example,
> > > +different media types and buses affect bandwidth and latency.
> > > +
> > > +A system supports such heterogeneous memory by grouping each memory type
> > > +under different domains, or "nodes", based on locality and performance
> > > +characteristics. Some memory may share the same node as a CPU, and others
> > > +are provided as memory only nodes. While memory only nodes do not provide
> > > +CPUs, they may still be local to one or more compute nodes relative to
> > > +other nodes. The following diagram shows one such example of two compute
> > > +nodes with local memory and a memory only node for each of compute node:
> > > +
> > > + +------------------+ +------------------+
> > > + | Compute Node 0 +-----+ Compute Node 1 |
> > > + | Local Node0 Mem | | Local Node1 Mem |
> > > + +--------+---------+ +--------+---------+
> > > + | |
> > > + +--------+---------+ +--------+---------+
> > > + | Slower Node2 Mem | | Slower Node3 Mem |
> > > + +------------------+ +--------+---------+
> > > +
> > > +A "memory initiator" is a node containing one or more devices such as
> > > +CPUs or separate memory I/O devices that can initiate memory requests.
> > > +A "memory target" is a node containing one or more physical address
> > > +ranges accessible from one or more memory initiators.
> > > +
> > > +When multiple memory initiators exist, they may not all have the same
> > > +performance when accessing a given memory target. Each initiator-target
> > > +pair may be organized into different ranked access classes to represent
> > > +this relationship.
> >
> > This concept is a bit vague at the moment. Largely because only access0
> > is actually defined. We should definitely keep a close eye on any others
> > that are defined in future to make sure this text is still valid.
> >
> > I can certainly see it being used for different ideas of 'best' rather
> > than simply best and second best etc.
>
> I tried to make the interface flexible to future extension, but I'm
> still not sure how potential users would want to see something like
> all pair-wise attributes, so I had some trouble trying to capture that
> in words.
Agreed, it is definitely non obvious. We might end up with something
totally different like Jerome is proposing anyway. Let's address
this when it happens!
>
> > > The highest performing initiator to a given target
> > > +is considered to be one of that target's local initiators, and given
> > > +the highest access class, 0. Any given target may have one or more
> > > +local initiators, and any given initiator may have multiple local
> > > +memory targets.
> > > +
> > > +To aid applications matching memory targets with their initiators, the
> > > +kernel provides symlinks to each other. The following example lists the
> > > +relationship for the access class "0" memory initiators and targets, which is
> > > +the of nodes with the highest performing access relationship::
> > > +
> > > + # symlinks -v /sys/devices/system/node/nodeX/access0/targets/
> > > + relative: /sys/devices/system/node/nodeX/access0/targets/nodeY -> ../../nodeY
> >
> > So this one perhaps needs a bit more description - I would put it after initiators
> > which precisely fits the description you have here now.
> >
> > "targets contains those nodes for which this initiator is the best possible initiator."
> >
> > which is subtly different form
> >
> > "targets contains those nodes to which this node has the highest
> > performing access characteristics."
> >
> > For example in my test case:
> > * 4 nodes with local memory and cpu, 1 node remote and equal distant from all of the
> > initiators,
> >
> > targets for the compute nodes contains both themselves and the remote node, to which
> > the characteristics are of course worse. As you point out before, we need to look
> > in
> > node0/access0/targets/node0/access0/initiators
> > node0/access0/targets/node4/access0/initiators
> > to get the relevant characteristics and work out that node0 is 'nearer' itself
> > (obviously this is a bit of a silly case, but we could have no memory node0 and
> > be talking about node4 and node5.
> >
> > I am happy with the actual interface, this is just a question about whether we can tweak
> > this text to be slightly clearer.
>
> Sure, I mention this in patch 4's commit message. Probably worth
> repeating here:
>
> A memory initiator may have multiple memory targets in the same access
> class. The target memory's initiators in a given class indicate the
> nodes access characteristics share the same performance relative to other
> linked initiator nodes. Each target within an initiator's access class,
> though, do not necessarily perform the same as each other.
That sounds good to me.
^ permalink raw reply
* Re: [PATCH v7 01/15] sched/core: uclamp: Add CPU's clamp buckets refcounting
From: Dietmar Eggemann @ 2019-03-12 12:52 UTC (permalink / raw)
To: Patrick Bellasi, linux-kernel, linux-pm, linux-api
Cc: Ingo Molnar, Peter Zijlstra, Tejun Heo, Rafael J . Wysocki,
Vincent Guittot, Viresh Kumar, Paul Turner, Quentin Perret,
Morten Rasmussen, Juri Lelli, Todd Kjos, Joel Fernandes,
Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190208100554.32196-2-patrick.bellasi@arm.com>
On 2/8/19 11:05 AM, Patrick Bellasi wrote:
[...]
> +config UCLAMP_BUCKETS_COUNT
> + int "Number of supported utilization clamp buckets"
> + range 5 20
> + default 5
> + depends on UCLAMP_TASK
> + help
> + Defines the number of clamp buckets to use. The range of each bucket
> + will be SCHED_CAPACITY_SCALE/UCLAMP_BUCKETS_COUNT. The higher the
> + number of clamp buckets the finer their granularity and the higher
> + the precision of clamping aggregation and tracking at run-time.
> +
> + For example, with the default configuration we will have 5 clamp
> + buckets tracking 20% utilization each. A 25% boosted tasks will be
> + refcounted in the [20..39]% bucket and will set the bucket clamp
> + effective value to 25%.
> + If a second 30% boosted task should be co-scheduled on the same CPU,
> + that task will be refcounted in the same bucket of the first task and
> + it will boost the bucket clamp effective value to 30%.
> + The clamp effective value of a bucket is reset to its nominal value
> + (20% in the example above) when there are anymore tasks refcounted in
this sounds weird.
[...]
> +static inline unsigned int uclamp_bucket_value(unsigned int clamp_value)
> +{
> + return UCLAMP_BUCKET_DELTA * uclamp_bucket_id(clamp_value);
> +}
Soemthing like uclamp_bucket_nominal_value() should be clearer.
> +static inline void uclamp_rq_update(struct rq *rq, unsigned int clamp_id)
> +{
> + struct uclamp_bucket *bucket = rq->uclamp[clamp_id].bucket;
> + unsigned int max_value = uclamp_none(clamp_id);
> + unsigned int bucket_id;
unsigned int bucket_id = UCLAMP_BUCKETS;
> +
> + /*
> + * Both min and max clamps are MAX aggregated, thus the topmost
> + * bucket with some tasks defines the rq's clamp value.
> + */
> + bucket_id = UCLAMP_BUCKETS;
to get rid of this line?
> + do {
> + --bucket_id;
> + if (!rq->uclamp[clamp_id].bucket[bucket_id].tasks)
if (!bucket[bucket_id].tasks)
[...]
> +/*
> + * When a task is enqueued on a rq, the clamp bucket currently defined by the
> + * task's uclamp::bucket_id is reference counted on that rq. This also
> + * immediately updates the rq's clamp value if required.
> + *
> + * Since tasks know their specific value requested from user-space, we track
> + * within each bucket the maximum value for tasks refcounted in that bucket.
> + * This provide a further aggregation (local clamping) which allows to track
s/This provide/This provides
> + * within each bucket the exact "requested" clamp value whenever all tasks
> + * RUNNABLE in that bucket require the same clamp.
> + */
> +static inline void uclamp_rq_inc_id(struct task_struct *p, struct rq *rq,
> + unsigned int clamp_id)
> +{
> + unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
> + unsigned int rq_clamp, bkt_clamp, tsk_clamp;
Wouldn't it be easier to have a pointer to the task's and rq's uclamp
structure as well to the bucket?
- unsigned int bucket_id = p->uclamp[clamp_id].bucket_id;
+ struct uclamp_se *uc_se = &p->uclamp[clamp_id];
+ struct uclamp_rq *uc_rq = &rq->uclamp[clamp_id];
+ struct uclamp_bucket *bucket = &uc_rq->bucket[uc_se->bucket_id];
The code in uclamp_rq_inc_id() and uclamp_rq_dec_id() for example
becomes much more readable.
[...]
> struct sched_class {
> const struct sched_class *next;
>
> +#ifdef CONFIG_UCLAMP_TASK
> + int uclamp_enabled;
> +#endif
> +
> void (*enqueue_task) (struct rq *rq, struct task_struct *p, int flags);
> void (*dequeue_task) (struct rq *rq, struct task_struct *p, int flags);
> - void (*yield_task) (struct rq *rq);
> - bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
>
> void (*check_preempt_curr)(struct rq *rq, struct task_struct *p, int flags);
>
> @@ -1685,7 +1734,6 @@ struct sched_class {
> void (*set_curr_task)(struct rq *rq);
> void (*task_tick)(struct rq *rq, struct task_struct *p, int queued);
> void (*task_fork)(struct task_struct *p);
> - void (*task_dead)(struct task_struct *p);
>
> /*
> * The switched_from() call is allowed to drop rq->lock, therefore we
> @@ -1702,12 +1750,17 @@ struct sched_class {
>
> void (*update_curr)(struct rq *rq);
>
> + void (*yield_task) (struct rq *rq);
> + bool (*yield_to_task)(struct rq *rq, struct task_struct *p, bool preempt);
> +
> #define TASK_SET_GROUP 0
> #define TASK_MOVE_GROUP 1
>
> #ifdef CONFIG_FAIR_GROUP_SCHED
> void (*task_change_group)(struct task_struct *p, int type);
> #endif
> +
> + void (*task_dead)(struct task_struct *p);
Why do you move yield_task, yield_to_task and task_dead here?
[...]
^ permalink raw reply
* Re: [PATCH 0/3] userfaultfd: allow to forbid unprivileged users
From: Peter Xu @ 2019-03-12 12:43 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-kernel, Paolo Bonzini, Hugh Dickins, Luis Chamberlain,
Maxime Coquelin, kvm, Jerome Glisse, Pavel Emelyanov,
Johannes Weiner, Martin Cracauer, Denis Plotnikov, linux-mm,
Marty McFadden, Maya Gokhale, Mike Kravetz, Andrea Arcangeli,
Mike Rapoport, Kees Cook, Mel Gorman, linux-fsdevel,
Dr . David Alan Gilbert
In-Reply-To: <20190312074951.i2md3npcjcceywqj@kshutemo-mobl1>
Hi, Kirill,
On Tue, Mar 12, 2019 at 10:49:51AM +0300, Kirill A. Shutemov wrote:
> On Mon, Mar 11, 2019 at 05:36:58PM +0800, Peter Xu wrote:
> > Hi,
> >
> > (The idea comes from Andrea, and following discussions with Mike and
> > other people)
> >
> > This patchset introduces a new sysctl flag to allow the admin to
> > forbid users from using userfaultfd:
> >
> > $ cat /proc/sys/vm/unprivileged_userfaultfd
> > [disabled] enabled kvm
>
> CC linux-api@
>
> This is unusual way to return current value for sysctl. Does it work fine
> with sysctl tool?
It can work, though it displays the same as "cat":
$ sysctl vm.unprivileged_userfaultfd
vm.unprivileged_userfaultfd = disabled enabled [kvm]
>
> Have you considered to place the switch into /sys/kernel/mm instead?
> I doubt it's the last tunable for userfaultfd. Maybe we should have an
> directory for it under /sys/kernel/mm?
I haven't thought about sysfs, if that's preferred I can consider to
switch to that. And yes I think creating a directory should be a good
idea.
Thanks,
--
Peter Xu
^ permalink raw reply
* Re: [PATCH] x86: Deprecate a.out support
From: Geert Uytterhoeven @ 2019-03-12 8:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: Arnd Bergmann, Måns Rullgård, Matt Turner,
Borislav Petkov, Alan Cox, Matthew Wilcox, Jann Horn, Al Viro,
Thomas Gleixner, kernel list, linux-fsdevel,
the arch/x86 maintainers, Linux API, Andrew Morton,
Richard Weinberger, Anton Ivanov, linux-alpha, linux-m68k
In-Reply-To: <CAHk-=wj3v41CWH+3ry3+vrkjAEcyGojtevpsZDbiXVvpv1nTkA@mail.gmail.com>
On Mon, Mar 11, 2019 at 10:46 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> On Mon, Mar 11, 2019 at 2:34 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > The main historic use case I've heard of was running Netscape
> > Navigator on Alpha Linux, before there was an open source version.
> > Doing this today to connect to the open internet is probably
> > a bit pointless, but there may be other use cases.
>
> The _really_ main version was that I decided to make my life easier
> for the initial alpha port by trying to run basic (tested) OSF/1
> binaries directly.
>
> Netscape may have been one of the binaries people actually ended up
> using, but it's probably not a reason any more, since the internet has
> moved past that anyway.
Yeah, the alphas on the server side, powering AltaVista, are also long
gone...
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 0/3] userfaultfd: allow to forbid unprivileged users
From: Kirill A. Shutemov @ 2019-03-12 7:49 UTC (permalink / raw)
To: Peter Xu
Cc: linux-kernel, Paolo Bonzini, Hugh Dickins, Luis Chamberlain,
Maxime Coquelin, kvm, Jerome Glisse, Pavel Emelyanov,
Johannes Weiner, Martin Cracauer, Denis Plotnikov, linux-mm,
Marty McFadden, Maya Gokhale, Mike Kravetz, Andrea Arcangeli,
Mike Rapoport, Kees Cook, Mel Gorman, linux-fsdevel,
Dr . David Alan Gilbert
In-Reply-To: <20190311093701.15734-1-peterx@redhat.com>
On Mon, Mar 11, 2019 at 05:36:58PM +0800, Peter Xu wrote:
> Hi,
>
> (The idea comes from Andrea, and following discussions with Mike and
> other people)
>
> This patchset introduces a new sysctl flag to allow the admin to
> forbid users from using userfaultfd:
>
> $ cat /proc/sys/vm/unprivileged_userfaultfd
> [disabled] enabled kvm
CC linux-api@
This is unusual way to return current value for sysctl. Does it work fine
with sysctl tool?
Have you considered to place the switch into /sys/kernel/mm instead?
I doubt it's the last tunable for userfaultfd. Maybe we should have an
directory for it under /sys/kernel/mm?
> - When set to "disabled", all unprivileged users are forbidden to
> use userfaultfd syscalls.
>
> - When set to "enabled", all users are allowed to use userfaultfd
> syscalls.
>
> - When set to "kvm", all unprivileged users are forbidden to use the
> userfaultfd syscalls, except the user who has permission to open
> /dev/kvm.
>
> This new flag can add one more layer of security to reduce the attack
> surface of the kernel by abusing userfaultfd. Here we grant the
> thread userfaultfd permission by checking against CAP_SYS_PTRACE
> capability. By default, the value is "disabled" which is the most
> strict policy. Distributions can have their own perferred value.
>
> The "kvm" entry is a bit special here only to make sure that existing
> users like QEMU/KVM won't break by this newly introduced flag. What
> we need to do is simply set the "unprivileged_userfaultfd" flag to
> "kvm" here to automatically grant userfaultfd permission for processes
> like QEMU/KVM without extra code to tweak these flags in the admin
> code.
>
> Patch 1: The interface patch to introduce the flag
>
> Patch 2: The KVM related changes to detect opening of /dev/kvm
>
> Patch 3: Apply the flag to userfaultfd syscalls
>
> All comments would be greatly welcomed. Thanks,
>
> Peter Xu (3):
> userfaultfd/sysctl: introduce unprivileged_userfaultfd
> kvm/mm: introduce MMF_USERFAULTFD_ALLOW flag
> userfaultfd: apply unprivileged_userfaultfd check
>
> fs/userfaultfd.c | 121 +++++++++++++++++++++++++++++++++
> include/linux/sched/coredump.h | 1 +
> include/linux/userfaultfd_k.h | 5 ++
> init/Kconfig | 11 +++
> kernel/sysctl.c | 11 +++
> virt/kvm/kvm_main.c | 7 ++
> 6 files changed, 156 insertions(+)
>
> --
> 2.17.1
>
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH] x86: Deprecate a.out support
From: Michael Cree @ 2019-03-12 6:38 UTC (permalink / raw)
To: Matt Turner
Cc: Arnd Bergmann, Måns Rullgård, Linus Torvalds,
Borislav Petkov, Alan Cox, Matthew Wilcox, Jann Horn, Al Viro,
Thomas Gleixner, kernel list, linux-fsdevel,
the arch/x86 maintainers, Linux API, Andrew Morton,
Richard Weinberger, Anton Ivanov, linux-alpha, linux-m68k
In-Reply-To: <CAEdQ38HSKy+muCwAk9w0yNCt649HVja1Pe+yY23-4zBjDvWFRA@mail.gmail.com>
On Mon, Mar 11, 2019 at 03:11:55PM -0700, Matt Turner wrote:
> On Mon, Mar 11, 2019 at 2:34 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Mar 11, 2019 at 8:47 PM Måns Rullgård <mans@mansr.com> wrote:
> > > Linus Torvalds <torvalds@linux-foundation.org> writes:
> > > > On Mon, Mar 11, 2019 at 11:08 AM Måns Rullgård <mans@mansr.com> wrote:
> > > > We don't have any specific support for ECOFF.
> > > >
> > > > I _think_. Again, it's been years and years.
I agree. I personally have never run any OSF/1 executables on
Linux Alpha and have no interest in doing so.
> > The main historic use case I've heard of was running Netscape
> > Navigator on Alpha Linux, before there was an open source version.
> > Doing this today to connect to the open internet is probably
> > a bit pointless, but there may be other use cases.
>
> The best use case I know of is to run their C compiler. Måns sent
> patches in fact to make it work.
>
> There is a Linux version of the same compiler but I have a vague
> memory that it's broken in various ways that the Tru64 version is
> not.
The last time I tried the Compaq C compiler for Alpha-Linux it still
worked, well, that is, the compiler worked, but the library header
files are broken and haven't worked with glibc for a long time. So
it is only useful as a free-standing compiler.
In the past it also produced better code than gcc, but gcc is now
so vastly improved w.r.t. optimisation and compliance to more recent
standards, that I would be surprised if there is any real use for
the Compaq compiler.
Cheers,
Michael.
^ 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