Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
On 20-Jul 17:25, Suren Baghdasaryan wrote: [...] > > ---8<--- > > > > /* Uclamp flags */ > > #define SCHED_FLAG_UTIL_CLAMP_STRICT0x11 /* Roll-back on failure */ > > #define SCHED_FLAG_UTIL_CLAMP_MIN 0x12 /* Update util_min */ > > #define SCHED_FLAG_UTIL_CLAMP_MAX 0x14 /* Update util_max */ > > #define SCHED_FLAG_UTIL_CLAMP ( \ > > SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX) > > > > Having ability to update only min or only max this way might be indeed > very useful. > Instead of rolling back on failure I would suggest to check both > inputs first to make sure there won't be any error before updating. > This would remove the need for SCHED_FLAG_UTIL_CLAMP_STRICT (which I > think any user would want to set to 1 anyway). > Looks like uclamp_group_get() can fail only if uclamp_group_find() > fails to find a slot for uclamp_value or a free slot. So one way to do > this search before update is to call uclamp_group_find() for both > UCLAMP_MIN and UCLAMP_MAX beforehand and if they succeed then pass > obtained next_group_ids into uclamp_group_get() to avoid doing the > same search twice. This requires some refactoring of > uclamp_group_get() but I think the end result would be a cleaner and > more predictable solution. Yes, that sound possible... provided we check all the groups under the same uclamp_mutex, it should be possible to find the group_ids before actually increasing the refcount. ... will look into this for the next reposting. -- #include Patrick Bellasi
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
On 20-Jul 17:25, Suren Baghdasaryan wrote: [...] > > ---8<--- > > > > /* Uclamp flags */ > > #define SCHED_FLAG_UTIL_CLAMP_STRICT0x11 /* Roll-back on failure */ > > #define SCHED_FLAG_UTIL_CLAMP_MIN 0x12 /* Update util_min */ > > #define SCHED_FLAG_UTIL_CLAMP_MAX 0x14 /* Update util_max */ > > #define SCHED_FLAG_UTIL_CLAMP ( \ > > SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX) > > > > Having ability to update only min or only max this way might be indeed > very useful. > Instead of rolling back on failure I would suggest to check both > inputs first to make sure there won't be any error before updating. > This would remove the need for SCHED_FLAG_UTIL_CLAMP_STRICT (which I > think any user would want to set to 1 anyway). > Looks like uclamp_group_get() can fail only if uclamp_group_find() > fails to find a slot for uclamp_value or a free slot. So one way to do > this search before update is to call uclamp_group_find() for both > UCLAMP_MIN and UCLAMP_MAX beforehand and if they succeed then pass > obtained next_group_ids into uclamp_group_get() to avoid doing the > same search twice. This requires some refactoring of > uclamp_group_get() but I think the end result would be a cleaner and > more predictable solution. Yes, that sound possible... provided we check all the groups under the same uclamp_mutex, it should be possible to find the group_ids before actually increasing the refcount. ... will look into this for the next reposting. -- #include Patrick Bellasi
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
Hi Patrick,
On Fri, Jul 20, 2018 at 8:11 AM, Patrick Bellasi
wrote:
> Hi Suren,
> thanks for the review, all good point... some more comments follow
> inline.
>
> On 19-Jul 16:51, Suren Baghdasaryan wrote:
>> On Mon, Jul 16, 2018 at 1:28 AM, Patrick Bellasi
>> wrote:
>
> [...]
>
>> > +/**
>> > + * uclamp_group_available: checks if a clamp group is available
>> > + * @clamp_id: the utilization clamp index (i.e. min or max clamp)
>> > + * @group_id: the group index in the given clamp_id
>> > + *
>> > + * A clamp group is not free if there is at least one SE which is sing a
>> > clamp
>>
>> Did you mean to say "single clamp"?
>
> No, it's "...at least one SE which is USING a clamp value..."
>
>> > + * value mapped on the specified clamp_id. These SEs are reference
>> > counted by
>> > + * the se_count of a uclamp_map entry.
>> > + *
>> > + * Return: true if there are no SE's mapped on the specified clamp
>> > + * index and group
>> > + */
>> > +static inline bool uclamp_group_available(int clamp_id, int group_id)
>> > +{
>> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
>> > +
>> > + return (uc_map[group_id].value == UCLAMP_NONE);
>>
>> The usage of UCLAMP_NONE is very confusing to me. It was not used at
>> all in the patch where it was introduced [1/12], here it's used as a
>> clamp value and in uclamp_group_find() it's used as group_id. Please
>> clarify the usage.
>
> Yes, it's meant to represent a "clamp not valid" condition, whatever
> it's a "clamp group" or a "clamp value"... perhaps the name can be
> improved.
>
>> I also feel UCLAMP_NONE does not really belong to
>> the uclamp_id enum because other elements there are indexes in
>> uclamp_maps and this one is a special value.
>
> Right, it looks a bit misplaced, I agree. I think I tried to set it
> using a #define but there was some issues I don't remember now...
> Anyway, I'll give it another go...
>
>
>> IMHO if both *group_id*
>> and *value* need a special value (-1) to represent
>> unused/uninitialized entry it would be better to use different
>> constants. Maybe UCLAMP_VAL_NONE and UCLAMP_GROUP_NONE?
>
> Yes, maybe we can use a
>
>#define UCLAMP_NOT_VALID -1
>
> and get rid the confusing enum entry.
>
> Will update it on v3.
>
Sounds good to me.
>> > +}
>
> [...]
>
>> > +/**
>> > + * uclamp_group_find: finds the group index of a utilization clamp group
>> > + * @clamp_id: the utilization clamp index (i.e. min or max clamping)
>> > + * @clamp_value: the utilization clamping value lookup for
>> > + *
>> > + * Verify if a group has been assigned to a certain clamp value and return
>> > + * its index to be used for accounting.
>> > + *
>> > + * Since only a limited number of utilization clamp groups are allowed,
>> > if no
>> > + * groups have been assigned for the specified value, a new group is
>> > assigned
>> > + * if possible. Otherwise an error is returned, meaning that an
>> > additional clamp
>> > + * value is not (currently) supported.
>> > + */
>> > +static int
>> > +uclamp_group_find(int clamp_id, unsigned int clamp_value)
>> > +{
>> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
>> > + int free_group_id = UCLAMP_NONE;
>> > + unsigned int group_id = 0;
>> > +
>> > + for ( ; group_id <= CONFIG_UCLAMP_GROUPS_COUNT; ++group_id) {
>> > + /* Keep track of first free clamp group */
>> > + if (uclamp_group_available(clamp_id, group_id)) {
>> > + if (free_group_id == UCLAMP_NONE)
>> > + free_group_id = group_id;
>> > + continue;
>> > + }
>> > + /* Return index of first group with same clamp value */
>> > + if (uc_map[group_id].value == clamp_value)
>> > + return group_id;
>> > + }
>> > + /* Default to first free clamp group */
>> > + if (group_id > CONFIG_UCLAMP_GROUPS_COUNT)
>>
>> Is the condition above needed? I think it's always true if you got here.
>> Also AFAIKT after the for loop you can just do:
>>
>> return (free_group_id != UCLAMP_NONE) ? free_group_id : -ENOSPC;
>
> Yes, you right... the code above can be simplified!
>
>>
>> > + group_id = free_group_id;
>> > + /* All clamp group already track different clamp values */
>> > + if (group_id == UCLAMP_NONE)
>> > + return -ENOSPC;
>> > + return group_id;
>> > +}
>
> [...]
>
>> > +static inline void uclamp_group_put(int clamp_id, int group_id)
>> > +{
>> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
>> > + unsigned long flags;
>> > +
>> > + /* Ignore SE's not yet attached */
>> > + if (group_id == UCLAMP_NONE)
>> > + return;
>> > +
>> > + /* Remove SE from this clamp group */
>> > + raw_spin_lock_irqsave(_map[group_id].se_lock, flags);
>> > + uc_map[group_id].se_count -= 1;
>>
>> If uc_map[group_id].se_count was 0 before decrement you
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
Hi Patrick,
On Fri, Jul 20, 2018 at 8:11 AM, Patrick Bellasi
wrote:
> Hi Suren,
> thanks for the review, all good point... some more comments follow
> inline.
>
> On 19-Jul 16:51, Suren Baghdasaryan wrote:
>> On Mon, Jul 16, 2018 at 1:28 AM, Patrick Bellasi
>> wrote:
>
> [...]
>
>> > +/**
>> > + * uclamp_group_available: checks if a clamp group is available
>> > + * @clamp_id: the utilization clamp index (i.e. min or max clamp)
>> > + * @group_id: the group index in the given clamp_id
>> > + *
>> > + * A clamp group is not free if there is at least one SE which is sing a
>> > clamp
>>
>> Did you mean to say "single clamp"?
>
> No, it's "...at least one SE which is USING a clamp value..."
>
>> > + * value mapped on the specified clamp_id. These SEs are reference
>> > counted by
>> > + * the se_count of a uclamp_map entry.
>> > + *
>> > + * Return: true if there are no SE's mapped on the specified clamp
>> > + * index and group
>> > + */
>> > +static inline bool uclamp_group_available(int clamp_id, int group_id)
>> > +{
>> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
>> > +
>> > + return (uc_map[group_id].value == UCLAMP_NONE);
>>
>> The usage of UCLAMP_NONE is very confusing to me. It was not used at
>> all in the patch where it was introduced [1/12], here it's used as a
>> clamp value and in uclamp_group_find() it's used as group_id. Please
>> clarify the usage.
>
> Yes, it's meant to represent a "clamp not valid" condition, whatever
> it's a "clamp group" or a "clamp value"... perhaps the name can be
> improved.
>
>> I also feel UCLAMP_NONE does not really belong to
>> the uclamp_id enum because other elements there are indexes in
>> uclamp_maps and this one is a special value.
>
> Right, it looks a bit misplaced, I agree. I think I tried to set it
> using a #define but there was some issues I don't remember now...
> Anyway, I'll give it another go...
>
>
>> IMHO if both *group_id*
>> and *value* need a special value (-1) to represent
>> unused/uninitialized entry it would be better to use different
>> constants. Maybe UCLAMP_VAL_NONE and UCLAMP_GROUP_NONE?
>
> Yes, maybe we can use a
>
>#define UCLAMP_NOT_VALID -1
>
> and get rid the confusing enum entry.
>
> Will update it on v3.
>
Sounds good to me.
>> > +}
>
> [...]
>
>> > +/**
>> > + * uclamp_group_find: finds the group index of a utilization clamp group
>> > + * @clamp_id: the utilization clamp index (i.e. min or max clamping)
>> > + * @clamp_value: the utilization clamping value lookup for
>> > + *
>> > + * Verify if a group has been assigned to a certain clamp value and return
>> > + * its index to be used for accounting.
>> > + *
>> > + * Since only a limited number of utilization clamp groups are allowed,
>> > if no
>> > + * groups have been assigned for the specified value, a new group is
>> > assigned
>> > + * if possible. Otherwise an error is returned, meaning that an
>> > additional clamp
>> > + * value is not (currently) supported.
>> > + */
>> > +static int
>> > +uclamp_group_find(int clamp_id, unsigned int clamp_value)
>> > +{
>> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
>> > + int free_group_id = UCLAMP_NONE;
>> > + unsigned int group_id = 0;
>> > +
>> > + for ( ; group_id <= CONFIG_UCLAMP_GROUPS_COUNT; ++group_id) {
>> > + /* Keep track of first free clamp group */
>> > + if (uclamp_group_available(clamp_id, group_id)) {
>> > + if (free_group_id == UCLAMP_NONE)
>> > + free_group_id = group_id;
>> > + continue;
>> > + }
>> > + /* Return index of first group with same clamp value */
>> > + if (uc_map[group_id].value == clamp_value)
>> > + return group_id;
>> > + }
>> > + /* Default to first free clamp group */
>> > + if (group_id > CONFIG_UCLAMP_GROUPS_COUNT)
>>
>> Is the condition above needed? I think it's always true if you got here.
>> Also AFAIKT after the for loop you can just do:
>>
>> return (free_group_id != UCLAMP_NONE) ? free_group_id : -ENOSPC;
>
> Yes, you right... the code above can be simplified!
>
>>
>> > + group_id = free_group_id;
>> > + /* All clamp group already track different clamp values */
>> > + if (group_id == UCLAMP_NONE)
>> > + return -ENOSPC;
>> > + return group_id;
>> > +}
>
> [...]
>
>> > +static inline void uclamp_group_put(int clamp_id, int group_id)
>> > +{
>> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
>> > + unsigned long flags;
>> > +
>> > + /* Ignore SE's not yet attached */
>> > + if (group_id == UCLAMP_NONE)
>> > + return;
>> > +
>> > + /* Remove SE from this clamp group */
>> > + raw_spin_lock_irqsave(_map[group_id].se_lock, flags);
>> > + uc_map[group_id].se_count -= 1;
>>
>> If uc_map[group_id].se_count was 0 before decrement you
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
Hi Suren,
thanks for the review, all good point... some more comments follow
inline.
On 19-Jul 16:51, Suren Baghdasaryan wrote:
> On Mon, Jul 16, 2018 at 1:28 AM, Patrick Bellasi
> wrote:
[...]
> > +/**
> > + * uclamp_group_available: checks if a clamp group is available
> > + * @clamp_id: the utilization clamp index (i.e. min or max clamp)
> > + * @group_id: the group index in the given clamp_id
> > + *
> > + * A clamp group is not free if there is at least one SE which is sing a
> > clamp
>
> Did you mean to say "single clamp"?
No, it's "...at least one SE which is USING a clamp value..."
> > + * value mapped on the specified clamp_id. These SEs are reference counted
> > by
> > + * the se_count of a uclamp_map entry.
> > + *
> > + * Return: true if there are no SE's mapped on the specified clamp
> > + * index and group
> > + */
> > +static inline bool uclamp_group_available(int clamp_id, int group_id)
> > +{
> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
> > +
> > + return (uc_map[group_id].value == UCLAMP_NONE);
>
> The usage of UCLAMP_NONE is very confusing to me. It was not used at
> all in the patch where it was introduced [1/12], here it's used as a
> clamp value and in uclamp_group_find() it's used as group_id. Please
> clarify the usage.
Yes, it's meant to represent a "clamp not valid" condition, whatever
it's a "clamp group" or a "clamp value"... perhaps the name can be
improved.
> I also feel UCLAMP_NONE does not really belong to
> the uclamp_id enum because other elements there are indexes in
> uclamp_maps and this one is a special value.
Right, it looks a bit misplaced, I agree. I think I tried to set it
using a #define but there was some issues I don't remember now...
Anyway, I'll give it another go...
> IMHO if both *group_id*
> and *value* need a special value (-1) to represent
> unused/uninitialized entry it would be better to use different
> constants. Maybe UCLAMP_VAL_NONE and UCLAMP_GROUP_NONE?
Yes, maybe we can use a
#define UCLAMP_NOT_VALID -1
and get rid the confusing enum entry.
Will update it on v3.
> > +}
[...]
> > +/**
> > + * uclamp_group_find: finds the group index of a utilization clamp group
> > + * @clamp_id: the utilization clamp index (i.e. min or max clamping)
> > + * @clamp_value: the utilization clamping value lookup for
> > + *
> > + * Verify if a group has been assigned to a certain clamp value and return
> > + * its index to be used for accounting.
> > + *
> > + * Since only a limited number of utilization clamp groups are allowed, if
> > no
> > + * groups have been assigned for the specified value, a new group is
> > assigned
> > + * if possible. Otherwise an error is returned, meaning that an additional
> > clamp
> > + * value is not (currently) supported.
> > + */
> > +static int
> > +uclamp_group_find(int clamp_id, unsigned int clamp_value)
> > +{
> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
> > + int free_group_id = UCLAMP_NONE;
> > + unsigned int group_id = 0;
> > +
> > + for ( ; group_id <= CONFIG_UCLAMP_GROUPS_COUNT; ++group_id) {
> > + /* Keep track of first free clamp group */
> > + if (uclamp_group_available(clamp_id, group_id)) {
> > + if (free_group_id == UCLAMP_NONE)
> > + free_group_id = group_id;
> > + continue;
> > + }
> > + /* Return index of first group with same clamp value */
> > + if (uc_map[group_id].value == clamp_value)
> > + return group_id;
> > + }
> > + /* Default to first free clamp group */
> > + if (group_id > CONFIG_UCLAMP_GROUPS_COUNT)
>
> Is the condition above needed? I think it's always true if you got here.
> Also AFAIKT after the for loop you can just do:
>
> return (free_group_id != UCLAMP_NONE) ? free_group_id : -ENOSPC;
Yes, you right... the code above can be simplified!
>
> > + group_id = free_group_id;
> > + /* All clamp group already track different clamp values */
> > + if (group_id == UCLAMP_NONE)
> > + return -ENOSPC;
> > + return group_id;
> > +}
[...]
> > +static inline void uclamp_group_put(int clamp_id, int group_id)
> > +{
> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
> > + unsigned long flags;
> > +
> > + /* Ignore SE's not yet attached */
> > + if (group_id == UCLAMP_NONE)
> > + return;
> > +
> > + /* Remove SE from this clamp group */
> > + raw_spin_lock_irqsave(_map[group_id].se_lock, flags);
> > + uc_map[group_id].se_count -= 1;
>
> If uc_map[group_id].se_count was 0 before decrement you end up with
> se_count == -1 and no reset for the element.
Well... this should never happen, otherwise the refcounting is not
working as expected.
Maybe we can add (at least) a debug check and warning, something like:
#ifdef SCHED_DEBUG
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
Hi Suren,
thanks for the review, all good point... some more comments follow
inline.
On 19-Jul 16:51, Suren Baghdasaryan wrote:
> On Mon, Jul 16, 2018 at 1:28 AM, Patrick Bellasi
> wrote:
[...]
> > +/**
> > + * uclamp_group_available: checks if a clamp group is available
> > + * @clamp_id: the utilization clamp index (i.e. min or max clamp)
> > + * @group_id: the group index in the given clamp_id
> > + *
> > + * A clamp group is not free if there is at least one SE which is sing a
> > clamp
>
> Did you mean to say "single clamp"?
No, it's "...at least one SE which is USING a clamp value..."
> > + * value mapped on the specified clamp_id. These SEs are reference counted
> > by
> > + * the se_count of a uclamp_map entry.
> > + *
> > + * Return: true if there are no SE's mapped on the specified clamp
> > + * index and group
> > + */
> > +static inline bool uclamp_group_available(int clamp_id, int group_id)
> > +{
> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
> > +
> > + return (uc_map[group_id].value == UCLAMP_NONE);
>
> The usage of UCLAMP_NONE is very confusing to me. It was not used at
> all in the patch where it was introduced [1/12], here it's used as a
> clamp value and in uclamp_group_find() it's used as group_id. Please
> clarify the usage.
Yes, it's meant to represent a "clamp not valid" condition, whatever
it's a "clamp group" or a "clamp value"... perhaps the name can be
improved.
> I also feel UCLAMP_NONE does not really belong to
> the uclamp_id enum because other elements there are indexes in
> uclamp_maps and this one is a special value.
Right, it looks a bit misplaced, I agree. I think I tried to set it
using a #define but there was some issues I don't remember now...
Anyway, I'll give it another go...
> IMHO if both *group_id*
> and *value* need a special value (-1) to represent
> unused/uninitialized entry it would be better to use different
> constants. Maybe UCLAMP_VAL_NONE and UCLAMP_GROUP_NONE?
Yes, maybe we can use a
#define UCLAMP_NOT_VALID -1
and get rid the confusing enum entry.
Will update it on v3.
> > +}
[...]
> > +/**
> > + * uclamp_group_find: finds the group index of a utilization clamp group
> > + * @clamp_id: the utilization clamp index (i.e. min or max clamping)
> > + * @clamp_value: the utilization clamping value lookup for
> > + *
> > + * Verify if a group has been assigned to a certain clamp value and return
> > + * its index to be used for accounting.
> > + *
> > + * Since only a limited number of utilization clamp groups are allowed, if
> > no
> > + * groups have been assigned for the specified value, a new group is
> > assigned
> > + * if possible. Otherwise an error is returned, meaning that an additional
> > clamp
> > + * value is not (currently) supported.
> > + */
> > +static int
> > +uclamp_group_find(int clamp_id, unsigned int clamp_value)
> > +{
> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
> > + int free_group_id = UCLAMP_NONE;
> > + unsigned int group_id = 0;
> > +
> > + for ( ; group_id <= CONFIG_UCLAMP_GROUPS_COUNT; ++group_id) {
> > + /* Keep track of first free clamp group */
> > + if (uclamp_group_available(clamp_id, group_id)) {
> > + if (free_group_id == UCLAMP_NONE)
> > + free_group_id = group_id;
> > + continue;
> > + }
> > + /* Return index of first group with same clamp value */
> > + if (uc_map[group_id].value == clamp_value)
> > + return group_id;
> > + }
> > + /* Default to first free clamp group */
> > + if (group_id > CONFIG_UCLAMP_GROUPS_COUNT)
>
> Is the condition above needed? I think it's always true if you got here.
> Also AFAIKT after the for loop you can just do:
>
> return (free_group_id != UCLAMP_NONE) ? free_group_id : -ENOSPC;
Yes, you right... the code above can be simplified!
>
> > + group_id = free_group_id;
> > + /* All clamp group already track different clamp values */
> > + if (group_id == UCLAMP_NONE)
> > + return -ENOSPC;
> > + return group_id;
> > +}
[...]
> > +static inline void uclamp_group_put(int clamp_id, int group_id)
> > +{
> > + struct uclamp_map *uc_map = _maps[clamp_id][0];
> > + unsigned long flags;
> > +
> > + /* Ignore SE's not yet attached */
> > + if (group_id == UCLAMP_NONE)
> > + return;
> > +
> > + /* Remove SE from this clamp group */
> > + raw_spin_lock_irqsave(_map[group_id].se_lock, flags);
> > + uc_map[group_id].se_count -= 1;
>
> If uc_map[group_id].se_count was 0 before decrement you end up with
> se_count == -1 and no reset for the element.
Well... this should never happen, otherwise the refcounting is not
working as expected.
Maybe we can add (at least) a debug check and warning, something like:
#ifdef SCHED_DEBUG
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
On Mon, Jul 16, 2018 at 1:28 AM, Patrick Bellasi
wrote:
> Utilization clamping requires each CPU to know which clamp values are
> assigned to tasks that are currently RUNNABLE on that CPU.
> Multiple tasks can be assigned the same clamp value and tasks with
> different clamp values can be concurrently active on the same CPU.
> Thus, a proper data structure is required to support a fast and
> efficient aggregation of the clamp values required by the currently
> RUNNABLE tasks.
>
> For this purpose we use a per-CPU array of reference counters,
> where each slot is used to account how many tasks require a certain
> clamp value are currently RUNNABLE on each CPU.
> Each clamp value corresponds to a "clamp index" which identifies the
> position within the array of reference couters.
>
> :
>(user-space changes) : (kernel space / scheduler)
> :
> SLOW PATH : FAST PATH
> :
> task_struct::uclamp::value : sched/core::enqueue/dequeue
> : cpufreq_schedutil
> :
> ++++ +---+
> | TASK || CLAMP GROUP| |CPU CLAMPS |
> ++++ +---+
> ||| clamp_{min,max} | | clamp_{min,max} |
> | util_{min,max} || se_count | |tasks count|
> ++++ +---+
> :
>+--> : +--->
> group_id = map(clamp_value) : ref_count(group_id)
> :
> :
>
> Let's introduce the support to map tasks to "clamp groups".
> Specifically we introduce the required functions to translate a
> "clamp value" into a clamp's "group index" (group_id).
>
> Only a limited number of (different) clamp values are supported since:
> 1. there are usually only few classes of workloads for which it makes
>sense to boost/limit to different frequencies,
>e.g. background vs foreground, interactive vs low-priority
> 2. it allows a simpler and more memory/time efficient tracking of
>the per-CPU clamp values in the fast path.
>
> The number of possible different clamp values is currently defined at
> compile time. Thus, setting a new clamp value for a task can result into
> a -ENOSPC error in case this will exceed the number of maximum different
> clamp values supported.
>
> Signed-off-by: Patrick Bellasi
> Cc: Ingo Molnar
> Cc: Peter Zijlstra
> Cc: Paul Turner
> Cc: Todd Kjos
> Cc: Joel Fernandes
> Cc: Juri Lelli
> Cc: Dietmar Eggemann
> Cc: Morten Rasmussen
> Cc: linux-kernel@vger.kernel.org
> Cc: linux...@vger.kernel.org
> ---
> include/linux/sched.h | 15 ++-
> init/Kconfig | 22
> kernel/sched/core.c | 300 +-
> 3 files changed, 330 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index fd8495723088..0635e8073cd3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -578,6 +578,19 @@ struct sched_dl_entity {
> struct hrtimer inactive_timer;
> };
>
> +/**
> + * Utilization's clamp group
> + *
> + * A utilization clamp group maps a "clamp value" (value), i.e.
> + * util_{min,max}, to a "clamp group index" (group_id).
> + */
> +struct uclamp_se {
> + /* Utilization constraint for tasks in this group */
> + unsigned int value;
> + /* Utilization clamp group for this constraint */
> + unsigned int group_id;
> +};
> +
> union rcu_special {
> struct {
> u8 blocked;
> @@ -662,7 +675,7 @@ struct task_struct {
>
> #ifdef CONFIG_UCLAMP_TASK
> /* Utlization clamp values for this task */
> - int uclamp[UCLAMP_CNT];
> + struct uclamp_seuclamp[UCLAMP_CNT];
> #endif
>
> #ifdef CONFIG_PREEMPT_NOTIFIERS
> diff --git a/init/Kconfig b/init/Kconfig
> index 1d45a6877d6f..0a377ad7c166 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -601,7 +601,29 @@ config UCLAMP_TASK
>
> If in doubt, say N.
>
> +config UCLAMP_GROUPS_COUNT
> + int "Number of different utilization clamp values supported"
> + range 0 127
> + default 2
> + depends on UCLAMP_TASK
> + help
> + This defines the maximum number of different utilization clamp
> + values which can be concurrently enforced for each utilization
> + clamp index (i.e. minimum and maximum utilization).
> +
> + Only a limited number of clamp values are supported because:
> + 1. there are usually only few classes of workloads for which it
> +
Re: [PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
On Mon, Jul 16, 2018 at 1:28 AM, Patrick Bellasi
wrote:
> Utilization clamping requires each CPU to know which clamp values are
> assigned to tasks that are currently RUNNABLE on that CPU.
> Multiple tasks can be assigned the same clamp value and tasks with
> different clamp values can be concurrently active on the same CPU.
> Thus, a proper data structure is required to support a fast and
> efficient aggregation of the clamp values required by the currently
> RUNNABLE tasks.
>
> For this purpose we use a per-CPU array of reference counters,
> where each slot is used to account how many tasks require a certain
> clamp value are currently RUNNABLE on each CPU.
> Each clamp value corresponds to a "clamp index" which identifies the
> position within the array of reference couters.
>
> :
>(user-space changes) : (kernel space / scheduler)
> :
> SLOW PATH : FAST PATH
> :
> task_struct::uclamp::value : sched/core::enqueue/dequeue
> : cpufreq_schedutil
> :
> ++++ +---+
> | TASK || CLAMP GROUP| |CPU CLAMPS |
> ++++ +---+
> ||| clamp_{min,max} | | clamp_{min,max} |
> | util_{min,max} || se_count | |tasks count|
> ++++ +---+
> :
>+--> : +--->
> group_id = map(clamp_value) : ref_count(group_id)
> :
> :
>
> Let's introduce the support to map tasks to "clamp groups".
> Specifically we introduce the required functions to translate a
> "clamp value" into a clamp's "group index" (group_id).
>
> Only a limited number of (different) clamp values are supported since:
> 1. there are usually only few classes of workloads for which it makes
>sense to boost/limit to different frequencies,
>e.g. background vs foreground, interactive vs low-priority
> 2. it allows a simpler and more memory/time efficient tracking of
>the per-CPU clamp values in the fast path.
>
> The number of possible different clamp values is currently defined at
> compile time. Thus, setting a new clamp value for a task can result into
> a -ENOSPC error in case this will exceed the number of maximum different
> clamp values supported.
>
> Signed-off-by: Patrick Bellasi
> Cc: Ingo Molnar
> Cc: Peter Zijlstra
> Cc: Paul Turner
> Cc: Todd Kjos
> Cc: Joel Fernandes
> Cc: Juri Lelli
> Cc: Dietmar Eggemann
> Cc: Morten Rasmussen
> Cc: linux-kernel@vger.kernel.org
> Cc: linux...@vger.kernel.org
> ---
> include/linux/sched.h | 15 ++-
> init/Kconfig | 22
> kernel/sched/core.c | 300 +-
> 3 files changed, 330 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index fd8495723088..0635e8073cd3 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -578,6 +578,19 @@ struct sched_dl_entity {
> struct hrtimer inactive_timer;
> };
>
> +/**
> + * Utilization's clamp group
> + *
> + * A utilization clamp group maps a "clamp value" (value), i.e.
> + * util_{min,max}, to a "clamp group index" (group_id).
> + */
> +struct uclamp_se {
> + /* Utilization constraint for tasks in this group */
> + unsigned int value;
> + /* Utilization clamp group for this constraint */
> + unsigned int group_id;
> +};
> +
> union rcu_special {
> struct {
> u8 blocked;
> @@ -662,7 +675,7 @@ struct task_struct {
>
> #ifdef CONFIG_UCLAMP_TASK
> /* Utlization clamp values for this task */
> - int uclamp[UCLAMP_CNT];
> + struct uclamp_seuclamp[UCLAMP_CNT];
> #endif
>
> #ifdef CONFIG_PREEMPT_NOTIFIERS
> diff --git a/init/Kconfig b/init/Kconfig
> index 1d45a6877d6f..0a377ad7c166 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -601,7 +601,29 @@ config UCLAMP_TASK
>
> If in doubt, say N.
>
> +config UCLAMP_GROUPS_COUNT
> + int "Number of different utilization clamp values supported"
> + range 0 127
> + default 2
> + depends on UCLAMP_TASK
> + help
> + This defines the maximum number of different utilization clamp
> + values which can be concurrently enforced for each utilization
> + clamp index (i.e. minimum and maximum utilization).
> +
> + Only a limited number of clamp values are supported because:
> + 1. there are usually only few classes of workloads for which it
> +
[PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
Utilization clamping requires each CPU to know which clamp values are
assigned to tasks that are currently RUNNABLE on that CPU.
Multiple tasks can be assigned the same clamp value and tasks with
different clamp values can be concurrently active on the same CPU.
Thus, a proper data structure is required to support a fast and
efficient aggregation of the clamp values required by the currently
RUNNABLE tasks.
For this purpose we use a per-CPU array of reference counters,
where each slot is used to account how many tasks require a certain
clamp value are currently RUNNABLE on each CPU.
Each clamp value corresponds to a "clamp index" which identifies the
position within the array of reference couters.
:
(user-space changes) : (kernel space / scheduler)
:
SLOW PATH : FAST PATH
:
task_struct::uclamp::value : sched/core::enqueue/dequeue
: cpufreq_schedutil
:
++++ +---+
| TASK || CLAMP GROUP| |CPU CLAMPS |
++++ +---+
||| clamp_{min,max} | | clamp_{min,max} |
| util_{min,max} || se_count | |tasks count|
++++ +---+
:
+--> : +--->
group_id = map(clamp_value) : ref_count(group_id)
:
:
Let's introduce the support to map tasks to "clamp groups".
Specifically we introduce the required functions to translate a
"clamp value" into a clamp's "group index" (group_id).
Only a limited number of (different) clamp values are supported since:
1. there are usually only few classes of workloads for which it makes
sense to boost/limit to different frequencies,
e.g. background vs foreground, interactive vs low-priority
2. it allows a simpler and more memory/time efficient tracking of
the per-CPU clamp values in the fast path.
The number of possible different clamp values is currently defined at
compile time. Thus, setting a new clamp value for a task can result into
a -ENOSPC error in case this will exceed the number of maximum different
clamp values supported.
Signed-off-by: Patrick Bellasi
Cc: Ingo Molnar
Cc: Peter Zijlstra
Cc: Paul Turner
Cc: Todd Kjos
Cc: Joel Fernandes
Cc: Juri Lelli
Cc: Dietmar Eggemann
Cc: Morten Rasmussen
Cc: linux-kernel@vger.kernel.org
Cc: linux...@vger.kernel.org
---
include/linux/sched.h | 15 ++-
init/Kconfig | 22
kernel/sched/core.c | 300 +-
3 files changed, 330 insertions(+), 7 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index fd8495723088..0635e8073cd3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -578,6 +578,19 @@ struct sched_dl_entity {
struct hrtimer inactive_timer;
};
+/**
+ * Utilization's clamp group
+ *
+ * A utilization clamp group maps a "clamp value" (value), i.e.
+ * util_{min,max}, to a "clamp group index" (group_id).
+ */
+struct uclamp_se {
+ /* Utilization constraint for tasks in this group */
+ unsigned int value;
+ /* Utilization clamp group for this constraint */
+ unsigned int group_id;
+};
+
union rcu_special {
struct {
u8 blocked;
@@ -662,7 +675,7 @@ struct task_struct {
#ifdef CONFIG_UCLAMP_TASK
/* Utlization clamp values for this task */
- int uclamp[UCLAMP_CNT];
+ struct uclamp_seuclamp[UCLAMP_CNT];
#endif
#ifdef CONFIG_PREEMPT_NOTIFIERS
diff --git a/init/Kconfig b/init/Kconfig
index 1d45a6877d6f..0a377ad7c166 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -601,7 +601,29 @@ config UCLAMP_TASK
If in doubt, say N.
+config UCLAMP_GROUPS_COUNT
+ int "Number of different utilization clamp values supported"
+ range 0 127
+ default 2
+ depends on UCLAMP_TASK
+ help
+ This defines the maximum number of different utilization clamp
+ values which can be concurrently enforced for each utilization
+ clamp index (i.e. minimum and maximum utilization).
+
+ Only a limited number of clamp values are supported because:
+ 1. there are usually only few classes of workloads for which it
+ makes sense to boost/cap for different frequencies,
+ e.g. background vs foreground, interactive vs low-priority.
+ 2. it allows a simpler and more memory/time efficient tracking of
+ the per-CPU clamp values.
+
+ Set to 0 (default
[PATCH v2 02/12] sched/core: uclamp: map TASK's clamp values into CPU's clamp groups
Utilization clamping requires each CPU to know which clamp values are
assigned to tasks that are currently RUNNABLE on that CPU.
Multiple tasks can be assigned the same clamp value and tasks with
different clamp values can be concurrently active on the same CPU.
Thus, a proper data structure is required to support a fast and
efficient aggregation of the clamp values required by the currently
RUNNABLE tasks.
For this purpose we use a per-CPU array of reference counters,
where each slot is used to account how many tasks require a certain
clamp value are currently RUNNABLE on each CPU.
Each clamp value corresponds to a "clamp index" which identifies the
position within the array of reference couters.
:
(user-space changes) : (kernel space / scheduler)
:
SLOW PATH : FAST PATH
:
task_struct::uclamp::value : sched/core::enqueue/dequeue
: cpufreq_schedutil
:
++++ +---+
| TASK || CLAMP GROUP| |CPU CLAMPS |
++++ +---+
||| clamp_{min,max} | | clamp_{min,max} |
| util_{min,max} || se_count | |tasks count|
++++ +---+
:
+--> : +--->
group_id = map(clamp_value) : ref_count(group_id)
:
:
Let's introduce the support to map tasks to "clamp groups".
Specifically we introduce the required functions to translate a
"clamp value" into a clamp's "group index" (group_id).
Only a limited number of (different) clamp values are supported since:
1. there are usually only few classes of workloads for which it makes
sense to boost/limit to different frequencies,
e.g. background vs foreground, interactive vs low-priority
2. it allows a simpler and more memory/time efficient tracking of
the per-CPU clamp values in the fast path.
The number of possible different clamp values is currently defined at
compile time. Thus, setting a new clamp value for a task can result into
a -ENOSPC error in case this will exceed the number of maximum different
clamp values supported.
Signed-off-by: Patrick Bellasi
Cc: Ingo Molnar
Cc: Peter Zijlstra
Cc: Paul Turner
Cc: Todd Kjos
Cc: Joel Fernandes
Cc: Juri Lelli
Cc: Dietmar Eggemann
Cc: Morten Rasmussen
Cc: linux-kernel@vger.kernel.org
Cc: linux...@vger.kernel.org
---
include/linux/sched.h | 15 ++-
init/Kconfig | 22
kernel/sched/core.c | 300 +-
3 files changed, 330 insertions(+), 7 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index fd8495723088..0635e8073cd3 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -578,6 +578,19 @@ struct sched_dl_entity {
struct hrtimer inactive_timer;
};
+/**
+ * Utilization's clamp group
+ *
+ * A utilization clamp group maps a "clamp value" (value), i.e.
+ * util_{min,max}, to a "clamp group index" (group_id).
+ */
+struct uclamp_se {
+ /* Utilization constraint for tasks in this group */
+ unsigned int value;
+ /* Utilization clamp group for this constraint */
+ unsigned int group_id;
+};
+
union rcu_special {
struct {
u8 blocked;
@@ -662,7 +675,7 @@ struct task_struct {
#ifdef CONFIG_UCLAMP_TASK
/* Utlization clamp values for this task */
- int uclamp[UCLAMP_CNT];
+ struct uclamp_seuclamp[UCLAMP_CNT];
#endif
#ifdef CONFIG_PREEMPT_NOTIFIERS
diff --git a/init/Kconfig b/init/Kconfig
index 1d45a6877d6f..0a377ad7c166 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -601,7 +601,29 @@ config UCLAMP_TASK
If in doubt, say N.
+config UCLAMP_GROUPS_COUNT
+ int "Number of different utilization clamp values supported"
+ range 0 127
+ default 2
+ depends on UCLAMP_TASK
+ help
+ This defines the maximum number of different utilization clamp
+ values which can be concurrently enforced for each utilization
+ clamp index (i.e. minimum and maximum utilization).
+
+ Only a limited number of clamp values are supported because:
+ 1. there are usually only few classes of workloads for which it
+ makes sense to boost/cap for different frequencies,
+ e.g. background vs foreground, interactive vs low-priority.
+ 2. it allows a simpler and more memory/time efficient tracking of
+ the per-CPU clamp values.
+
+ Set to 0 (default
