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
Date: Fri, 29 Sep 2023 13:42:21 -0400 [thread overview]
Message-ID: <20230929174221.GA19137@cmpxchg.org> (raw)
In-Reply-To: <CAJD7tkZ1NiMMvQhxGSGzsPqYfBpwzP6svPe17s2FTDoHY6jYWQ@mail.gmail.com>
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@cmpxchg.org> 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.
next prev parent reply other threads:[~2023-09-29 17:42 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-28 0:57 [PATCH v2 0/2] hugetlb memcg accounting Nhat Pham
2023-09-28 0:57 ` [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in memory controller Nhat Pham
2023-09-28 22:59 ` Frank van der Linden
2023-09-29 0:33 ` Nhat Pham
2023-09-29 0:38 ` Yosry Ahmed
2023-09-29 0:58 ` Nhat Pham
2023-09-29 1:07 ` Nhat Pham
2023-09-29 1:18 ` Yosry Ahmed
2023-09-29 1:25 ` Nhat Pham
2023-09-29 15:08 ` Johannes Weiner
2023-09-29 15:11 ` Yosry Ahmed
2023-09-29 17:42 ` Johannes Weiner [this message]
2023-09-29 17:48 ` Nhat Pham
2023-09-29 18:07 ` Frank van der Linden
2023-10-02 12:18 ` Michal Hocko
2023-09-29 18:17 ` [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in memory controller (fix) Nhat Pham
2023-09-29 18:19 ` Nhat Pham
2023-10-02 13:43 ` [PATCH v2 1/2] hugetlb: memcg: account hugetlb-backed memory in memory controller Michal Hocko
2023-10-02 14:50 ` Johannes Weiner
2023-10-02 15:08 ` Michal Hocko
2023-10-02 15:25 ` Johannes Weiner
2023-10-02 17:32 ` Nhat Pham
2023-10-03 9:17 ` Michal Hocko
2023-10-02 16:21 ` Johannes Weiner
2023-10-02 17:28 ` Nhat Pham
2023-09-28 0:57 ` [PATCH v2 2/2] selftests: add a selftest to verify hugetlb usage in memcg Nhat Pham
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230929174221.GA19137@cmpxchg.org \
--to=hannes@cmpxchg.org \
--cc=akpm@linux-foundation.org \
--cc=cgroups@vger.kernel.org \
--cc=kernel-team@meta.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=lizefan.x@bytedance.com \
--cc=mhocko@kernel.org \
--cc=mike.kravetz@oracle.com \
--cc=muchun.song@linux.dev \
--cc=nphamcs@gmail.com \
--cc=riel@surriel.com \
--cc=roman.gushchin@linux.dev \
--cc=shakeelb@google.com \
--cc=shuah@kernel.org \
--cc=tj@kernel.org \
--cc=yosryahmed@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.