Hi Dietmar,
On Mon, Oct 26, 2020 at 08:00:48PM +0100, Dietmar Eggemann wrote:
> On 26/10/2020 16:45, Yun Hsiang wrote:
> > Hi Dietmar,
> >
> > On Mon, Oct 26, 2020 at 10:47:11AM +0100, Dietmar Eggemann wrote:
> >> On 25/10/2020 08:36, Yun Hsiang 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.
> >>>
> >>> The policy is
> >>> _CLAMP_RESET => reset both min and max
> >>> _CLAMP_RESET | _CLAMP_MIN => reset min value
> >>> _CLAMP_RESET | _CLAMP_MAX => reset max value
> >>> _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max
> >>>
> >>> Signed-off-by: Yun Hsiang <hsiang023...@gmail.com>
> >>
> >> [...]
> >>
> >>> @@ -1451,7 +1464,8 @@ static void __setscheduler_uclamp(struct
> >>> task_struct *p,
> >>> struct uclamp_se *uc_se = &p->uclamp_req[clamp_id];
> >>>
> >>> /* Keep using defined clamps across class changes */
> >>> - if (uc_se->user_defined)
> >>> + if (flags != SCHED_FLAG_UTIL_CLAMP_RESET &&
> >>> + uc_se->user_defined)
> >>> continue;
> >>
> >> With:
> >>
> >> (1) _CLAMP_RESET => reset both min and max
> >> (2) _CLAMP_RESET | _CLAMP_MIN => reset min value
> >> (3) _CLAMP_RESET | _CLAMP_MAX => reset max value
> >> (4) _CLAMP_RESET | _CLAMP_MIN | _CLAMP_MAX => reset both min and max
> >>
> >> If you reset an RT task with (1) you don't reset its uclamp.min value.
> >>
> >> __uclamp_update_util_min_rt_default(p) doesn't know about
> >> SCHED_FLAG_UTIL_CLAMP_RESET. It only knows user_defined and will bail
> >> early.
> >>
> >
> > Sorry I didn't notice __uclamp_update_util_min_rt_default will return
> > directly if user_defined is set, I'll fix it.
> >
> >> [...]
> >>
> >>> - if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP)))
> >>> + if (likely(!flags || flags == SCHED_FLAG_UTIL_CLAMP_RESET))
> >>> return;
> >>>
> >>> - if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> >>> + if (flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> >>> + if (reset) {
> >>> + clamp_value = __default_uclamp_value(p, UCLAMP_MIN);
> >>> + user_defined = false;
> >>> + } else {
> >>> + clamp_value = attr->sched_util_min;
> >>> + user_defined = true;
> >>> + }
> >>
> >> Why do you reset for (1) in the for_each_clamp_id(clamp_id) loop and for
> >> (2)-(4) in the if (flags & SCHED_FLAG_UTIL_CLAMP_MXX) condition?
> >>
> >> You could reset (1)-(4) in the for_each_clamp_id(clamp_id) loop? In this
> >> case you wouldn't need __default_uclamp_value().
> >
> > Do you mean adding these code in for_each_clamp_id(clamp_id) loop?
> >
> > if (clamp_id == UCLAMP_MIN) {
> > if (flags == SCHED_FLAG_UTIL_CLAMP_RESET ||
> > (reset && (flags || SCHED_FLAG_UTIL_CLAMP_MIN)) ||
> > !se->user_defined) {
> > if (task_rt(p)) {
> > clamp_value = sysctl_sched_uclamp_util_min_rt_default
> > } else {
> > clamp_value = uclamp_none(clamp_id);
> > }
> > } else
> > continue;
> > }
> > /* similar code for UCLAMP_MAX */
> > ...
> > uclamp_se_set(uc_se, clamp_value, false);
> >
> > It seems more clear.
> > If you think this one is better, I'll use this method and send patch V4.
>
> 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)
> +{
> + if (!(flags & SCHED_FLAG_UTIL_CLAMP_RESET))
> + return false;
> +
> + if (!(flags & (SCHED_FLAG_UTIL_CLAMP_MIN | SCHED_FLAG_UTIL_CLAMP_MAX)))
> + return true;
> +
> + if ((flags & SCHED_FLAG_UTIL_CLAMP_MIN) && clamp_id == UCLAMP_MIN)
> + return true;
> +
> + if ((flags & SCHED_FLAG_UTIL_CLAMP_MAX) && clamp_id == UCLAMP_MAX)
> + return true;
> +
> + 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,
> */
> 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;
> + }
>
> /*
> * 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;
> 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) {
> return;
> + }
>
> if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
Got it. This is much better. I'll test and send patch V4.
Thank for review and suggestions!
> --
> 2.17.1
>
>
>
Best Regards,
Yun
