[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZyIZ_Sq9D_v5v43l@tiehlicka>
Date: Wed, 30 Oct 2024 12:35:25 +0100
From: Michal Hocko <mhocko@...e.com>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: hannes@...xchg.org, 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 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.
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?
--
Michal Hocko
SUSE Labs
Powered by blists - more mailing lists