Hi Dietmar, Yun,
I hope I'm not too late before v4 posting ;)
I think the overall approach is sound, I just added in a couple of
cleanups and a possible fix (user_defined reset).
Best,
Patrick
On Tue, Oct 27, 2020 at 16:58:13 +0100, Yun Hsiang <hsiang023...@gmail.com>
wrote...
> Hi Diet mar,
> On Mon, Oct 26, 2020 at 08:00:48PM +0100, Dietmar Eggemann wrote:
>> On 26/10/2020 16:45, Yun Hsiang wrote:
[...]
>> I thought about something like this. Only lightly tested.
>>
>> ---8<---
>>
>> From: Dietmar Eggemann <dietmar.eggem...@arm.com>
>> Date: Mon, 26 Oct 2020 13:52:23 +0100
>> Subject: [PATCH] SCHED_FLAG_UTIL_CLAMP_RESET
>>
>> Signed-off-by: Dietmar Eggemann <dietmar.eggem...@arm.com>
>> ---
>> include/uapi/linux/sched.h | 4 +++-
>> kernel/sched/core.c | 31 +++++++++++++++++++++++++++----
>> 2 files changed, 30 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
>> index 3bac0a8ceab2..0dd890822751 100644
>> --- a/include/uapi/linux/sched.h
>> +++ b/include/uapi/linux/sched.h
>> @@ -132,12 +132,14 @@ struct clone_args {
>> #define SCHED_FLAG_KEEP_PARAMS 0x10
>> #define SCHED_FLAG_UTIL_CLAMP_MIN 0x20
>> #define SCHED_FLAG_UTIL_CLAMP_MAX 0x40
>> +#define SCHED_FLAG_UTIL_CLAMP_RESET 0x80
>>
>> #define SCHED_FLAG_KEEP_ALL (SCHED_FLAG_KEEP_POLICY | \
>> SCHED_FLAG_KEEP_PARAMS)
>>
>> #define SCHED_FLAG_UTIL_CLAMP (SCHED_FLAG_UTIL_CLAMP_MIN | \
>> - SCHED_FLAG_UTIL_CLAMP_MAX)
>> + SCHED_FLAG_UTIL_CLAMP_MAX | \
>> + SCHED_FLAG_UTIL_CLAMP_RESET)
>>
>> #define SCHED_FLAG_ALL (SCHED_FLAG_RESET_ON_FORK | \
>> SCHED_FLAG_RECLAIM | \
>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> index 3dc415f58bd7..717b1cf5cf1f 100644
>> --- a/kernel/sched/core.c
>> +++ b/kernel/sched/core.c
>> @@ -1438,6 +1438,23 @@ static int uclamp_validate(struct task_struct *p,
>> return 0;
>> }
>>
>> +static bool uclamp_reset(enum uclamp_id clamp_id, unsigned long flags)
>> +{
Maybe we can add in some comments?
/* No _UCLAMP_RESET flag set: do not reset */
>> + if (!(flags & SCHED_FLAG_UTIL_CLAMP_RESET))
>> + return false;
>> +
/* Only _UCLAMP_RESET flag set: reset both clamps */
>> + if (!(flags & (SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX)))
>> + return true;
>> +
/* Both _UCLAMP_RESET and _UCLAMP_MIN flags are set: reset only min */
>> + if ((flags & SCHED_FLAG_UTIL_CLAMP_MIN) && clamp_id == UCLAMP_MIN)
>> + return true;
>> +
/* Both _UCLAMP_RESET and _UCLAMP_MAX flags are set: reset only max */
>> + if ((flags & SCHED_FLAG_UTIL_CLAMP_MAX) && clamp_id == UCLAMP_MAX)
>> + return true;
Since the evaluation ordering is important, do we have to better
_always_ use a READ_ONCE() for all flags accesses above, to ensure it is
preserved?
>> +
>> + return false;
>> +}
>> +
>> static void __setscheduler_uclamp(struct task_struct *p,
>> const struct sched_attr *attr)
>> {
>> @@ -1449,24 +1466,30 @@ static void __setscheduler_uclamp(struct task_struct
>> *p,
>> */
Perhaps we should update the comment above this loop with something
like:
/*
* Reset to default clamps on forced _UCLAMP_RESET (always) and
* for tasks without a task-specific value (on scheduling class change).
*/
>> for_each_clamp_id(clamp_id) {
>> struct uclamp_se *uc_se = &p->uclamp_req[clamp_id];
>> + unsigned int value;
>>
>> /* Keep using defined clamps across class changes */
>> - if (uc_se->user_defined)
>> + if (!uclamp_reset(clamp_id, attr->sched_flags) &&
>> + uc_se->user_defined) {
>> continue;
>> + }
I think we miss to reset the user_defined flag here.
What about replacing the above chunk with:
if (uclamp_reset(clamp_id, attr->sched_flags))
uc_se->user_defined = false;
if (uc-se->user_defined)
continue;
?
>>
>> /*
>> * RT by default have a 100% boost value that could be modified
>> * at runtime.
>> */
>> if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
>> - __uclamp_update_util_min_rt_default(p);
>> + value = sysctl_sched_uclamp_util_min_rt_default;
By removing this usage of __uclamp_updadate_util_min_rt_default(p),
the only other usage remaining is the call from:
uclamp_udpate_util_min_rt_default().
What about an additional cleanup by in-lining the only surviving usage?
>> else
>> - uclamp_se_set(uc_se, uclamp_none(clamp_id), false);
>> + value = uclamp_none(clamp_id);
>>
>> + uclamp_se_set(uc_se, value, false);
>> }
>>
>> - if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
>> + if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)) ||
>> + attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET) {
The likely() above should not wrap both conditions to be effective?
>> return;
>> + }
>>
>> if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
>> uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
