Messages in this thread | | | Date | Tue, 8 Jun 2021 19:21:57 +0100 | From | Qais Yousef <> | Subject | Re: [PATCH] sched/uclamp: Avoid setting cpu.uclamp.min bigger than cpu.uclamp.max |
| |
On 06/08/21 23:01, Xuewen Yan wrote: > Hi > > On Tue, Jun 8, 2021 at 10:25 PM Qais Yousef <qais.yousef@arm.com> wrote: > > > > --->8--- > > > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > > index 9e9a5be35cde..1d2d3e6648a6 100644 > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -1403,38 +1403,28 @@ static void uclamp_sync_util_min_rt_default(void) > > static inline struct uclamp_se > > uclamp_tg_restrict(struct task_struct *p, enum uclamp_id clamp_id) > > { > > - struct uclamp_se uc_req = p->uclamp_req[clamp_id]; > > + /* Copy by value as we could modify it */ > > + struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; > > #ifdef CONFIG_UCLAMP_TASK_GROUP > > + unsigned long tg_min, tg_max, value; > > > > /* > > * Tasks in autogroups or root task group will be > > * restricted by system defaults. > > */ > > if (task_group_is_autogroup(task_group(p))) > > - return uc_req; > > + return uc_eff; > > if (task_group(p) == &root_task_group) > > - return uc_req; > > + return uc_eff; > > > > - switch (clamp_id) { > > - case UCLAMP_MIN: { > > - struct uclamp_se uc_min = task_group(p)->uclamp[clamp_id]; > > - if (uc_req.value < uc_min.value) > > - return uc_min; > > - break; > > - } > > - case UCLAMP_MAX: { > > - struct uclamp_se uc_max = task_group(p)->uclamp[clamp_id]; > > - if (uc_req.value > uc_max.value) > > - return uc_max; > > - break; > > - } > > - default: > > - WARN_ON_ONCE(1); > > - break; > > - } > > + tg_min = task_group(p)->uclamp[UCLAMP_MIN].value; > > + tg_max = task_group(p)->uclamp[UCLAMP_MAX].value; > > + value = uc_eff.value; > > + value = clamp(value, tg_min, tg_max); > > + uclamp_se_set(&uc_eff, value, false); > > Is it reasonable to set user_defined to be false here?
Yep, it doesn't really matter for the effective value. It matters for the actual task request.
> > > #endif > > > > - return uc_req; > > + return uc_eff; > > } > > > > /* > > @@ -1661,8 +1651,7 @@ uclamp_update_active(struct task_struct *p, enum uclamp_id clamp_id) > > > > #ifdef CONFIG_UCLAMP_TASK_GROUP > > static inline void > > -uclamp_update_active_tasks(struct cgroup_subsys_state *css, > > - unsigned int clamps) > > +uclamp_update_active_tasks(struct cgroup_subsys_state *css) > > { > > enum uclamp_id clamp_id; > > struct css_task_iter it; > > @@ -1670,10 +1659,8 @@ uclamp_update_active_tasks(struct cgroup_subsys_state *css, > > > > css_task_iter_start(css, 0, &it); > > while ((p = css_task_iter_next(&it))) { > > - for_each_clamp_id(clamp_id) { > > - if ((0x1 << clamp_id) & clamps) > > - uclamp_update_active(p, clamp_id); > > - } > > + for_each_clamp_id(clamp_id) > > + uclamp_update_active(p, clamp_id); > > } > > css_task_iter_end(&it); > > } > > @@ -9626,7 +9613,7 @@ static void cpu_util_update_eff(struct cgroup_subsys_state *css) > > } > > > > /* Immediately update descendants RUNNABLE tasks */ > > - uclamp_update_active_tasks(css, clamps); > > + uclamp_update_active_tasks(css); > > } > > } > > Would you resend another email? maybe it would be better to resend an > email with a new subject?
Yeah I will do a proper posting. But I need to stare at this a bit more then will do.
Thanks
-- Qais Yousef
| |