| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20201029130649.h4wm2ak5j7zkgb3s@e107158-lin>
Date: Thu, 29 Oct 2020 13:06:49 +0000
From: Qais Yousef <qais.yousef@....com>
To: Yun Hsiang <hsiang023167@...il.com>
Cc: dietmar.eggemann@....com, peterz@...radead.org,
linux-kernel@...r.kernel.org, patrick.bellasi@...bug.net
Subject: Re: [PATCH v3 1/1] sched/uclamp: add SCHED_FLAG_UTIL_CLAMP_RESET
flag to reset uclamp
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 <hsiang023167@...il.com>
> > > ---
> > > 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 <qais.yousef@....com>
> > 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 <qais.yousef@....com>
> > ---
> > 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
Powered by blists - more mailing lists