Hi Qais,
On Tue, Apr 28, 2020 at 05:41:33PM +0100, Qais Yousef wrote:
[...]
>
> +static void uclamp_sync_util_min_rt_default(struct task_struct *p)
> +{
> + struct uclamp_se *uc_se = &p->uclamp_req[UCLAMP_MIN];
> +
> + if (unlikely(rt_task(p)) && !uc_se->user_defined)
> + uclamp_se_set(uc_se, sysctl_sched_uclamp_util_min_rt_default,
> false);
> +}
Unlike system default clamp values, RT default value is written to
p->uclamp_req[UCLAMP_MIN]. A user may not be able to set the uclamp.max to a
lower value than sysctl_sched_uclamp_util_min_rt_default. This is not a
big deal. Just sharing my observation. Is this how you expected it to work?
> +
> static inline struct uclamp_se
> uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id)
> {
> @@ -907,8 +935,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);
> +
> + uc_req = uclamp_tg_restrict(p, clamp_id);
> + uc_max = uclamp_default[clamp_id];
We are calling uclamp_sync_util_min_rt_default() unnecessarily for
clamp_id == UCLAMP_MAX case. Would it be better to have a separate
uclamp_default for RT like uclamp_default_rt and select uc_max based
on task policy? Since all tunables are handled in sysctl_sched_uclamp_handler
we can cover the case of uclamp_util_min < uclamp_util_min_rt.
>
> /* System default restrictions always apply */
> if (unlikely(uc_req.value > uc_max.value))
> @@ -1114,12 +1149,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 +1169,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 +1206,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 +1249,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
>
Thanks,
Pavan
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
Foundation Collaborative Project.
