mm: fix PMD level mTHP accounting bugs
Subsystems (3)
cover mm: fix PMD level mTHP accounting bugs
While running selftests I noticed the PMD level per-mTHP stats (nr_anon) remained elevated after each run. After further investigation I noticed this accounting error occurs for both the migration.private_anon_htlb_test and the HMM tests.
In the HMM case this is due to folio_add_new_anon_rmap() incrementing the mTHP stats, but never containing a corresponding decrement in free_zone_device_folio(). We solve this by making sure to decrement the counter when freeing device memory.
In the migration case, we are incrementing this counter without first checking whether this folio is a hugetlb folio, which relies on a separate accounting system. We solve this by adding the proper hugetlb check before incrementing this counter.
With these changes in place, the two tests no longer cause elevated PMD level accounting issues.
Nico Pache (2):
mm: decrement MTHP_STAT_NR_ANON in free_zone_device_folio() mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
mm/memremap.c | 1 + mm/migrate.c | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-)
Signature · 1 line
2.54.0
PATCH 1/2 mm: decrement MTHP_STAT_NR_ANON in free_zone_device_folio()
When a zone device folio is mapped as anonymous, folio_add_new_anon_rmap() increments MTHP_STAT_NR_ANON. The corresponding decrement lives in __free_pages_prepare() in page_alloc.c, but zone device folios are freed via free_zone_device_folio() which never calls __free_pages_prepare(). This causes nr_anon to remain permanently elevated after zone device folios are freed.
Add the missing mod_mthp_stat() decrement to free_zone_device_folio() so that the counter is properly balanced.
--- mm/memremap.c | 1 + 1 file changed, 1 insertion(+)
mm/memremap.c +1 -0
@@ -425,6 +425,7 @@ void free_zone_device_folio(struct folio *folio)mem_cgroup_uncharge (folio);if (folio_test_anon (folio)) { +mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,-1 );for (i =0 ; i < nr; i++)__ClearPageAnonExclusive (folio_page (folio, i)); }
Signature · 1 line
2.54.0
Re: [PATCH 1/2] mm: decrement MTHP_STAT_NR_ANON in free_zone_device_folio()
Quoted 10 lines · Nico Pache
When a zone device folio is mapped as anonymous,
folio_add_new_anon_rmap()incrementsMTHP_STAT_NR_ANON. The corresponding decrement lives in__free_pages_prepare()inpage_alloc.c, but zone device folios are freed viafree_zone_device_folio()which never calls__free_pages_prepare(). This causesnr_anonto remain permanently elevated after zone device folios are freed.Add the missing
mod_mthp_stat()decrement tofree_zone_device_folio()so that the counter is properly balanced.
Quoted 8 lines
Fixes 5d65c8d758f2 ("mm: count the number of anonymous THPs per size")Co-developed-by David Hildenbrand <[email protected]>Signed-off-by David Hildenbrand <[email protected]>Signed-off-by Nico Pache <[email protected]>--- mm/memremap.c | 1 + 1 file changed, 1 insertion(+)
LGTM.
Signature · 2 lines
Best Regards, Yan, Zi
[email protected]
AI reviewRe: [PATCH 1/2] mm: decrement MTHP_STAT_NR_ANON in free_zone_device_folio()
In reply to Nico PacheThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] free_zone_device_folio() bypasses folio_unqueue_deferred_split(), causing list corruption on the deferred split LRU, leaking MTHP_STAT_NR_ANON_PARTIALLY_MAPPED, and triggering a WARN_ON_ONCE. --
commit e9684c49325471cce57373674f8d1d1d76f029b6
Author: Nico Pache <[email protected]>
mm: decrement MTHP_STAT_NR_ANON in free_zone_device_folio()
When a zone device folio is mapped as anonymous, folio_add_new_anon_rmap() increments MTHP_STAT_NR_ANON. The corresponding decrement lives in __free_pages_prepare() in page_alloc.c, but zone device folios are freed via free_zone_device_folio() which never calls __free_pages_prepare().
mm/memremap.c void free_zone_device_folio(struct folio *folio) · line 428
Quoted 9 lines
mm/memremap.c +0
-0@@ -425,6 +425,7 @@ void free_zone_device_folio(struct folio *folio)if (WARN_ON_ONCE (!pgmap))return ;mem_cgroup_uncharge (folio);
[Severity: High]
This is a pre-existing issue, but can bypassing folio_unqueue_deferred_split() here lead to list corruption on the deferred split LRU or leak MTHP_STAT_NR_ANON_PARTIALLY_MAPPED?
ZONE_DEVICE folios bypass the normal cleanup path in __folio_put():
mm/swap.c:__folio_put() {
if (unlikely(folio_is_zone_device(folio))) {
free_zone_device_folio(folio);
return;
}
...
}If a ZONE_DEVICE folio (such as MEMORY_DEVICE_COHERENT) mapped as a large anonymous folio is partially unmapped, folio_remove_rmap_pte() will queue it on the deferred split list. When its refcount hits 0, it calls free_zone_device_folio() directly.
When mem_cgroup_uncharge(folio) is called, if memcg is disabled or uncharged, it returns early, leaving the freed folio linked in the deferred_split_lru. When the deferred split shrinker runs under memory pressure, could this iteration over freed memory cause a use-after-free crash? (If memcg is enabled, the uncharge path forcefully unqueues it but emits a WARN_ON_ONCE).
Would explicitly calling folio_unqueue_deferred_split(folio) before mem_cgroup_uncharge(folio) prevent this issue?
Quoted 6 lines
mm/memremap.c +1
-0if (folio_test_anon (folio)) { +mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,-1 );for (i =0 ; i < nr; i++)mm/memremap.c +0
-0__ClearPageAnonExclusive (folio_page (folio, i)); }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH 2/2 mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
In reply to Nico Pache__folio_migrate_mapping() increments MTHP_STAT_NR_ANON for the destination folio when folio_test_anon(folio) && folio_test_large(folio) is true. However, hugetlb folios satisfy both conditions despite having a completely separate accounting system; they use hugetlb_add_anon_rmap() which does not touch mTHP stats, and their free path also bypasses the mTHP decrement in __free_pages_prepare().
This causes MTHP_STAT_NR_ANON to be incremented on each hugetlb migration without a corresponding decrement, permanently inflating the nr_anon counter.
Add a !folio_test_hugetlb() check to both places in __folio_migrate_mapping() so that only actual mTHP folios are counted.
--- mm/migrate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
mm/migrate.c +4 -2
@@ -590,7 +590,8 @@ static int __folio_migrate_mapping(struct address_space *mapping,/* No turning back from here */ newfolio->index = folio->index; newfolio->mapping = folio->mapping; -if (folio_test_anon (folio) &&folio_test_large (folio)) +if (folio_test_anon (folio) &&folio_test_large (folio) && + !folio_test_hugetlb (folio))mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,1 );if (folio_test_swapbacked (folio))__folio_set_swapbacked (newfolio);
@@ -623,7 +624,8 @@ static int __folio_migrate_mapping(struct address_space *mapping,*/ newfolio->index = folio->index; newfolio->mapping = folio->mapping; -if (folio_test_anon (folio) &&folio_test_large (folio)) +if (folio_test_anon (folio) &&folio_test_large (folio) && + !folio_test_hugetlb (folio))mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,1 );folio_ref_add (newfolio, nr);/* add cache reference */ if (folio_test_swapbacked (folio))
Signature · 1 line
2.54.0
Re: [PATCH 2/2] mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
Quoted 14 lines · Nico Pache
__folio_migrate_mapping()incrementsMTHP_STAT_NR_ANONfor the destination folio whenfolio_test_anon(folio) && folio_test_large(folio)is true. However, hugetlb folios satisfy both conditions despite having a completely separate accounting system; they usehugetlb_add_anon_rmap()which does not touch mTHP stats, and their free path also bypasses the mTHP decrement in__free_pages_prepare().This causes
MTHP_STAT_NR_ANONto be incremented on each hugetlb migration without a corresponding decrement, permanently inflating thenr_anoncounter.Add a !folio_test_hugetlb() check to both places in
__folio_migrate_mapping()so that only actual mTHP folios are counted.
Quoted 8 lines
Fixes 5d65c8d758f2 ("mm: count the number of anonymous THPs per size")Co-developed-by David Hildenbrand <[email protected]>Signed-off-by David Hildenbrand <[email protected]>Signed-off-by Nico Pache <[email protected]>--- mm/migrate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
LGTM. Thanks.
Signature · 2 lines
Best Regards, Yan, Zi
Re: [PATCH 2/2] mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
In reply to Nico PacheQuoted 27 lines · Nico Pache
__folio_migrate_mapping()incrementsMTHP_STAT_NR_ANONfor the destination folio whenfolio_test_anon(folio) && folio_test_large(folio)is true. However, hugetlb folios satisfy both conditions despite having a completely separate accounting system; they usehugetlb_add_anon_rmap()which does not touch mTHP stats, and their free path also bypasses the mTHP decrement in__free_pages_prepare().This causes
MTHP_STAT_NR_ANONto be incremented on each hugetlb migration without a corresponding decrement, permanently inflating thenr_anoncounter.Add a !folio_test_hugetlb() check to both places in
__folio_migrate_mapping()so that only actual mTHP folios are counted.Fixes 5d65c8d758f2 ("mm: count the number of anonymous THPs per size")Co-developed-by David Hildenbrand <[email protected]>Signed-off-by David Hildenbrand <[email protected]>Signed-off-by Nico Pache <[email protected]>--- mm/migrate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)mm/migrate.c +0
-0
mm/migrate.c static int __folio_migrate_mapping(struct address_space *mapping, · line 595
Quoted 7 lines
mm/migrate.c +2
-1@@ -590,7 +590,8 @@ static int __folio_migrate_mapping(struct address_space *mapping,/* No turning back from here */ newfolio->index = folio->index; newfolio->mapping = folio->mapping; -if (folio_test_anon (folio) &&folio_test_large (folio)) +if (folio_test_anon (folio) &&folio_test_large (folio) && + !folio_test_hugetlb (folio))mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,1 );
Yes, anonymous hugetlb mappings can reach here, so this check looks reasonable to me.
mm/migrate.c static int __folio_migrate_mapping(struct address_space *mapping, · line 628
Quoted 9 lines
mm/migrate.c +2
-1if (folio_test_swapbacked (folio))__folio_set_swapbacked (newfolio);@@ -623,7 +624,8 @@ static int __folio_migrate_mapping(struct address_space *mapping,*/ newfolio->index = folio->index; newfolio->mapping = folio->mapping; -if (folio_test_anon (folio) &&folio_test_large (folio)) +if (folio_test_anon (folio) &&folio_test_large (folio) && + !folio_test_hugetlb (folio))
If the hugetlb folio has a non-NULL mapping (i.e., it's a shared mapping), we should migrate it via hugetlbfs_migrate_folio()->migrate_huge_page_move_mapping(). In other words, shared hugetlb mappings should not reach this path, so this hugetlb check can be dropped. Or am I missing something?
Quoted 3 lines
mm/migrate.c +0
-0mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,1 );folio_ref_add (newfolio, nr);/* add cache reference */ if (folio_test_swapbacked (folio))
David Hildenbrand (Arm)
maintainerRe: [PATCH 2/2] mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
Quoted 7 lines
Quoted 6 lines
newfolio->index=folio->index;newfolio->mapping=folio->mapping;
- if (folio_test_anon(folio)&&folio_test_large(folio)) + if (folio_test_anon(folio)&&folio_test_large(folio)&& + !folio_test_hugetlb(folio))
Quoted 5 lines
If the hugetlb folio has a non-NULL mapping (i.e., it's a shared mapping), we should migrate it via hugetlbfs_migrate_folio()-
Quoted 1 line
migrate_huge_page_move_mapping(). In other words, shared hugetlb mappingsshould not reach this path, so this hugetlb check can be dropped. Or am I missing something?
Good point: we should only reach here with anonymous folios that are in the swapcache. That doesn't apply to hugetlb.
Signature · 3 lines
Cheers, David
Re: [PATCH 2/2] mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
Quoted 7 lines · David Hildenbrand (Arm)
Quoted 7 lines
Quoted 6 lines
newfolio->index=folio->index;newfolio->mapping=folio->mapping;
- if (folio_test_anon(folio)&&folio_test_large(folio)) + if (folio_test_anon(folio)&&folio_test_large(folio)&& + !folio_test_hugetlb(folio))
Quoted 14 lines
Quoted 5 lines
If the hugetlb folio has a non-NULL mapping (i.e., it's a shared mapping), we should migrate it via hugetlbfs_migrate_folio()-
Quoted 1 line
migrate_huge_page_move_mapping(). In other words, shared hugetlb mappingsshould not reach this path, so this hugetlb check can be dropped. Or am I missing something?
Good point: we should only reach here with anonymous folios that are in the swapcache. That doesn't apply to hugetlb.
Signature · 3 lines
Cheers, David
I think this is right. THe second site requires folio_test_anon() with a non-NULL mapping, i. e. an anon folio in the swap cache, and hugetlb folios never enter the swap cache. Shared hugetlb goes through migrate_huge_page_move_mapping() as you say, and the only other callers of folio_migrate_mapping() never pass hugetlb folios either. So the check in the second hunk can't fire.
--Richard
Re: [PATCH 2/2] mm/migrate: exclude hugetlb folios from MTHP_STAT_NR_ANON accounting
In reply to Baolin WangQuoted 42 lines · Baolin Wang
Quoted 27 lines · Nico Pache
__folio_migrate_mapping()incrementsMTHP_STAT_NR_ANONfor the destination folio whenfolio_test_anon(folio) && folio_test_large(folio)is true. However, hugetlb folios satisfy both conditions despite having a completely separate accounting system; they usehugetlb_add_anon_rmap()which does not touch mTHP stats, and their free path also bypasses the mTHP decrement in__free_pages_prepare().This causes
MTHP_STAT_NR_ANONto be incremented on each hugetlb migration without a corresponding decrement, permanently inflating thenr_anoncounter.Add a !folio_test_hugetlb() check to both places in
__folio_migrate_mapping()so that only actual mTHP folios are counted.Fixes 5d65c8d758f2 ("mm: count the number of anonymous THPs per size")Co-developed-by David Hildenbrand <[email protected]>Signed-off-by David Hildenbrand <[email protected]>Signed-off-by Nico Pache <[email protected]>--- mm/migrate.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)mm/migrate.c +0
-0mm/migrate.c static int __folio_migrate_mapping(struct address_space *mapping, · line 595
Quoted 7 lines
mm/migrate.c +2
-1@@ -590,7 +590,8 @@ static int __folio_migrate_mapping(struct address_space *mapping,/* No turning back from here */ newfolio->index = folio->index; newfolio->mapping = folio->mapping; -if (folio_test_anon (folio) &&folio_test_large (folio)) +if (folio_test_anon (folio) &&folio_test_large (folio) && + !folio_test_hugetlb (folio))mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,1 );Yes, anonymous hugetlb mappings can reach here, so this check looks reasonable to me.
Quoted 15 lines
mm/migrate.c static int __folio_migrate_mapping(struct address_space *mapping, · line 628
Quoted 9 lines
mm/migrate.c +2
-1if (folio_test_swapbacked (folio))__folio_set_swapbacked (newfolio);@@ -623,7 +624,8 @@ static int __folio_migrate_mapping(struct address_space *mapping,*/ newfolio->index = folio->index; newfolio->mapping = folio->mapping; -if (folio_test_anon (folio) &&folio_test_large (folio)) +if (folio_test_anon (folio) &&folio_test_large (folio) && + !folio_test_hugetlb (folio))If the hugetlb folio has a non-NULL mapping (i.e., it's a shared mapping), we should migrate it via hugetlbfs_migrate_folio()->migrate_huge_page_move_mapping(). In other words, shared hugetlb mappings should not reach this path, so this hugetlb check can be dropped. Or am I missing something?
I double checked (and others seem to have confirmed), you are correct; these hugetlb pages shouldn't be reachable here.
Ill drop this hunk and retest :)
Thank you,
-- Nico
Quoted 5 lines
Quoted 4 lines
mm/migrate.c +0
-0mod_mthp_stat (folio_order (folio),MTHP_STAT_NR_ANON ,1 );folio_ref_add (newfolio, nr);/* add cache reference */ if (folio_test_swapbacked (folio))