linux-kernel - Re: [PATCH 1/1] cgroup: make per-cgroup pressure stall tracking configurable

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJuCfpGx7w2E8Bq7jcq7HS41i40r4Lp+-z9m1P095B+MkAOWqg@mail.gmail.com>
Date:   Mon, 17 May 2021 13:02:36 -0700
From:   Suren Baghdasaryan <surenb@...gle.com>
To:     Johannes Weiner <hannes@...xchg.org>
Cc:     Peter Zijlstra <peterz@...radead.org>, Tejun Heo <tj@...nel.org>,
        lizefan.x@...edance.com, Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Benjamin Segall <bsegall@...gle.com>, mgorman@...e.de,
        Minchan Kim <minchan@...nel.org>,
        Jonathan Corbet <corbet@....net>, bristot@...hat.com,
        "Paul E . McKenney" <paulmck@...nel.org>, rdunlap@...radead.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>, macro@...am.me.uk,
        Viresh Kumar <viresh.kumar@...aro.org>,
        mike.kravetz@...cle.com, linux-doc@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>,
        cgroups mailinglist <cgroups@...r.kernel.org>,
        kernel-team <kernel-team@...roid.com>
Subject: Re: [PATCH 1/1] cgroup: make per-cgroup pressure stall tracking configurable

On Mon, May 17, 2021 at 11:31 AM Johannes Weiner <hannes@...xchg.org> wrote:
>
> On Sun, May 16, 2021 at 12:52:32PM -0700, Suren Baghdasaryan wrote:
> > After reworking the code to add a static key I had to expand the
> > #ifdef CONFIG_CGROUPS section, so I think a code refactoring below
> > would make sense. It localizes config-specific code and it has the
> > same exact code for CONFIG_CGROUPS=n and for
> > cgroup_psi_enabled()==false. WDYT?:
> >
> > --- a/kernel/sched/psi.c
> > +++ b/kernel/sched/psi.c
> > @@ -181,6 +181,7 @@ struct psi_group psi_system = {
> >  };
> >
> >  static void psi_avgs_work(struct work_struct *work);
> > +static void cgroup_iterator_init(void);
> >
> >  static void group_init(struct psi_group *group)
> >  {
> > @@ -211,6 +212,8 @@ void __init psi_init(void)
> >                  return;
> >          }
> >
> > +        cgroup_iterator_init();
> > +
> >          psi_period = jiffies_to_nsecs(PSI_FREQ);
> >          group_init(&psi_system);
> >  }
> > @@ -742,11 +745,31 @@ static void psi_group_change(struct psi_group
> > *group, int cpu,
> >                  schedule_delayed_work(&group->avgs_work, PSI_FREQ);
> >  }
> >
> > -static struct psi_group *iterate_groups(struct task_struct *task, void **iter)
> > +static inline struct psi_group *sys_group_iterator(struct task_struct *task,
> > +                                                   void **iter)
> >  {
> > +        *iter = &psi_system;
> > +        return &psi_system;
> > +}
> > +
> >  #ifdef CONFIG_CGROUPS
> > +
> > +DEFINE_STATIC_KEY_FALSE(psi_cgroups_disabled);
> > +
> > +static void cgroup_iterator_init(void)
> > +{
> > +        if (!cgroup_psi_enabled())
> > +                static_branch_enable(&psi_cgroups_disabled);
> > +}
> > +
> > +static struct psi_group *iterate_groups(struct task_struct *task, void **iter)
> > +{
> >          struct cgroup *cgroup = NULL;
> >
> > +        /* Skip to psi_system if per-cgroup accounting is disabled */
> > +        if (static_branch_unlikely(&psi_cgroups_disabled))
> > +                return *iter ? NULL : sys_group_iterator(task, iter);
> > +
> >          if (!*iter)
> >                  cgroup = task->cgroups->dfl_cgrp;
>
> That looks over-engineered. You have to check iter whether cgroups are
> enabled or not. Pulling the jump label check up doesn't save anything,
> but it ends up duplicating code.
>
> What you had in the beginning was better, it just had the system label
> in an unexpected place where it would check iter twice in a row.
>
> The (*iter == &psi_system) check inside the cgroups branch has the
> same purpose as the (*iter) check in the else branch. We could
> consolidate that by pulling it up front.
>
> If we wrap the entire cgroup iteration block into the static branch,
> IMO it becomes a bit clearer as well.
>
> How about this?
>
> static struct psi_group *iterate_groups(struct task_struct *task, void **iter)
> {
>         if (*iter == &psi_system)
>                 return NULL;
>
> #ifdef CONFIG_CGROUPS
>         if (!static_branch_likely(&psi_cgroups_disabled)) {
>                 struct cgroup *cgroup = NULL;
>
>                 if (!*iter)
>                         cgroup = task->cgroups->dfl_cgrp;
>                 else
>                         cgroup = cgroup_parent(*iter);
>
>                 if (cgroup && cgroup_parent(cgroup)) {
>                         *iter = cgroup;
>                         return cgroup_psi(cgroup);
>                 }
>         }
> #endif
>
>         *iter = &psi_system;
>         return &psi_system;
> }

This looks great to me. Will use it in the next version. Thanks!