Messages in this thread Patch in this message | | | Subject | Re: [PATCH] sched/uclamp: Fix uclamp_tg_restrict() | From | Dietmar Eggemann <> | Date | Wed, 16 Jun 2021 19:09:26 +0200 |
| |
On 11/06/2021 14:22, Qais Yousef wrote: > Now cpu.uclamp.min acts as a protection, we need to make sure that the > uclamp request of the task is within the allowed range of the cgroup, > that is it is clamp()'ed correctly by tg->uclamp[UCLAMP_MIN] and > tg->uclamp[UCLAMP_MAX]. > > As reported by Xuewen [1] we can have some corner cases where there's > inverstion between uclamp requested by task (p) and the uclamp values of
s/inverstion/inversion
[...]
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index 9e9a5be35cde..0318b00baa97 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 int 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); > #endif > > - return uc_req; > + return uc_eff; > }
I got confused by the renaming uc_req -> uc_eff.
We have:
uclamp_eff_value() (1)
uclamp_se uc_eff = uclamp_eff_get(p, clamp_id); (2)
uclamp_se uc_req = uclamp_tg_restrict(p, clamp_id) (3)
struct uclamp_se uc_eff = p->uclamp_req[clamp_id]; ....
(3) is now calling it uc_eff where (2) still uses uc_req for the return of (3). IMHO uc_*eff* was used after the system level ( uclamp_default) have been applied.
[...]
> @@ -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);
Since we now always have to update both clamp_id's, can you not update both under the same task_rq_lock() (in uclamp_update_active())?
| |