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.