Messages in this thread | | | Date | Wed, 8 Feb 2023 14:20:12 -0500 | From | Johannes Weiner <> | Subject | Re: [PATCH 2/2] sched/psi: iterate through cgroups directly |
| |
On Wed, Feb 08, 2023 at 06:29:56PM +0100, Michal Koutný wrote: > On Thu, Feb 09, 2023 at 12:16:54AM +0800, Kairui Song <ryncsn@gmail.com> wrote: > > Signed-off-by: Kairui Song <kasong@tencent.com> > > Signed-off-by: Kairui Song <ryncsn@gmail.com> > > Typo? > > > -static inline struct psi_group *task_psi_group(struct task_struct *task) > > +static inline struct psi_group *psi_iter_first(struct task_struct *task, void **iter) > > { > > #ifdef CONFIG_CGROUPS > > - if (static_branch_likely(&psi_cgroups_enabled)) > > - return cgroup_psi(task_dfl_cgroup(task)); > > + if (static_branch_likely(&psi_cgroups_enabled)) { > > + struct cgroup *cgroup = task_dfl_cgroup(task); > > + > > + *iter = cgroup_parent(cgroup); > > This seems to skip a cgroup level -- maybe that's the observed > performance gain?
Hm, I don't think it does. It sets up *iter to point to the parent for the _next() call, but it returns task_dfl_cgroup()->psi. The next call does the same: cgroup = *iter, *iter = parent, return cgroup->psi.
It could be a bit more readable to have *iter always point to the current cgroup - but no strong preference either way from me:
psi_groups_first(task, iter) { #ifdef CONFIG_CGROUPS if (static_branch_likely(&psi_cgroups_enabled)) { struct cgroup *cgroup = task_dfl_cgroup(task);
*iter = cgroup; return cgroup_psi(cgroup); } #endif return &psi_system; }
psi_groups_next(iter) { #ifdef CONFIG_CGROUPS if (static_branch_likely(&psi_cgroups_enabled)) { struct cgroup *cgroup = *iter;
if (cgroup) { *iter = cgroup_parent(cgroup); return cgroup_psi(cgroup); } } return NULL; #endif }
| |