Re: [PATCH v11 4/5] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps

2019-07-16 Thread Michal Koutný
On Tue, Jul 16, 2019 at 03:34:35PM +0100, Patrick Bellasi 
 wrote:
> Am I missing something?
No, it's rather my misinterpretation of the syscall semantics.

> Otherwise, I think the changelog sentence you quoted is just
> misleading.
It certainly mislead me to thinking about the sched_setattr calls as
requests of utilization being in the given interval (substituting 0 or 1 when
only one boundary is given, and further constrained by tg's interval).

I see your point, those are actually two (mostly) independent controls.
Makes sense now.

Thanks,
Michal


Re: [PATCH v11 4/5] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps

2019-07-16 Thread Patrick Bellasi
On 15-Jul 18:42, Michal Koutný wrote:
> On Mon, Jul 08, 2019 at 09:43:56AM +0100, Patrick Bellasi 
>  wrote:
> > This mimics what already happens for a task's CPU affinity mask when the
> > task is also in a cpuset, i.e. cgroup attributes are always used to
> > restrict per-task attributes.
> If I am not mistaken when set_schedaffinity(2) call is made that results
> in an empty cpuset, the call fails with EINVAL [1].
> 
> If I track the code correctly, the values passed to sched_setattr(2) are
> checked against the trivial validity (umin <= umax) and later on, they
> are adjusted to match the effective clamping of the containing
> task_group. Is that correct?
> 
> If the user attempted to sched_setattr [a, b], and the effective uclamp
> was [c, d] such that [a, b] ∩ [c, d] = ∅, the set uclamp will be
> silently moved out of their intended range. Wouldn't it be better to
> return with EINVAL too when the intersection is empty (since the user
> supplied range won't be attained)?

You right for the cpuset case, but I don't think we never end up with
a "empty" set in the case of utilization clamping.

We limit clamps hierarchically in such a way that:

  clamp[clamp_id] = min(task::clamp[clamp_id],
tg::clamp[clamp_id],
system::clamp[clamp_id])

and we ensure, on top of the above that:

  clamp[UCLAMP_MIN] = min(clamp[UCLAMP_MIN], clamp[UCLAMP_MAX])

Since it's all and only about "capping" values, at the very extreme
case you can end up with:

  clamp[UCLAMP_MIN] = clamp[UCLAMP_MAX] = 0

but that's till a valid configuration.

Am I missing something?

Otherwise, I think the changelog sentence you quoted is just
misleading.  I'll remove it from v12 since it does not really clarify
anything more then the rest of the message.

Cheers,
Patrick

-- 
#include 

Patrick Bellasi


Re: [PATCH v11 4/5] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps

2019-07-15 Thread Michal Koutný
On Mon, Jul 08, 2019 at 09:43:56AM +0100, Patrick Bellasi 
 wrote:
> This mimics what already happens for a task's CPU affinity mask when the
> task is also in a cpuset, i.e. cgroup attributes are always used to
> restrict per-task attributes.
If I am not mistaken when set_schedaffinity(2) call is made that results
in an empty cpuset, the call fails with EINVAL [1].

If I track the code correctly, the values passed to sched_setattr(2) are
checked against the trivial validity (umin <= umax) and later on, they
are adjusted to match the effective clamping of the containing
task_group. Is that correct?

If the user attempted to sched_setattr [a, b], and the effective uclamp
was [c, d] such that [a, b] ∩ [c, d] = ∅, the set uclamp will be
silently moved out of their intended range. Wouldn't it be better to
return with EINVAL too when the intersection is empty (since the user
supplied range won't be attained)?

Michal

[1] 
http://www.linux-arm.org/git?p=linux-pb.git;a=blob;f=kernel/sched/core.c;h=ddc5fcd4b9cfaa95496b24d8599c03bc140e768e;hb=2c15043a2a2b5d86eb409556cbe1e36d4fd275b5#l1660



[PATCH v11 4/5] sched/core: uclamp: Use TG's clamps to restrict TASK's clamps

2019-07-08 Thread Patrick Bellasi
When a task specific clamp value is configured via sched_setattr(2), this
value is accounted in the corresponding clamp bucket every time the task is
{en,de}qeued. However, when cgroups are also in use, the task specific
clamp values could be restricted by the task_group (TG) clamp values.

Update uclamp_cpu_inc() to aggregate task and TG clamp values. Every time a
task is enqueued, it's accounted in the clamp bucket tracking the smaller
clamp between the task specific value and its TG effective value. This
allows to:

1. ensure cgroup clamps are always used to restrict task specific requests,
   i.e. boosted not more than its TG effective protection and capped at
   least as its TG effective limit.

2. implement a "nice-like" policy, where tasks are still allowed to request
   less than what enforced by their TG effective limits and protections

This mimics what already happens for a task's CPU affinity mask when the
task is also in a cpuset, i.e. cgroup attributes are always used to
restrict per-task attributes.

Do this by exploiting the concept of "effective" clamp, which is already
used by a TG to track parent enforced restrictions.

Apply task group clamp restrictions only to tasks belonging to a child
group. While, for tasks in the root group or in an autogroup, only system
defaults are enforced.

Signed-off-by: Patrick Bellasi 
Cc: Ingo Molnar 
Cc: Peter Zijlstra 
Cc: Tejun Heo 
---
 kernel/sched/core.c | 28 +++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 276f9c2f6103..2591a70c85cf 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -873,16 +873,42 @@ unsigned int uclamp_rq_max_value(struct rq *rq, unsigned 
int clamp_id,
return uclamp_idle_value(rq, clamp_id, clamp_value);
 }
 
+static inline struct uclamp_se
+uclamp_tg_restrict(struct task_struct *p, unsigned int clamp_id)
+{
+   struct uclamp_se uc_req = p->uclamp_req[clamp_id];
+#ifdef CONFIG_UCLAMP_TASK_GROUP
+   struct uclamp_se uc_max;
+
+   /*
+* Tasks in autogroups or root task group will be
+* restricted by system defaults.
+*/
+   if (task_group_is_autogroup(task_group(p)))
+   return uc_req;
+   if (task_group(p) == _task_group)
+   return uc_req;
+
+   uc_max = task_group(p)->uclamp[clamp_id];
+   if (uc_req.value > uc_max.value || !uc_req.user_defined)
+   return uc_max;
+#endif
+
+   return uc_req;
+}
+
 /*
  * The effective clamp bucket index of a task depends on, by increasing
  * priority:
  * - the task specific clamp value, when explicitly requested from userspace
+ * - the task group effective clamp value, for tasks not either in the root
+ *   group or in an autogroup
  * - the system default clamp value, defined by the sysadmin
  */
 static inline struct uclamp_se
 uclamp_eff_get(struct task_struct *p, unsigned int clamp_id)
 {
-   struct uclamp_se uc_req = p->uclamp_req[clamp_id];
+   struct uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id);
struct uclamp_se uc_max = uclamp_default[clamp_id];
 
/* System default restrictions always apply */
-- 
2.21.0