Hi Patrick,
On Wed, Oct 28, 2020 at 11:11:07AM +0100, Patrick Bellasi wrote:
>
> 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?
>
I'll add these comment.
>
> /* 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?
>
Is this mean that we want to use READ_ONCE to avoid compiler reordering these
conditions?
> >> +
> >> + 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;
>
> ?
user_defined flag will be reset later by uclamp_se_set(uc_se, value,
false). But I agree to split it to two condition because it seems
clearer.
>
>
> >>
> >> /*
> >> * 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?
Got it.
>
> >> return;
> >> + }
> >>
> >> if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) {
> >> uclamp_se_set(&p->uclamp_req[UCLAMP_MIN],
