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.
