Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
Date:   Fri, 29 Sep 2023 11:08:29 -0400
From:   Johannes Weiner <hannes@cmpxchg.org>
To:     Yosry Ahmed <yosryahmed@google.com>
Cc:     Nhat Pham <nphamcs@gmail.com>, akpm@linux-foundation.org,
        riel@surriel.com, mhocko@kernel.org, roman.gushchin@linux.dev,
        shakeelb@google.com, muchun.song@linux.dev, tj@kernel.org,
        lizefan.x@bytedance.com, shuah@kernel.org, mike.kravetz@oracle.com,
        linux-mm@kvack.org, kernel-team@meta.com,
        linux-kernel@vger.kernel.org, cgroups@vger.kernel.org
Subject: Re: [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in
 memory controller
Message-ID: <20230929150829.GA16353@cmpxchg.org>
References: <20230928005723.1709119-1-nphamcs@gmail.com>
 <20230928005723.1709119-2-nphamcs@gmail.com>
 <CAJD7tkanr99d_Y=LefQTFsykyiO5oZpPUC=suD3P-L5eS=0SXA@mail.gmail.com>
 <CAKEwX=M=8KYqvBTz9z1csrsFUpGf2tgWj-oyu96dSpRjn3ZnUQ@mail.gmail.com>
 <CAKEwX=Npb4mwZ2ibJkmD5GyqXazr7PH8UGLu+YSDY8acf152Eg@mail.gmail.com>
 <CAJD7tkaeDBTHC3UM91O56yrp8oCU-UBO6i_5HJMjVBDQAw0ipQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJD7tkaeDBTHC3UM91O56yrp8oCU-UBO6i_5HJMjVBDQAw0ipQ@mail.gmail.com>
Precedence: bulk

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.