| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMgjq7AVQBfbr+zqrsGfJMkD-PeciEmOQMbwR4+FLpGQcdyAkw@mail.gmail.com>
Date: Thu, 9 Feb 2023 23:32:03 +0800
From: Kairui Song <ryncsn@...il.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: Suren Baghdasaryan <surenb@...gle.com>,
Chengming Zhou <zhouchengming@...edance.com>,
Michal Koutný <mkoutny@...e.com>,
Tejun Heo <tj@...nel.org>, Ingo Molnar <mingo@...hat.com>,
Peter Zijlstra <peterz@...radead.org>, cgroups@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] sched/psi: iterate through cgroups directly
Johannes Weiner <hannes@...xchg.org> 于2023年2月9日周四 03:15写道：
>
> On Thu, Feb 09, 2023 at 12:16:54AM +0800, Kairui Song wrote:
> > From: Kairui Song <kasong@...cent.com>
> >
> > psi_group->parent has the same hierarchy as the cgroup it's in.
> > So just iterate through cgroup instead.
> >
> > By adjusting the iteration logic, save some space in psi_group
> > struct, and the performance is actually better. I see a measurable
> > performance gain using mmtests/perfpipe:
> >
> > (AVG of 100 test, ops/sec, the higher the better)
> > KVM guest on a i7-9700:
> > psi=0 root cgroup 5 levels of cgroup
> > Before: 59221 55352 47821
> > After: 60100 56036 50884
> >
> > KVM guest on a Ryzen 9 5900HX:
> > psi=0 root cgroup 5 levels of cgroup
> > Before: 144566 138919 128888
> > After: 145812 139580 133514
> >
> > Signed-off-by: Kairui Song <kasong@...cent.com>
> > Signed-off-by: Kairui Song <ryncsn@...il.com>
>
> Awesome!
>
> A few comments below:
>
> > @@ -858,15 +858,34 @@ static void psi_group_change(struct psi_group *group, int cpu,
> > schedule_delayed_work(&group->avgs_work, PSI_FREQ);
> > }
> >
> > -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)
>
> Please name these psi_groups_first() and psi_groups_next().
>
> > #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);
> > + return cgroup_psi(cgroup);
> > + }
> > #endif
> > return &psi_system;
> > }
> >
> > +static inline struct psi_group *psi_iter_next(void **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);
> > + }
> > + }
> > +#endif
> > + return NULL;
> > +}
>
> > @@ -886,6 +905,7 @@ void psi_task_change(struct task_struct *task, int clear, int set)
> > {
> > int cpu = task_cpu(task);
> > struct psi_group *group;
> > + void *iter;
> > u64 now;
> >
> > if (!task->pid)
> > @@ -895,16 +915,17 @@ void psi_task_change(struct task_struct *task, int clear, int set)
> >
> > now = cpu_clock(cpu);
> >
> > - group = task_psi_group(task);
> > + group = psi_iter_first(task, &iter);
> > do {
> > psi_group_change(group, cpu, clear, set, now, true);
> > - } while ((group = group->parent));
> > + } while ((group = psi_iter_next(&iter)));
> > }
> >
> > void psi_task_switch(struct task_struct *prev, struct task_struct *next,
> > bool sleep)
> > {
> > struct psi_group *group, *common = NULL;
> > + void *iter;
> > int cpu = task_cpu(prev);
> > u64 now = cpu_clock(cpu);
>
> Please add @iter at the end to keep line length sorting.
>
> > @@ -915,7 +936,7 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next,
> > * ancestors with @prev, those will already have @prev's
> > * TSK_ONCPU bit set, and we can stop the iteration there.
> > */
> > - group = task_psi_group(next);
> > + group = psi_iter_first(prev, &iter);
> > do {
> > if (per_cpu_ptr(group->pcpu, cpu)->state_mask &
> > PSI_ONCPU) {
> > @@ -924,7 +945,7 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next,
> > }
> >
> > psi_group_change(group, cpu, 0, TSK_ONCPU, now, true);
> > - } while ((group = group->parent));
> > + } while ((group = psi_iter_next(&iter)));
> > }
> >
> > if (prev->pid) {
> > @@ -957,12 +978,12 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next,
> >
> > psi_flags_change(prev, clear, set);
> >
> > - group = task_psi_group(prev);
> > + group = psi_iter_first(prev, &iter);
> > do {
> > if (group == common)
> > break;
> > psi_group_change(group, cpu, clear, set, now, wake_clock);
> > - } while ((group = group->parent));
> > + } while ((group = psi_iter_next(&iter)));
> >
> > /*
> > * TSK_ONCPU is handled up to the common ancestor. If there are
> > @@ -972,7 +993,7 @@ void psi_task_switch(struct task_struct *prev, struct task_struct *next,
> > */
> > if ((prev->psi_flags ^ next->psi_flags) & ~TSK_ONCPU) {
> > clear &= ~TSK_ONCPU;
> > - for (; group; group = group->parent)
> > + for (; group; group = psi_iter_next(&iter))
> > psi_group_change(group, cpu, clear, set, now, wake_clock);
> > }
> > }
> > @@ -983,6 +1004,7 @@ void psi_account_irqtime(struct task_struct *task, u32 delta)
> > {
> > int cpu = task_cpu(task);
> > struct psi_group *group;
> > + void *iter;
> > struct psi_group_cpu *groupc;
> > u64 now;
>
> Ditto. You can move @groupc in the same patch.
>
> Otherwise, this looks good to me. Please add:
>
> Acked-by: Johannes Weiner <hannes@...xchg.org>
Thanks! I'll update the patch as you suggested.
Powered by blists - more mailing lists