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

[Nhat Pham]

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.

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