[PATCH v4 1/2] sched/uclamp: Add a new sysctl to control RT default boost value

[PATCH v4 1/2] sched/uclamp: Add a new sysctl to control RT default boost value

[Qais Yousef]

RT tasks by default run at the highest capacity/performance level. When
uclamp is selected this default behavior is retained by enforcing the
requested uclamp.min (p->uclamp_req[UCLAMP_MIN]) of the RT tasks to be
uclamp_none(UCLAMP_MAX), which is SCHED_CAPACITY_SCALE; the maximum
value.

This is also referred to as 'the default boost value of RT tasks'.

See commit 1a00d999971c ("sched/uclamp: Set default clamps for RT tasks").

On battery powered devices, it is desired to control this default
(currently hardcoded) behavior at runtime to reduce energy consumed by
RT tasks.

For example, a mobile device manufacturer where big.LITTLE architecture
is dominant, the performance of the little cores varies across SoCs, and
on high end ones the big cores could be too power hungry.

Given the diversity of SoCs, the new knob allows manufactures to tune
the best performance/power for RT tasks for the particular hardware they
run on.

They could opt to further tune the value when the user selects
a different power saving mode or when the device is actively charging.

The runtime aspect of it further helps in creating a single kernel image
that can be run on multiple devices that require different tuning.

Keep in mind that a lot of RT tasks in the system are created by the
kernel. On Android for instance I can see over 50 RT tasks, only
a handful of which created by the Android framework.

To control the default behavior globally by system admins and device
integrators, introduce the new sysctl_sched_uclamp_util_min_rt_default
to change the default boost value of the RT tasks.

I anticipate this to be mostly in the form of modifying the init script
of a particular device.

Whenever the new default changes, it'd be applied lazily on the next
opportunity the scheduler needs to calculate the effective uclamp.min
value for the task, assuming that it still uses the system default value
and not a user applied one.

Tested on Juno-r2 in combination with the RT capacity awareness [1].
By default an RT task will go to the highest capacity CPU and run at the
maximum frequency, which is particularly energy inefficient on high end
mobile devices because the biggest core[s] are 'huge' and power hungry.

With this patch the RT task can be controlled to run anywhere by
default, and doesn't cause the frequency to be maximum all the time.
Yet any task that really needs to be boosted can easily escape this
default behavior by modifying its requested uclamp.min value
(p->uclamp_req[UCLAMP_MIN]) via sched_setattr() syscall.

[1] 804d402fb6f6: ("sched/rt: Make RT capacity-aware")

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Jonathan Corbet <corbet@lwn.net>
CC: Juri Lelli <juri.lelli@redhat.com>
CC: Vincent Guittot <vincent.guittot@linaro.org>
CC: Dietmar Eggemann <dietmar.eggemann@arm.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ben Segall <bsegall@google.com>
CC: Mel Gorman <mgorman@suse.de>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Kees Cook <keescook@chromium.org>
CC: Iurii Zaikin <yzaikin@google.com>
CC: Quentin Perret <qperret@google.com>
CC: Valentin Schneider <valentin.schneider@arm.com>
CC: Patrick Bellasi <patrick.bellasi@matbug.net>
CC: Pavan Kondeti <pkondeti@codeaurora.org>
CC: Randy Dunlap <rdunlap@infradead.org>
CC: linux-doc@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
---

Changes in v4:
	* Make uclamp_sync_util_min_rt_default() inline and more selective
	  about when to do the sync (Pavan, Dietmar).

v3 discussion:

https://lore.kernel.org/lkml/20200428164134.5588-1-qais.yousef@arm.com/


 include/linux/sched/sysctl.h |  1 +
 kernel/sched/core.c          | 77 +++++++++++++++++++++++++++++++++---
 kernel/sysctl.c              |  7 ++++
 3 files changed, 80 insertions(+), 5 deletions(-)

diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
index d4f6215ee03f..e62cef019094 100644
--- a/include/linux/sched/sysctl.h
+++ b/include/linux/sched/sysctl.h
@@ -59,6 +59,7 @@ extern int sysctl_sched_rt_runtime;
 #ifdef CONFIG_UCLAMP_TASK
 extern unsigned int sysctl_sched_uclamp_util_min;
 extern unsigned int sysctl_sched_uclamp_util_max;
+extern unsigned int sysctl_sched_uclamp_util_min_rt_default;
 #endif
 
 #ifdef CONFIG_CFS_BANDWIDTH
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 9a2fbf98fd6f..15d2978e1869 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -790,6 +790,26 @@ unsigned int sysctl_sched_uclamp_util_min = SCHED_CAPACITY_SCALE;
 /* Max allowed maximum utilization */
 unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
 
