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

[Yosry Ahmed]

On Thu, Sep 28, 2023 at 6:07 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
>
> On Thu, Sep 28, 2023 at 5:58 PM Nhat Pham <nphamcs@xxxxxxxxx> wrote:
> >
> > On Thu, Sep 28, 2023 at 5:38 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> 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 :)