[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241030150102.GA706616@cmpxchg.org>
Date: Wed, 30 Oct 2024 11:01:02 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Michal Hocko <mhocko@...e.com>
Cc: Joshua Hahn <joshua.hahnjy@...il.com>, nphamcs@...il.com,
shakeel.butt@...ux.dev, roman.gushchin@...ux.dev,
muchun.song@...ux.dev, tj@...nel.org, lizefan.x@...edance.com,
mkoutny@...e.com, corbet@....net, lnyng@...a.com,
akpm@...ux-foundation.org, cgroups@...r.kernel.org,
linux-mm@...ck.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org, kernel-team@...a.com
Subject: Re: [PATCH v3 1/1] memcg/hugetlb: Adding hugeTLB counters to memcg
On Wed, Oct 30, 2024 at 12:35:25PM +0100, Michal Hocko wrote:
> On Mon 28-10-24 14:05:05, Joshua Hahn wrote:
> [...]
> > Changelog
> > v3:
> > * Removed check for whether CGRP_ROOT_HUGETLB_ACCOUNTING is on, since
> > this check is already handled by lruvec_stat_mod (and doing the
> > check in hugetlb.c actually breaks the build if MEMCG is not
> > enabled.
> [...]
> > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> > index 190fa05635f4..fbb10e52d7ea 100644
> > --- a/mm/hugetlb.c
> > +++ b/mm/hugetlb.c
> > @@ -1925,6 +1925,7 @@ void free_huge_folio(struct folio *folio)
> > pages_per_huge_page(h), folio);
> > hugetlb_cgroup_uncharge_folio_rsvd(hstate_index(h),
> > pages_per_huge_page(h), folio);
> > + lruvec_stat_mod_folio(folio, NR_HUGETLB, -pages_per_huge_page(h));
> > mem_cgroup_uncharge(folio);
> > if (restore_reserve)
> > h->resv_huge_pages++;
> > @@ -3093,6 +3094,7 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
> >
> > if (!memcg_charge_ret)
> > mem_cgroup_commit_charge(folio, memcg);
> > + lruvec_stat_mod_folio(folio, NR_HUGETLB, pages_per_huge_page(h));
> > mem_cgroup_put(memcg);
> >
> > return folio;
>
> I do not see any specific checks for CGRP_ROOT_MEMORY_HUGETLB_ACCOUNTING
> in these paths. I guess you wanted to say that you rely on
> mem_cgroup_commit_charge setting memcg pointer which then __lruvec_stat_mod_folio
> relies on when updating stats.
Yes, this is what Shakeel pointed out here:
https://lore.kernel.org/lkml/il346o3nahawquum3t5rzcuuntkdpyahidpm2ctmdibj3td7pm@2aqirlm5hrdh/
> I suspect this all is done because you want a global counter to be
> updated as well, right? Changelog doesn't say anything about that
> though. Why is this needed when /proc/meminfo already describes the
> global hugetlb usage?
Sigh.
vmstats is the preferred framework for cgroup stats. It makes stat
items consistent between global and cgroup. It provides a per-node
breakdown as well which is useful. It avoids proliferating
cgroup-specific hooks in generic MM code.
It was a ton of work to integrate cgroup stats into vmstats and get
rid of all the memcg special casing everywhere. You were there for all
of it. We're not adding cgroup-specific stats unless unavoidable.
Duplication doesn't matter, either. We have plenty of overlap between
vmstat and meminfo. By all means, send a follow-up patch to have the
meminfo one sourced from global_node_page_state().
But you know all this. I'm having a hard time seeing the way you are,
and have been, engaging with this patch as good-faithed.
Powered by blists - more mailing lists