+/*
+ * By default RT tasks run at the maximum performance point/capacity of the
+ * system. Uclamp enforces this by always setting UCLAMP_MIN of RT tasks to
+ * SCHED_CAPACITY_SCALE.
+ *
+ * This knob allows admins to change the default behavior when uclamp is being
+ * used. In battery powered devices, particularly, running at the maximum
+ * capacity and frequency will increase energy consumption and shorten the
+ * battery life.
+ *
+ * This knob only affects RT tasks that their uclamp_se->user_defined == false.
+ *
+ * This knob will not override the system default sched_util_clamp_min defined
+ * above.
+ *
+ * Any modification is applied lazily on the next attempt to calculate the
+ * effective value of the task.
+ */
+unsigned int sysctl_sched_uclamp_util_min_rt_default = SCHED_CAPACITY_SCALE;
+
 /* All clamps are required to be less or equal than these values */
 static struct uclamp_se uclamp_default[UCLAMP_CNT];
 
@@ -872,6 +892,28 @@ unsigned int uclamp_rq_max_value(struct rq *rq, enum uclamp_id clamp_id,
 	return uclamp_idle_value(rq, clamp_id, clamp_value);
 }
 
+static inline void uclamp_sync_util_min_rt_default(struct task_struct *p,
+						   enum uclamp_id clamp_id)
+{
+	struct uclamp_se *uc_se;
+
+	/* Only sync for UCLAMP_MIN and RT tasks */
+	if (clamp_id != UCLAMP_MIN || likely(!rt_task(p)))
+		return;
+
+	uc_se = &p->uclamp_req[UCLAMP_MIN];
+
+	/*
+	 * Only sync if user didn't override the default request and the sysctl
+	 * knob has changed.
+	 */
+	if (unlikely(uc_se->user_defined) ||
+	    likely(uc_se->value == sysctl_sched_uclamp_util_min_rt_default))
+		return;
+
+	uclamp_se_set(uc_se, sysctl_sched_uclamp_util_min_rt_default, false);
+}
+
 static inline struct uclamp_se
 uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
 {
@@ -907,8 +949,15 @@ uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
 static inline struct uclamp_se
 uclamp_eff_get(struct task_struct *p, enum uclamp_id clamp_id)
 {
-	struct uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id);
-	struct uclamp_se uc_max = uclamp_default[clamp_id];
+	struct uclamp_se uc_req, uc_max;
+
+	/*
+	 * Sync up any change to sysctl_sched_uclamp_util_min_rt_default value.
+	 */
+	uclamp_sync_util_min_rt_default(p, clamp_id);
+
+	uc_req = uclamp_tg_restrict(p, clamp_id);
+	uc_max = uclamp_default[clamp_id];
 
 	/* System default restrictions always apply */
 	if (unlikely(uc_req.value > uc_max.value))
@@ -1114,12 +1163,13 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
 				loff_t *ppos)
 {
 	bool update_root_tg = false;
-	int old_min, old_max;
+	int old_min, old_max, old_min_rt;
 	int result;
 
 	mutex_lock(&uclamp_mutex);
 	old_min = sysctl_sched_uclamp_util_min;
 	old_max = sysctl_sched_uclamp_util_max;
+	old_min_rt = sysctl_sched_uclamp_util_min_rt_default;
 
 	result = proc_dointvec(table, write, buffer, lenp, ppos);
 	if (result)
@@ -1133,6 +1183,18 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
 		goto undo;
 	}
 
+	/*
+	 * The new value will be applied to RT tasks the next time the
+	 * scheduler needs to calculate the effective uclamp.min for that task,
+	 * assuming the task is using the system default and not a user
+	 * specified value. In the latter we shall leave the value as the user
+	 * requested.
+	 */
+	if (sysctl_sched_uclamp_util_min_rt_default > SCHED_CAPACITY_SCALE) {
+		result = -EINVAL;
+		goto undo;
+	}
+
 	if (old_min != sysctl_sched_uclamp_util_min) {
 		uclamp_se_set(&uclamp_default[UCLAMP_MIN],
 			      sysctl_sched_uclamp_util_min, false);
@@ -1158,6 +1220,7 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
 undo:
 	sysctl_sched_uclamp_util_min = old_min;
 	sysctl_sched_uclamp_util_max = old_max;
+	sysctl_sched_uclamp_util_min_rt_default = old_min_rt;
 done:
 	mutex_unlock(&uclamp_mutex);
 
@@ -1200,9 +1263,13 @@ static void __setscheduler_uclamp(struct task_struct *p,
 		if (uc_se->user_defined)
 			continue;
 
-		/* By default, RT tasks always get 100% boost */
+		/*
+		 * By default, RT tasks always get 100% boost, which the admins
+		 * are allowed to change via
+		 * sysctl_sched_uclamp_util_min_rt_default knob.
+		 */
 		if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
-			clamp_value = uclamp_none(UCLAMP_MAX);
+			clamp_value = sysctl_sched_uclamp_util_min_rt_default;
 
 		uclamp_se_set(uc_se, clamp_value, false);
 	}
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 8a176d8727a3..64117363c502 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -453,6 +453,13 @@ static struct ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= sysctl_sched_uclamp_handler,
 	},
