Hi Yun,
thanks for sharing this new implementation.
On Mon, Oct 12, 2020 at 18:31:40 +0200, Yun Hsiang <hsiang023...@gmail.com>
wrote...
> If the user wants to stop controlling uclamp and let the task inherit
> the value from the group, we need a method to reset.
>
> Add SCHED_FLAG_UTIL_CLAMP_RESET flag to allow the user to reset uclamp via
> sched_setattr syscall.
Looks like what you say here is not what you code, since you actually
add two new flags, _RESET_{MIN,MAX}.
I think we value instead a simple user-space interface where just the
additional one flag _RESET should be good enough.
> Signed-off-by: Yun Hsiang <hsiang023...@gmail.com>
> ---
> include/uapi/linux/sched.h | 9 ++++++++-
> kernel/sched/core.c | 16 ++++++++++++----
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> index 3bac0a8ceab2..a12e88c362d8 100644
> --- a/include/uapi/linux/sched.h
> +++ b/include/uapi/linux/sched.h
> @@ -132,6 +132,9 @@ 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_MIN 0x80
> +#define SCHED_FLAG_UTIL_CLAMP_RESET_MAX 0x100
What about adding just SCHED_FLAG_UTIL_CLAMP_RESET 0x08 ...
>
> #define SCHED_FLAG_KEEP_ALL (SCHED_FLAG_KEEP_POLICY | \
> SCHED_FLAG_KEEP_PARAMS)
> @@ -139,10 +142,14 @@ struct clone_args {
> #define SCHED_FLAG_UTIL_CLAMP (SCHED_FLAG_UTIL_CLAMP_MIN | \
> SCHED_FLAG_UTIL_CLAMP_MAX)
... making it part of SCHED_FLAG_UTIL_CLAMP ...
>
> +#define SCHED_FLAG_UTIL_CLAMP_RESET (SCHED_FLAG_UTIL_CLAMP_RESET_MIN | \
> + SCHED_FLAG_UTIL_CLAMP_RESET_MAX)
> +
> #define SCHED_FLAG_ALL (SCHED_FLAG_RESET_ON_FORK | \
> SCHED_FLAG_RECLAIM | \
> SCHED_FLAG_DL_OVERRUN | \
> SCHED_FLAG_KEEP_ALL | \
> - SCHED_FLAG_UTIL_CLAMP)
> + SCHED_FLAG_UTIL_CLAMP | \
> + SCHED_FLAG_UTIL_CLAMP_RESET)
... and use it in conjunction with the existing _CLAMP_{MIN,MAX} to know
which clamp should be reset?
> #endif /* _UAPI_LINUX_SCHED_H */
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9a2fbf98fd6f..ed4cb412dde7 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1207,15 +1207,22 @@ static void __setscheduler_uclamp(struct task_struct
> *p,
> uclamp_se_set(uc_se, clamp_value, false);
> }
>
> - if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
> + if (likely(!(attr->sched_flags &
> + (SCHED_FLAG_UTIL_CLAMP | SCHED_FLAG_UTIL_CLAMP_RESET))))
> return;
This check will not be changed, while we will have to add a bypass in
uclamp_validate().
>
> - if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> + if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET_MIN) {
> + uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
> + 0, false);
> + } else if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
> attr->sched_util_min, true);
> }
>
These checks also will have to be updated to check _RESET and
_{MIN,MAX} combinations.
Bonus point would be to be possible to pass in just the _RESET flag if
we want to reset both clamps. IOW, passing in _RESET only should be
consumed as if we passed in _RESET|_MIN|_MAX.
Caveat, RT tasks have got a special 'reset value' for _MIN.
We should check and ensure __uclamp_update_uti_min_rt_default() is
property called for those tasks, which likely will require some
additional refactoring :/
> - if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) {
> + if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_RESET_MAX) {
> + uclamp_se_set(&p->uclamp_req[UCLAMP_MAX],
> + 1024, false);
> + } else if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) {
> uclamp_se_set(&p->uclamp_req[UCLAMP_MAX],
> attr->sched_util_max, true);
> }
> @@ -4901,7 +4908,8 @@ static int __sched_setscheduler(struct task_struct *p,
> goto change;
> if (dl_policy(policy) && dl_param_changed(p, attr))
> goto change;
> - if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)
> + if (attr->sched_flags &
> + (SCHED_FLAG_UTIL_CLAMP |
> SCHED_FLAG_UTIL_CLAMP_RESET))
> goto change;
>
> p->sched_reset_on_fork = reset_on_fork;
