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

[Nhat Pham]

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 :)
>
> >
> > >
> > > +               return 0;
> > > +
> > > +       memcg = get_mem_cgroup_from_current();
> > > +       ret = charge_memcg(folio, memcg, gfp);
> > > +       mem_cgroup_put(memcg);
> > > +
> > > +       return ret;
> > > +}
> > > +
> > >  /**
> > >   * mem_cgroup_swapin_charge_folio - Charge a newly allocated folio for swapin.
> > >   * @folio: folio to charge.
> > > --
> > > 2.34.1