+	{
+		.procname	= "sched_util_clamp_min_rt_default",
+		.data		= &sysctl_sched_uclamp_util_min_rt_default,
+		.maxlen		= sizeof(unsigned int),
+		.mode		= 0644,
+		.proc_handler	= sysctl_sched_uclamp_handler,
+	},
 #endif
 #ifdef CONFIG_SCHED_AUTOGROUP
 	{
-- 
2.17.1

[PATCH v4 2/2] Documentation/sysctl: Document uclamp sysctl knobs

[Qais Yousef]

Uclamp exposes 3 sysctl knobs:

	* sched_util_clamp_min
	* sched_util_clamp_max
	* sched_util_clamp_min_rt_default

Document them in sysctl/kernel.rst.

Signed-off-by: Qais Yousef <qais.yousef@arm.com>
CC: Jonathan Corbet <corbet@lwn.net>
CC: Juri Lelli <juri.lelli@redhat.com>
CC: Vincent Guittot <vincent.guittot@linaro.org>
CC: Dietmar Eggemann <dietmar.eggemann@arm.com>
CC: Steven Rostedt <rostedt@goodmis.org>
CC: Ben Segall <bsegall@google.com>
CC: Mel Gorman <mgorman@suse.de>
CC: Luis Chamberlain <mcgrof@kernel.org>
CC: Kees Cook <keescook@chromium.org>
CC: Iurii Zaikin <yzaikin@google.com>
CC: Quentin Perret <qperret@google.com>
CC: Valentin Schneider <valentin.schneider@arm.com>
CC: Patrick Bellasi <patrick.bellasi@matbug.net>
CC: Pavan Kondeti <pkondeti@codeaurora.org>
CC: Randy Dunlap <rdunlap@infradead.org>
CC: linux-doc@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: linux-fsdevel@vger.kernel.org
---

Changes in v4:
	* Punctuation fixes (Randy Dunlap).


 Documentation/admin-guide/sysctl/kernel.rst | 48 +++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
index 0d427fd10941..521c18ce3d92 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -940,6 +940,54 @@ Enables/disables scheduler statistics. Enabling this feature
 incurs a small amount of overhead in the scheduler but is
 useful for debugging and performance tuning.
 
+sched_util_clamp_min:
+=====================
+
+Max allowed *minimum* utilization.
+
+Default value is SCHED_CAPACITY_SCALE (1024), which is the maximum possible
+value.
+
+It means that any requested uclamp.min value cannot be greater than
+sched_util_clamp_min, i.e., it is restricted to the range
+[0:sched_util_clamp_min].
+
+sched_util_clamp_max:
+=====================
+
+Max allowed *maximum* utilization.
+
+Default value is SCHED_CAPACITY_SCALE (1024), which is the maximum possible
+value.
+
+It means that any requested uclamp.max value cannot be greater than
+sched_util_clamp_max, i.e., it is restricted to the range
+[0:sched_util_clamp_max].
+
+sched_util_clamp_min_rt_default:
+================================
+
+By default Linux is tuned for performance. Which means that RT tasks always run
+at the highest frequency and most capable (highest capacity) CPU (in
+heterogeneous systems).
+
+Uclamp achieves this by setting the requested uclamp.min of all RT tasks to
+SCHED_CAPACITY_SCALE (1024) by default, which effectively boosts the tasks to
+run at the highest frequency and biases them to run on the biggest CPU.
+
+This knob allows admins to change the default behavior when uclamp is being
+used. In battery powered devices particularly, running at the maximum
+capacity and frequency will increase energy consumption and shorten the battery
+life.
+
+This knob is only effective for RT tasks which the user hasn't modified their
+requested uclamp.min value via sched_setattr() syscall.
+
+This knob will not escape the constraint imposed by sched_util_clamp_min
+defined above.
+
+Any modification is applied lazily on the next opportunity the scheduler needs
+to calculate the effective value of uclamp.min of the task.
 
 seccomp
 =======
-- 
2.17.1

Re: [PATCH v4 2/2] Documentation/sysctl: Document uclamp sysctl knobs

[Patrick Bellasi]

Hi Qais,

On Fri, May 01, 2020 at 13:49:27 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

[...]

> diff --git a/Documentation/admin-guide/sysctl/kernel.rst b/Documentation/admin-guide/sysctl/kernel.rst
> index 0d427fd10941..521c18ce3d92 100644
> --- a/Documentation/admin-guide/sysctl/kernel.rst
> +++ b/Documentation/admin-guide/sysctl/kernel.rst
> @@ -940,6 +940,54 @@ Enables/disables scheduler statistics. Enabling this feature
>  incurs a small amount of overhead in the scheduler but is
>  useful for debugging and performance tuning.
>  
> +sched_util_clamp_min:
> +=====================
> +
> +Max allowed *minimum* utilization.
> +
> +Default value is SCHED_CAPACITY_SCALE (1024), which is the maximum possible
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^

Mmm... I feel one of the two is an implementation detail which should
probably not be exposed?

The user perhaps needs to know the value (1024) but we don't need to
expose the internal representation.


> +value.
> +
> +It means that any requested uclamp.min value cannot be greater than
> +sched_util_clamp_min, i.e., it is restricted to the range
> +[0:sched_util_clamp_min].
> +
> +sched_util_clamp_max:
> +=====================
> +
> +Max allowed *maximum* utilization.
> +
> +Default value is SCHED_CAPACITY_SCALE (1024), which is the maximum possible
> +value.
> +
> +It means that any requested uclamp.max value cannot be greater than
> +sched_util_clamp_max, i.e., it is restricted to the range
> +[0:sched_util_clamp_max].
> +
> +sched_util_clamp_min_rt_default:
> +================================
> +
> +By default Linux is tuned for performance. Which means that RT tasks always run
> +at the highest frequency and most capable (highest capacity) CPU (in
> +heterogeneous systems).
> +
> +Uclamp achieves this by setting the requested uclamp.min of all RT tasks to
> +SCHED_CAPACITY_SCALE (1024) by default, which effectively boosts the tasks to
> +run at the highest frequency and biases them to run on the biggest CPU.
> +
> +This knob allows admins to change the default behavior when uclamp is being
> +used. In battery powered devices particularly, running at the maximum
> +capacity and frequency will increase energy consumption and shorten the battery
> +life.
> +
> +This knob is only effective for RT tasks which the user hasn't modified their
> +requested uclamp.min value via sched_setattr() syscall.
> +
> +This knob will not escape the constraint imposed by sched_util_clamp_min
> +defined above.

Perhaps it's worth to specify that this value is going to be clamped by
the values above? Otherwise it's a bit ambiguous to know what happen
when it's bigger than schedu_util_clamp_min.

> +Any modification is applied lazily on the next opportunity the scheduler needs
> +to calculate the effective value of uclamp.min of the task.
                    ^^^^^^^^^

This is also an implementation detail, I would remove it.

>  
>  seccomp
>  =======


Best,
Patrick

Re: [PATCH v4 2/2] Documentation/sysctl: Document uclamp sysctl knobs

[Qais Yousef]

Hi Patrick

On 05/03/20 19:45, Patrick Bellasi wrote:
> > +sched_util_clamp_min:
> > +=====================
> > +
> > +Max allowed *minimum* utilization.
> > +
> > +Default value is SCHED_CAPACITY_SCALE (1024), which is the maximum possible
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Mmm... I feel one of the two is an implementation detail which should
> probably not be exposed?
> 
> The user perhaps needs to know the value (1024) but we don't need to
> expose the internal representation.

Okay.

> 
> 
> > +value.
> > +
> > +It means that any requested uclamp.min value cannot be greater than
> > +sched_util_clamp_min, i.e., it is restricted to the range
> > +[0:sched_util_clamp_min].
> > +
> > +sched_util_clamp_max:
> > +=====================
> > +
> > +Max allowed *maximum* utilization.
> > +
> > +Default value is SCHED_CAPACITY_SCALE (1024), which is the maximum possible
> > +value.
> > +
> > +It means that any requested uclamp.max value cannot be greater than
> > +sched_util_clamp_max, i.e., it is restricted to the range
> > +[0:sched_util_clamp_max].
> > +
> > +sched_util_clamp_min_rt_default:
> > +================================
> > +
> > +By default Linux is tuned for performance. Which means that RT tasks always run
> > +at the highest frequency and most capable (highest capacity) CPU (in
> > +heterogeneous systems).
> > +
> > +Uclamp achieves this by setting the requested uclamp.min of all RT tasks to
> > +SCHED_CAPACITY_SCALE (1024) by default, which effectively boosts the tasks to
> > +run at the highest frequency and biases them to run on the biggest CPU.
> > +
> > +This knob allows admins to change the default behavior when uclamp is being
> > +used. In battery powered devices particularly, running at the maximum
> > +capacity and frequency will increase energy consumption and shorten the battery
> > +life.
> > +
> > +This knob is only effective for RT tasks which the user hasn't modified their
> > +requested uclamp.min value via sched_setattr() syscall.
> > +
> > +This knob will not escape the constraint imposed by sched_util_clamp_min
> > +defined above.
> 
> Perhaps it's worth to specify that this value is going to be clamped by
> the values above? Otherwise it's a bit ambiguous to know what happen
> when it's bigger than schedu_util_clamp_min.

Hmm for me that sentence says exactly what you're asking for.

So what you want is

	s/will not escape the constraint imposed by/will be clamped by/

?

I'm not sure if this will help if the above is already ambiguous. Maybe if
I explicitly say

	..will not escape the *range* constrained imposed by..

sched_util_clamp_min is already defined as a range constraint, so hopefully it
should hit the mark better now?

> 
> > +Any modification is applied lazily on the next opportunity the scheduler needs
> > +to calculate the effective value of uclamp.min of the task.
>                     ^^^^^^^^^
> 
> This is also an implementation detail, I would remove it.

The idea is that this value is not updated 'immediately'/synchronously. So
currently RUNNING tasks will not see the effect, which could generate confusion
when users trip over it. IMO giving an idea of how it's updated will help with
expectation of the users. I doubt any will care, but I think it's an important
behavior element that is worth conveying and documenting. I'd be happy to
reword it if necessary.

I have this now

"""
 984 This knob will not escape the range constraint imposed by sched_util_clamp_min
 985 defined above.
 986
 987 For example if
 988
 989         sched_util_clamp_min_rt_default = 800
 990         sched_util_clamp_min = 600
 991
 992 Then the boost will be clamped to 600 because 800 is outside of the permissible
 993 range of [0:600]. This could happen for instance if a powersave mode will
 994 restrict all boosts temporarily by modifying sched_util_clamp_min. As soon as
 995 this restriction is lifted, the requested sched_util_clamp_min_rt_default
 996 will take effect.
 997
 998 Any modification is applied lazily to currently running tasks and should be
 999 visible by the next wakeup.
"""

Thanks

--
Qais Yousef

Re: [PATCH v4 2/2] Documentation/sysctl: Document uclamp sysctl knobs

[Patrick Bellasi]

Hi Qais,

On Tue, May 05, 2020 at 16:56:37 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

>> > +sched_util_clamp_min_rt_default:
>> > +================================
>> > +
>> > +By default Linux is tuned for performance. Which means that RT tasks always run
>> > +at the highest frequency and most capable (highest capacity) CPU (in
>> > +heterogeneous systems).
>> > +
>> > +Uclamp achieves this by setting the requested uclamp.min of all RT tasks to
>> > +SCHED_CAPACITY_SCALE (1024) by default, which effectively boosts the tasks to
>> > +run at the highest frequency and biases them to run on the biggest CPU.
>> > +
>> > +This knob allows admins to change the default behavior when uclamp is being
>> > +used. In battery powered devices particularly, running at the maximum
>> > +capacity and frequency will increase energy consumption and shorten the battery
>> > +life.
>> > +
>> > +This knob is only effective for RT tasks which the user hasn't modified their
>> > +requested uclamp.min value via sched_setattr() syscall.
>> > +
>> > +This knob will not escape the constraint imposed by sched_util_clamp_min
>> > +defined above.
>> 
>> Perhaps it's worth to specify that this value is going to be clamped by
>> the values above? Otherwise it's a bit ambiguous to know what happen
>> when it's bigger than schedu_util_clamp_min.
>
> Hmm for me that sentence says exactly what you're asking for.
>
> So what you want is
>
> 	s/will not escape the constraint imposed by/will be clamped by/
>
> ?
>
> I'm not sure if this will help if the above is already ambiguous. Maybe if
> I explicitly say
>
> 	..will not escape the *range* constrained imposed by..
>
> sched_util_clamp_min is already defined as a range constraint, so hopefully it
> should hit the mark better now?

Right, that also can work.

>> 
>> > +Any modification is applied lazily on the next opportunity the scheduler needs
>> > +to calculate the effective value of uclamp.min of the task.
>>                     ^^^^^^^^^
>> 
>> This is also an implementation detail, I would remove it.
>
> The idea is that this value is not updated 'immediately'/synchronously. So
> currently RUNNING tasks will not see the effect, which could generate confusion
> when users trip over it. IMO giving an idea of how it's updated will help with
> expectation of the users. I doubt any will care, but I think it's an important
> behavior element that is worth conveying and documenting. I'd be happy to
> reword it if necessary.

Right, I agree on giving an hint on the lazy update. What I was pointing
out was mainly the reference to the 'effective' value. Maybe we can just
drop that word.

> I have this now
>
> """
>  984 This knob will not escape the range constraint imposed by sched_util_clamp_min
>  985 defined above.
>  986
>  987 For example if
>  988
>  989         sched_util_clamp_min_rt_default = 800
>  990         sched_util_clamp_min = 600
>  991
>  992 Then the boost will be clamped to 600 because 800 is outside of the permissible
>  993 range of [0:600]. This could happen for instance if a powersave mode will
>  994 restrict all boosts temporarily by modifying sched_util_clamp_min. As soon as
>  995 this restriction is lifted, the requested sched_util_clamp_min_rt_default
>  996 will take effect.
>  997
>  998 Any modification is applied lazily to currently running tasks and should be
>  999 visible by the next wakeup.
> """

That's better IMHO, would just slightly change the last sentence to:

       Any modification is applied lazily to tasks and is effective
       starting from their next wakeup.

Best,
Patrick

Re: [PATCH v4 2/2] Documentation/sysctl: Document uclamp sysctl knobs

[Qais Yousef]

Hi Patrick

On 05/11/20 15:00, Patrick Bellasi wrote:

[...]

> > I have this now
> >
> > """
> >  984 This knob will not escape the range constraint imposed by sched_util_clamp_min
> >  985 defined above.
> >  986
> >  987 For example if
> >  988
> >  989         sched_util_clamp_min_rt_default = 800
> >  990         sched_util_clamp_min = 600
> >  991
> >  992 Then the boost will be clamped to 600 because 800 is outside of the permissible
> >  993 range of [0:600]. This could happen for instance if a powersave mode will
> >  994 restrict all boosts temporarily by modifying sched_util_clamp_min. As soon as
> >  995 this restriction is lifted, the requested sched_util_clamp_min_rt_default
> >  996 will take effect.
> >  997
> >  998 Any modification is applied lazily to currently running tasks and should be
> >  999 visible by the next wakeup.
> > """
> 
> That's better IMHO, would just slightly change the last sentence to:
> 
>        Any modification is applied lazily to tasks and is effective
>        starting from their next wakeup.

+1, will post v5 later today.

Thanks

--
Qais Yousef

Re: [PATCH v4 1/2] sched/uclamp: Add a new sysctl to control RT default boost value

[Patrick Bellasi]

Hi Qais,

few notes follows, but in general I like the way code is now organised.

On Fri, May 01, 2020 at 13:49:26 +0200, Qais Yousef <qais.yousef@arm.com> wrote...

[...]

> diff --git a/include/linux/sched/sysctl.h b/include/linux/sched/sysctl.h
> index d4f6215ee03f..e62cef019094 100644
> --- a/include/linux/sched/sysctl.h
> +++ b/include/linux/sched/sysctl.h
> @@ -59,6 +59,7 @@ extern int sysctl_sched_rt_runtime;
>  #ifdef CONFIG_UCLAMP_TASK
>  extern unsigned int sysctl_sched_uclamp_util_min;
>  extern unsigned int sysctl_sched_uclamp_util_max;
> +extern unsigned int sysctl_sched_uclamp_util_min_rt_default;
>  #endif
>  
>  #ifdef CONFIG_CFS_BANDWIDTH
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 9a2fbf98fd6f..15d2978e1869 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -790,6 +790,26 @@ unsigned int sysctl_sched_uclamp_util_min = SCHED_CAPACITY_SCALE;
>  /* Max allowed maximum utilization */
>  unsigned int sysctl_sched_uclamp_util_max = SCHED_CAPACITY_SCALE;
>  
> +/*
> + * By default RT tasks run at the maximum performance point/capacity of the
> + * system. Uclamp enforces this by always setting UCLAMP_MIN of RT tasks to
> + * SCHED_CAPACITY_SCALE.
> + *
> + * This knob allows admins to change the default behavior when uclamp is being
> + * used. In battery powered devices, particularly, running at the maximum
> + * capacity and frequency will increase energy consumption and shorten the
> + * battery life.
> + *
> + * This knob only affects RT tasks that their uclamp_se->user_defined == false.
> + *
> + * This knob will not override the system default sched_util_clamp_min defined
> + * above.
> + *
> + * Any modification is applied lazily on the next attempt to calculate the
> + * effective value of the task.
> + */
> +unsigned int sysctl_sched_uclamp_util_min_rt_default = SCHED_CAPACITY_SCALE;
> +
>  /* All clamps are required to be less or equal than these values */
>  static struct uclamp_se uclamp_default[UCLAMP_CNT];
>  
> @@ -872,6 +892,28 @@ unsigned int uclamp_rq_max_value(struct rq *rq, enum uclamp_id clamp_id,
>  	return uclamp_idle_value(rq, clamp_id, clamp_value);
>  }
>  
> +static inline void uclamp_sync_util_min_rt_default(struct task_struct *p,
> +						   enum uclamp_id clamp_id)
> +{
> +	struct uclamp_se *uc_se;
> +
> +	/* Only sync for UCLAMP_MIN and RT tasks */
> +	if (clamp_id != UCLAMP_MIN || likely(!rt_task(p)))
                                      ^^^^^^
Are we sure that likely makes any difference when used like that?

I believe you should either use:

	if (likely(clamp_id != UCLAMP_MIN || !rt_task(p)))

or completely drop it.

> +		return;
> +
> +	uc_se = &p->uclamp_req[UCLAMP_MIN];

nit-pick: you can probably move this at declaration time.

The compiler will be smart enough to either post-pone the init or, given
the likely() above, "pre-fetch" the value.

Anyway, the compiler is likely smarter then us. :)

> +
> +	/*
> +	 * Only sync if user didn't override the default request and the sysctl
> +	 * knob has changed.
> +	 */
> +	if (unlikely(uc_se->user_defined) ||
> +	    likely(uc_se->value == sysctl_sched_uclamp_util_min_rt_default))
> +		return;

Same here, I believe likely/unlikely work only if wrapping a full if()
condition. Thus, you should probably better split the above in two
separate checks, which also makes for a better inline doc.

> +
> +	uclamp_se_set(uc_se, sysctl_sched_uclamp_util_min_rt_default, false);

Nit-pick: perhaps we can also improve a bit readability by defining at
the beginning an alias variable with a shorter name, e.g.

       unsigned int uclamp_min =  sysctl_sched_uclamp_util_min_rt_default;

?

> +}
> +
>  static inline struct uclamp_se
>  uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
>  {
> @@ -907,8 +949,15 @@ uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
>  static inline struct uclamp_se
>  uclamp_eff_get(struct task_struct *p, enum uclamp_id clamp_id)
>  {
> -	struct uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id);
> -	struct uclamp_se uc_max = uclamp_default[clamp_id];
> +	struct uclamp_se uc_req, uc_max;
> +
> +	/*
> +	 * Sync up any change to sysctl_sched_uclamp_util_min_rt_default value.
                                                                         ^^^^^
> +	 */

nit-pick: we can use a single line comment if you drop the (useless)
'value' at the end.

> +	uclamp_sync_util_min_rt_default(p, clamp_id);
> +
> +	uc_req = uclamp_tg_restrict(p, clamp_id);
> +	uc_max = uclamp_default[clamp_id];
>  
>  	/* System default restrictions always apply */
>  	if (unlikely(uc_req.value > uc_max.value))
> @@ -1114,12 +1163,13 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
>  				loff_t *ppos)
>  {
>  	bool update_root_tg = false;
> -	int old_min, old_max;
> +	int old_min, old_max, old_min_rt;
>  	int result;
>  
>  	mutex_lock(&uclamp_mutex);
>  	old_min = sysctl_sched_uclamp_util_min;
>  	old_max = sysctl_sched_uclamp_util_max;
> +	old_min_rt = sysctl_sched_uclamp_util_min_rt_default;
>  
>  	result = proc_dointvec(table, write, buffer, lenp, ppos);
>  	if (result)
> @@ -1133,6 +1183,18 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
>  		goto undo;
>  	}
>  
> +	/*
> +	 * The new value will be applied to RT tasks the next time the
> +	 * scheduler needs to calculate the effective uclamp.min for that task,
> +	 * assuming the task is using the system default and not a user
> +	 * specified value. In the latter we shall leave the value as the user
> +	 * requested.

IMO it does not make sense to explain here what you will do with this
value. This will make even more complicated to maintain the comment
above if the code using it should change in the future.

So, if the code where we use the knob is not clear enough, maybe we can
move this comment to the description of:
   uclamp_sync_util_min_rt_default()
or to be part of the documentation of:
  sysctl_sched_uclamp_util_min_rt_default

By doing that you can also just add this if condition with the previous ones.

> +	 */
> +	if (sysctl_sched_uclamp_util_min_rt_default > SCHED_CAPACITY_SCALE) {
> +		result = -EINVAL;
> +		goto undo;
> +	}
> +
>  	if (old_min != sysctl_sched_uclamp_util_min) {
>  		uclamp_se_set(&uclamp_default[UCLAMP_MIN],
>  			      sysctl_sched_uclamp_util_min, false);
> @@ -1158,6 +1220,7 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
>  undo:
>  	sysctl_sched_uclamp_util_min = old_min;
>  	sysctl_sched_uclamp_util_max = old_max;
> +	sysctl_sched_uclamp_util_min_rt_default = old_min_rt;
>  done:
>  	mutex_unlock(&uclamp_mutex);
>  
> @@ -1200,9 +1263,13 @@ static void __setscheduler_uclamp(struct task_struct *p,
>  		if (uc_se->user_defined)
>  			continue;
>  
> -		/* By default, RT tasks always get 100% boost */
> +		/*
> +		 * By default, RT tasks always get 100% boost, which the admins
> +		 * are allowed to change via
> +		 * sysctl_sched_uclamp_util_min_rt_default knob.
> +		 */
>  		if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> -			clamp_value = uclamp_none(UCLAMP_MAX);
> +			clamp_value = sysctl_sched_uclamp_util_min_rt_default;

Mmm... I suspect we don't need this anymore.

If the task has a user_defined value, we skip this anyway.
If the task has not a user_defined value, we will do set this anyway at
each enqueue time.

No?

>  
>  		uclamp_se_set(uc_se, clamp_value, false);
>  	}
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 8a176d8727a3..64117363c502 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -453,6 +453,13 @@ static struct ctl_table kern_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= sysctl_sched_uclamp_handler,
>  	},
> +	{
> +		.procname	= "sched_util_clamp_min_rt_default",
> +		.data		= &sysctl_sched_uclamp_util_min_rt_default,
> +		.maxlen		= sizeof(unsigned int),
> +		.mode		= 0644,
> +		.proc_handler	= sysctl_sched_uclamp_handler,
> +	},
>  #endif
>  #ifdef CONFIG_SCHED_AUTOGROUP
>  	{

Best,
Patrick

Re: [PATCH v4 1/2] sched/uclamp: Add a new sysctl to control RT default boost value

[Qais Yousef]

On 05/03/20 19:37, Patrick Bellasi wrote:

[...]

> > +static inline void uclamp_sync_util_min_rt_default(struct task_struct *p,
> > +						   enum uclamp_id clamp_id)
> > +{
> > +	struct uclamp_se *uc_se;
> > +
> > +	/* Only sync for UCLAMP_MIN and RT tasks */
> > +	if (clamp_id != UCLAMP_MIN || likely(!rt_task(p)))
>                                       ^^^^^^
> Are we sure that likely makes any difference when used like that?
> 
> I believe you should either use:
> 
> 	if (likely(clamp_id != UCLAMP_MIN || !rt_task(p)))
> 
> or completely drop it.

I agree all these likely/unlikely better dropped.

> 
> > +		return;
> > +
> > +	uc_se = &p->uclamp_req[UCLAMP_MIN];
> 
> nit-pick: you can probably move this at declaration time.
> 
> The compiler will be smart enough to either post-pone the init or, given
> the likely() above, "pre-fetch" the value.
> 
> Anyway, the compiler is likely smarter then us. :)

I'll fling this question to the reviewers who voiced concerns about the
overhead. Personally I see the v3 implementation is the best fit :)

