On Thu, Sep 28, 2023 at 18:18 Yosry Ahmed <yosryahmed@google.com> wrote:

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


Ohh I see. Lemme do some testing to double
check :)



>
> 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 :)


I can at least try to not break v1 for a start :)
Thanks for pointing it out tho!

>