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

[Nhat Pham]

On Fri, Sep 29, 2023 at 10:42 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
>
> On Fri, Sep 29, 2023 at 08:11:54AM -0700, Yosry Ahmed wrote:
> > On Fri, Sep 29, 2023 at 8:08 AM Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> > >
> > > On Thu, Sep 28, 2023 at 06:18:19PM -0700, Yosry Ahmed wrote:
> > > > 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 :)
> > >
> > > I have a weak preference.
> > >
> > > It's definitely a little weird that the v1 controller's behavior
> > > changes based on the v2 mount flag. And that if you want it as an
> > > otherwise exclusive v1 user, you'd have to mount a dummy v2.
> > >
> > > But I also don't see a scenario where it would hurt, or where there
> > > would be an unresolvable conflict between v1 and v2 in expressing
> > > desired behavior, since the memory controller is exclusive to one.
> > >
> > > While we could eliminate this quirk with a simple
> > > !cgroup_subsys_on_dfl(memory_cgrp_subsys) inside the charge function,
> > > it would seem almost punitive to add extra code just to take something
> > > away that isn't really a problem and could be useful to some people.
> > >
> > > If Tejun doesn't object, I'd say let's just keep implied v1 behavior.
> >
> > I agree that adding extra code to take a feature away from v1 is
> > probably too much, but I also think relying on a v2 mount option is
> > weird. Would it be too much to just have a v1-specific flag as well
> > and use cgroup_subsys_on_dfl(memory_cgrp_subsys) to decide which flag
> > to read?
>
> Yeah, let's not preemptively add explicit new features to cgroup1.
>
> Since we agree the incidental support is weird, let's filter hugetlb
> charging on cgroup_subsys_on_dfl(memory_cgrp_subsys) after all. If
> somebody wants this for v1 - and it doesn't sound like Google is even
> in that category according to Frank - they should send a separate
> patch and we can go through all the reasons why switching to v2 is not
> an option for them.

My gut reaction when I became aware of this is to just eliminate it with
!cgroup_subsys_on_dfl(memory_cgrp_subsys) too :)

Yeah, let's just keep it simple + safe and disable it on cgroupv1 for now.
We can have this conversation again later in the future when someone
wants this in v1 (and yes, this conversation should include the option
of moving to v2).

Consider it yet another incentive to migrate to cgroupv2 ;)