linux-kernel - Re: [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in memory controller

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: <CAJD7tkaeDBTHC3UM91O56yrp8oCU-UBO6i_5HJMjVBDQAw0ipQ@mail.gmail.com>
Date:   Thu, 28 Sep 2023 18:18:19 -0700
From:   Yosry Ahmed <yosryahmed@...gle.com>
To:     Nhat Pham <nphamcs@...il.com>
Cc:     akpm@...ux-foundation.org, riel@...riel.com, hannes@...xchg.org,
        mhocko@...nel.org, roman.gushchin@...ux.dev, shakeelb@...gle.com,
        muchun.song@...ux.dev, tj@...nel.org, lizefan.x@...edance.com,
        shuah@...nel.org, mike.kravetz@...cle.com, linux-mm@...ck.org,
        kernel-team@...a.com, linux-kernel@...r.kernel.org,
        cgroups@...r.kernel.org
Subject: Re: [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in
 memory controller

On Thu, Sep 28, 2023 at 6:07 PM Nhat Pham <nphamcs@...il.com> wrote:
>
> On Thu, Sep 28, 2023 at 5:58 PM Nhat Pham <nphamcs@...il.com> wrote:
> >
> > On Thu, Sep 28, 2023 at 5:38 PM Yosry Ahmed <yosryahmed@...gle.com> wrote:
> > >
> > > <snip>
> > >
> > > >
> > > > +
> > > > +/**
> > > > + * mem_cgroup_hugetlb_charge_folio - Charge a newly allocated hugetlb folio.
> > > > + * @folio: folio to charge.
> > > > + * @gfp: reclaim mode
> > > > + *
> > > > + * This function charges an allocated hugetlbf folio to the memcg of the
> > > > + * current task.
> > > > + *
> > > > + * Returns 0 on success. Otherwise, an error code is returned.
> > > > + */
> > > > +int mem_cgroup_hugetlb_charge_folio(struct folio *folio, gfp_t gfp)
> > > > +{
> > > > +       struct mem_cgroup *memcg;
> > > > +       int ret;
> > > > +
> > > > +       if (mem_cgroup_disabled() ||
> > > > +               !(cgrp_dfl_root.flags & CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING))
> > >
> > > What happens if the memory controller is mounted in a cgroup v1
> > > hierarchy? It appears to me that we *will* go through with hugetlb
> > > charging in this case?
> >
> > Ah right, cgroup v1. Does it not work with mount flag guarding?
> > What's the behavior of cgroup v1 when it comes to memory
> > recursive protection for e.g (which this mount flag is based on)?
> >
> > If it doesn't work, we'll have to add a separate knob for v1 -
> > no biggies.
>
> But to be clear, my intention is that we're not adding this
> feature to v1 (which, to my understanding, has been
> deprecated).
>
> If it's added by virtue of it sharing infrastructure with v2,
> then it's fine, but only if the mount option still works to
> guard against unintentional enablement (if not we'll
> also short-circuit v1, or add knobs if ppl really want
> it in v1 as well).
>
> If it's not added at all, then I don't have any complaints :)
>
> >
> > Other than this concern, I don't have anything against cgroup v1
> > having this feature per se - everything should still work. But let
> > I know if it can break cgroupv1 accounting otherwise :)
> >

My concern is the scenario where the memory controller is mounted in
cgroup v1, and cgroup v2 is mounted with memory_hugetlb_accounting.

In this case it seems like the current code will only check whether
memory_hugetlb_accounting was set on cgroup v2 or not, disregarding
the fact that cgroup v1 did not enable hugetlb accounting.

I obviously prefer that any features are also added to cgroup v1,
because we still didn't make it to cgroup v2, especially when the
infrastructure is shared. On the other hand, I am pretty sure the
maintainers will not like what I am saying :)