lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20241017172234.GA71939@cmpxchg.org>
Date: Thu, 17 Oct 2024 13:22:34 -0400
From: Johannes Weiner <hannes@...xchg.org>
To: Joshua Hahn <joshua.hahnjy@...il.com>
Cc: nphamcs@...il.com, mhocko@...nel.org, roman.gushchin@...ux.dev,
	shakeel.butt@...ux.dev, muchun.song@...ux.dev,
	akpm@...ux-foundation.org, cgroups@...r.kernel.org,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org, lnyng@...a.com
Subject: Re: [PATCH 1/1] memcg/hugetlb: Adding hugeTLB counters to memory
 controller

On Thu, Oct 17, 2024 at 09:04:38AM -0700, Joshua Hahn wrote:
> HugeTLB is added as a metric in memcg_stat_item, and is updated in the
> alloc and free methods for hugeTLB, after (un)charging has already been
> committed. Changes are batched and updated / flushed like the rest of
> the memcg stats, which makes additional overhead by the infrequent
> hugetlb allocs / frees minimal.
> 
> Signed-off-by: Joshua Hahn <joshua.hahnjy@...il.com>
> ---
>  include/linux/memcontrol.h | 3 +++
>  mm/hugetlb.c               | 5 +++++
>  mm/memcontrol.c            | 6 ++++++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 34d2da05f2f1..66e925ae499a 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -39,6 +39,9 @@ enum memcg_stat_item {
>  	MEMCG_KMEM,
>  	MEMCG_ZSWAP_B,
>  	MEMCG_ZSWAPPED,
> +#ifdef CONFIG_HUGETLB_PAGE
> +	MEMCG_HUGETLB,
> +#endif
>  	MEMCG_NR_STAT,
>  };

It would be better to add a native vmstat counter for this, as there
is no strong reason to make this memcg specific. This would also make
it NUMA-node-aware.

IOW, add a new item to enum node_stat_item (plus the string in
vmstat.c and memcontrol.c).

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 190fa05635f4..ca7151096712 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1887,6 +1887,7 @@ void free_huge_folio(struct folio *folio)
>  	struct hstate *h = folio_hstate(folio);
>  	int nid = folio_nid(folio);
>  	struct hugepage_subpool *spool = hugetlb_folio_subpool(folio);
> +	struct mem_cgroup *memcg = get_mem_cgroup_from_current();
>  	bool restore_reserve;
>  	unsigned long flags;
>  
> @@ -1926,6 +1927,8 @@ void free_huge_folio(struct folio *folio)
>  	hugetlb_cgroup_uncharge_folio_rsvd(hstate_index(h),
>  					  pages_per_huge_page(h), folio);
>  	mem_cgroup_uncharge(folio);
> +	mod_memcg_state(memcg, MEMCG_HUGETLB, -pages_per_huge_page(h));
> +	mem_cgroup_put(memcg);
>  	if (restore_reserve)
>  		h->resv_huge_pages++;

This goes wrong if the folio is freed by somebody other than the
owning cgroup. For example if the task moved between cgroups after the
memory was charged.

It's better to use the folio->memcg linkage that was established by
the allocation path.

Use lruvec_stat_mod_folio(), it will handle all of this.

> @@ -3093,6 +3096,8 @@ struct folio *alloc_hugetlb_folio(struct vm_area_struct *vma,
>  
>  	if (!memcg_charge_ret)
>  		mem_cgroup_commit_charge(folio, memcg);
> +
> +	mod_memcg_state(memcg, MEMCG_HUGETLB, nr_pages);

And here as well.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