On 10/29/20 21:02, Yun Hsiang wrote: > Hi Qais, > > On Thu, Oct 29, 2020 at 11:08:18AM +0000, Qais Yousef wrote: > > Hi Yun > > > > Sorry for chipping in late. > > > > On 10/25/20 15: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 <[email protected]> > > > --- > > > include/uapi/linux/sched.h | 7 +++++-- > > > kernel/sched/core.c | 41 +++++++++++++++++++++++++++++++------- > > > 2 files changed, 39 insertions(+), 9 deletions(-) > > > > > > diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h > > > index 3bac0a8ceab2..6c823ddb1a1e 100644 > > > --- a/include/uapi/linux/sched.h > > > +++ b/include/uapi/linux/sched.h > > > @@ -132,17 +132,20 @@ 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) > > > > Is it safe to change this define in a uapi header without a potential > > consequence? > > > > FWIW I still have concerns about this approach. We're doing a reset to > > control > > cgroup behavior, I don't see any correlation between the two. Besides the > > difference between RESET and setting uclamp_min=0 without RESET is not > > obvious > > nor intuitive for someone who didn't look at the code. > > > > I propose something like the below which is more explicit about what is > > being > > requested and delivered here. And if we decide to deprecate this behavior, > > it'd be much easier to just ignore this flag. > > > > You must set this flag with your uclamp request to retain the cgroup > > inheritance behavior. If the flag is not set, we automatically clear it. > > I think this behavior may not meet android requirement. Becasue in > android there is group like top-app. And we want to boost the > group by setting group uclamp_min. If group inheritance is explicit, we > need to set this flag for all the tasks in top-app. This might be > costly.
You will not have to set it for every task. It's on by default like it works now. This behavior doesn't change. But if you change the uclamp value of a task but still want it to continue to inherit the cgroup values if it's attached to one, you must set this flag when changing the uclamp value. Thanks -- Qais Yousef > > > > > Only compile tested. > > > > Thanks > > > > -- > > Qais Yousef > > > > > > --------->8----------- > > > > From fe48fde7dea582b3b075e80e6936e1199f24363d Mon Sep 17 00:00:00 2001 > > From: Qais Yousef <[email protected]> > > Date: Wed, 28 Oct 2020 22:36:26 +0000 > > Subject: [PATCH] sched/uclamp: Add new flag to control cgroup behavior > > > > Signed-off-by: Qais Yousef <[email protected]> > > --- > > include/linux/sched.h | 1 + > > include/uapi/linux/sched.h | 6 ++++++ > > kernel/sched/core.c | 25 ++++++++++++++++--------- > > 3 files changed, 23 insertions(+), 9 deletions(-) > > > > diff --git a/include/linux/sched.h b/include/linux/sched.h > > index afe01e232935..6eb35dfaa893 100644 > > --- a/include/linux/sched.h > > +++ b/include/linux/sched.h > > @@ -604,6 +604,7 @@ struct uclamp_se { > > unsigned int bucket_id : bits_per(UCLAMP_BUCKETS); > > unsigned int active : 1; > > unsigned int user_defined : 1; > > + unsigned int follow_cgroup : 1; > > }; > > #endif /* CONFIG_UCLAMP_TASK */ > > > > diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h > > index 3bac0a8ceab2..33ff716a7574 100644 > > --- a/include/uapi/linux/sched.h > > +++ b/include/uapi/linux/sched.h > > @@ -132,6 +132,12 @@ struct clone_args { > > #define SCHED_FLAG_KEEP_PARAMS 0x10 > > #define SCHED_FLAG_UTIL_CLAMP_MIN 0x20 > > #define SCHED_FLAG_UTIL_CLAMP_MAX 0x40 > > +/* > > + * Control whether a task follows/inherits the cgroup uclamp.min/max or > > not. > > + * By default this flag is set for all tasks. Any explicit modification to > > a > > + * task's UCLAMP_MIN/MAX must set this flag to retain the bahavior. > > + */ > > +#define SCHED_FLAG_UTIL_FOLLOW_CGROUP 0x80 > > > > #define SCHED_FLAG_KEEP_ALL (SCHED_FLAG_KEEP_POLICY | \ > > SCHED_FLAG_KEEP_PARAMS) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 2d95dc3f4644..1c77d6b8bd96 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -1083,7 +1083,7 @@ uclamp_tg_restrict(struct task_struct *p, enum > > uclamp_id clamp_id) > > return uc_req; > > > > uc_max = task_group(p)->uclamp[clamp_id]; > > - if (uc_req.value > uc_max.value || !uc_req.user_defined) > > + if (uc_req.value > uc_max.value || uc_req.follow_cgroup) > > return uc_max; > > #endif > > > > @@ -1446,6 +1446,7 @@ static void __setscheduler_uclamp(struct task_struct > > *p, > > const struct sched_attr *attr) > > { > > enum uclamp_id clamp_id; > > + bool follow_cgroup; > > > > /* > > * On scheduling class change, reset to default clamps for tasks > > @@ -1472,14 +1473,18 @@ static void __setscheduler_uclamp(struct > > task_struct *p, > > if (likely(!(attr->sched_flags & SCHED_FLAG_UTIL_CLAMP))) > > return; > > > > + follow_cgroup = attr->sched_flags & SCHED_FLAG_UTIL_FOLLOW_CGROUP; > > + > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MIN) { > > - uclamp_se_set(&p->uclamp_req[UCLAMP_MIN], > > - attr->sched_util_min, true); > > + struct uclamp_se *uc_se = &p->uclamp_req[UCLAMP_MIN]; > > + uc_se->follow_cgroup = follow_cgroup; > > + uclamp_se_set(uc_se, attr->sched_util_min, true); > > } > > > > if (attr->sched_flags & SCHED_FLAG_UTIL_CLAMP_MAX) { > > - uclamp_se_set(&p->uclamp_req[UCLAMP_MAX], > > - attr->sched_util_max, true); > > + struct uclamp_se *uc_se = &p->uclamp_req[UCLAMP_MAX]; > > + uc_se->follow_cgroup = follow_cgroup; > > + uclamp_se_set(uc_se, attr->sched_util_max, true); > > } > > } > > > > @@ -1498,8 +1503,9 @@ static void uclamp_fork(struct task_struct *p) > > return; > > > > for_each_clamp_id(clamp_id) { > > - uclamp_se_set(&p->uclamp_req[clamp_id], > > - uclamp_none(clamp_id), false); > > + struct uclamp_se *uc_se = &p->uclamp_req[clamp_id]; > > + uc_se->follow_cgroup = true; > > + uclamp_se_set(uc_se, uclamp_none(clamp_id), false); > > } > > } > > > > @@ -1532,8 +1538,9 @@ static void __init init_uclamp(void) > > init_uclamp_rq(cpu_rq(cpu)); > > > > for_each_clamp_id(clamp_id) { > > - uclamp_se_set(&init_task.uclamp_req[clamp_id], > > - uclamp_none(clamp_id), false); > > + struct uclamp_se *uc_se = &init_task.uclamp_req[clamp_id]; > > + uc_se->follow_cgroup = true; > > + uclamp_se_set(uc_se, uclamp_none(clamp_id), false); > > } > > > > /* System defaults allow max clamp values for both indexes */ > > -- > > 2.25.1
