All of lore.kernel.org
 help / color / mirror / Atom feed
From: Valentin Schneider <valentin.schneider@arm.com>
To: Patrick Bellasi <patrick.bellasi@matbug.net>,
	Qais Yousef <qais.yousef@arm.com>
Cc: Ingo Molnar <mingo@redhat.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Kees Cook <keescook@chromium.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
	qperret@google.com, linux-kernel@vger.kernel.org,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] sched/rt: Add a new sysctl to control uclamp_util_min
Date: Thu, 9 Jan 2020 01:35:01 +0000	[thread overview]
Message-ID: <026e46e4-5d09-6260-0fa7-e365b0795c9a@arm.com> (raw)
In-Reply-To: <20200108185650.GA9635@darkstar>

On 08/01/2020 18:56, Patrick Bellasi wrote:
> Here you are force setting the task-specific _requests_ to match the
> system-wide _constraints_. This is not required and it's also
> conceptually wrong, since you mix two concepts: requests and
> constraints.
> 
> System-default values must never be synchronized with task-specific
> values. This allows to always satisfy task _requests_ when not
> conflicting with system-wide (or task-group) _constraints_.
> 
> For example, assuming we have a task with util_min=500 and we keep
> changing the system-wide constraints, we would like the following
> effective clamps to be enforced:
> 
>    time | system-wide | task-specific | effective clamp
>    -----+-------------+---------------+-----------------
>      t0 |        1024 |           500 |             500
>      t1 |           0 |           500 |               0
>      t2 |         200 |           500 |             200
>      t3 |         600 |           500 |             500
> 
> If the taks should then change it's requested util_min:
> 
>    time | system-wide | task-specific | effective clamp
>    -----+-------------+---------------+----------------
>      t4 |         600 |          800  |             600
>      t6 |        1024 |          800  |             800
> 
> If you force set the task-specific requests to match the system-wide
> constraints, you cannot get the above described behaviors since you
> keep overwriting the task _requests_ with system-wide _constraints_.
> 

But is what Qais' proposing really a system-wide *constraint*? What we want
to do here is have a knob for the RT uclamp.min values, because gotomax isn't
viable (for mobile, you know the story!). This leaves user_defined values
alone, so you should be able to reproduce exactly what you described above.
If I take your t3 and t4 examples:

| time | system-wide | rt default | task-specific | user_defined | effective |                       
|------+-------------+------------+---------------+--------------+-----------|                       
| t3   |         600 |       1024 |           500 | Y            |       500 |                       
| t4   |         600 |       1024 |           800 | Y            |       600 |

If the values were *not* user-defined, then it would depend on the default
knob Qais is introducing:

| time | system-wide | rt default | task-specific | user_defined | effective |                       
|------+-------------+------------+---------------+--------------+-----------|                       
| t3   |         600 |       1024 |          1024 | N            |       600 |                       
| t4   |         600 |          0 |             0 | N            |         0 | 

It's not forcing the task-specific value to the system-wide RT value, it's
just using it as tweakable default. At least that's how I understand it,
did I miss something?

> Thus, requests and contraints must always float independently and
> used to compute the effective clamp at task wakeup time via:
> 
>    enqueue_task(rq, p, flags)
>      uclamp_rq_inc(rq, p)
>        uclamp_rq_inc_id(rq, p, clamp_id)
>          uclamp_eff_get(p, clamp_id)
>            uclamp_tg_restrict(p, clamp_id)
>      p->sched_class->enqueue_task(rq, p, flags)
> 
> where the task-specific request is restricted considering its task group
> effective value (the constraint).
> 
> Do note that the root task group effective value (for cfs) tasks is kept
> in sync with the system default value and propagated down to the
> effective value of all subgroups.
> 
> Do note also that the effective value is computed before calling into
> the scheduling class's enqueue_task(). Which means that we have the
> right value in place before we poke sugov.
> 
> Thus, a proper implementation of what you need should just
> replicate/generalize what we already do for cfs tasks.
> 

Reading

  7274a5c1bbec ("sched/uclamp: Propagate system defaults to the root group")

I see "The clamp values are not tunable at the level of the root task group".
This means that, for mobile systems where we want a default uclamp.min of 0
for RT tasks, we would need to create a cgroup for all RT tasks (and tweak
its uclamp.min, but from playing around a bit I see that defaults to 0).

(Would we need CONFIG_RT_GROUP_SCHED for this? IIRC there's a few pain points
when turning it on, but I think we don't have to if we just want things like
uclamp value propagation?)

It's quite more work than the simple thing Qais is introducing (and on both
user and kernel side).

  reply	other threads:[~2020-01-09  1:35 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-20 16:48 [PATCH] sched/rt: Add a new sysctl to control uclamp_util_min Qais Yousef
2020-01-07 13:42 ` Quentin Perret
2020-01-07 19:30   ` Dietmar Eggemann
2020-01-08  9:51     ` Quentin Perret
2020-01-08 19:16       ` Patrick Bellasi
2020-01-09 11:36       ` Qais Yousef
2020-01-09 11:16     ` Qais Yousef
2020-01-09 11:12   ` Qais Yousef
2020-01-08 13:44 ` Peter Zijlstra
2020-01-08 19:08   ` Patrick Bellasi
2020-01-09 13:00   ` Qais Yousef
2020-01-10 13:39     ` Peter Zijlstra
2020-01-12 23:35       ` Qais Yousef
2020-01-10 13:42     ` Peter Zijlstra
2020-01-12 23:31       ` Qais Yousef
2020-01-08 18:56 ` Patrick Bellasi
2020-01-09  1:35   ` Valentin Schneider [this message]
2020-01-09  9:21     ` Patrick Bellasi
2020-01-09 13:38       ` Qais Yousef
2020-01-14 21:34       ` Qais Yousef
2020-01-22 10:19         ` Patrick Bellasi
2020-01-22 11:45           ` Qais Yousef
2020-01-22 12:44             ` Patrick Bellasi
2020-01-22 14:57               ` Qais Yousef
2020-01-09 13:15     ` Qais Yousef

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=026e46e4-5d09-6260-0fa7-e365b0795c9a@arm.com \
    --to=valentin.schneider@arm.com \
    --cc=bsegall@google.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=juri.lelli@redhat.com \
    --cc=keescook@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=patrick.bellasi@matbug.net \
    --cc=peterz@infradead.org \
    --cc=qais.yousef@arm.com \
    --cc=qperret@google.com \
    --cc=rostedt@goodmis.org \
    --cc=vincent.guittot@linaro.org \
    --cc=yzaikin@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.