> 
> > +
> > +	/*
> > +	 * Only sync if user didn't override the default request and the sysctl
> > +	 * knob has changed.
> > +	 */
> > +	if (unlikely(uc_se->user_defined) ||
> > +	    likely(uc_se->value == sysctl_sched_uclamp_util_min_rt_default))
> > +		return;
> 
> Same here, I believe likely/unlikely work only if wrapping a full if()
> condition. Thus, you should probably better split the above in two
> separate checks, which also makes for a better inline doc.
> 
> > +
> > +	uclamp_se_set(uc_se, sysctl_sched_uclamp_util_min_rt_default, false);
> 
> Nit-pick: perhaps we can also improve a bit readability by defining at
> the beginning an alias variable with a shorter name, e.g.
> 
>        unsigned int uclamp_min =  sysctl_sched_uclamp_util_min_rt_default;
> 
> ?

Could do. I used default_util_min as a name though.

> 
> > +}
> > +
> >  static inline struct uclamp_se
> >  uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
> >  {
> > @@ -907,8 +949,15 @@ uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
> >  static inline struct uclamp_se
> >  uclamp_eff_get(struct task_struct *p, enum uclamp_id clamp_id)
> >  {
> > -	struct uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id);
> > -	struct uclamp_se uc_max = uclamp_default[clamp_id];
> > +	struct uclamp_se uc_req, uc_max;
> > +
> > +	/*
> > +	 * Sync up any change to sysctl_sched_uclamp_util_min_rt_default value.
>                                                                          ^^^^^
> > +	 */
> 
> nit-pick: we can use a single line comment if you drop the (useless)
> 'value' at the end.

Okay.

> 
> > +	uclamp_sync_util_min_rt_default(p, clamp_id);
> > +
> > +	uc_req = uclamp_tg_restrict(p, clamp_id);
> > +	uc_max = uclamp_default[clamp_id];
> >  
> >  	/* System default restrictions always apply */
> >  	if (unlikely(uc_req.value > uc_max.value))
> > @@ -1114,12 +1163,13 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
> >  				loff_t *ppos)
> >  {
> >  	bool update_root_tg = false;
> > -	int old_min, old_max;
> > +	int old_min, old_max, old_min_rt;
> >  	int result;
> >  
> >  	mutex_lock(&uclamp_mutex);
> >  	old_min = sysctl_sched_uclamp_util_min;
> >  	old_max = sysctl_sched_uclamp_util_max;
> > +	old_min_rt = sysctl_sched_uclamp_util_min_rt_default;
> >  
> >  	result = proc_dointvec(table, write, buffer, lenp, ppos);
> >  	if (result)
> > @@ -1133,6 +1183,18 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
> >  		goto undo;
> >  	}
> >  
> > +	/*
> > +	 * The new value will be applied to RT tasks the next time the
> > +	 * scheduler needs to calculate the effective uclamp.min for that task,
> > +	 * assuming the task is using the system default and not a user
> > +	 * specified value. In the latter we shall leave the value as the user
> > +	 * requested.
> 
> IMO it does not make sense to explain here what you will do with this
> value. This will make even more complicated to maintain the comment
> above if the code using it should change in the future.
> 
> So, if the code where we use the knob is not clear enough, maybe we can
> move this comment to the description of:
>    uclamp_sync_util_min_rt_default()
> or to be part of the documentation of:
>   sysctl_sched_uclamp_util_min_rt_default
> 
> By doing that you can also just add this if condition with the previous ones.

Okay.

> 
> > +	 */
> > +	if (sysctl_sched_uclamp_util_min_rt_default > SCHED_CAPACITY_SCALE) {
> > +		result = -EINVAL;
> > +		goto undo;
> > +	}
> > +
> >  	if (old_min != sysctl_sched_uclamp_util_min) {
> >  		uclamp_se_set(&uclamp_default[UCLAMP_MIN],
> >  			      sysctl_sched_uclamp_util_min, false);
> > @@ -1158,6 +1220,7 @@ int sysctl_sched_uclamp_handler(struct ctl_table *table, int write,
> >  undo:
> >  	sysctl_sched_uclamp_util_min = old_min;
> >  	sysctl_sched_uclamp_util_max = old_max;
> > +	sysctl_sched_uclamp_util_min_rt_default = old_min_rt;
> >  done:
> >  	mutex_unlock(&uclamp_mutex);
> >  
> > @@ -1200,9 +1263,13 @@ static void __setscheduler_uclamp(struct task_struct *p,
> >  		if (uc_se->user_defined)
> >  			continue;
> >  
> > -		/* By default, RT tasks always get 100% boost */
> > +		/*
> > +		 * By default, RT tasks always get 100% boost, which the admins
> > +		 * are allowed to change via
> > +		 * sysctl_sched_uclamp_util_min_rt_default knob.
> > +		 */
> >  		if (unlikely(rt_task(p) && clamp_id == UCLAMP_MIN))
> > -			clamp_value = uclamp_none(UCLAMP_MAX);
> > +			clamp_value = sysctl_sched_uclamp_util_min_rt_default;
> 
> Mmm... I suspect we don't need this anymore.
> 
> If the task has a user_defined value, we skip this anyway.
> If the task has not a user_defined value, we will do set this anyway at
> each enqueue time.
> 
> No?

Indeed.

Thanks

--
Qais Yousef