Re: [PATCH 6.12] mm: replace free hugepage folios after migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mm: dequeue_hugetlb_folio_node_exact() skips isolated folios
  counted as free
- [Medium] mm: missing replace_free_hugepage_folios() breaks
  alloc_contig_range()
--

commit 90894e199378334e1089f01dd3d151f3b4957700
Author: yangge <yangge1116@126.com>

mm: replace free hugepage folios after migration

This backport takes only the dequeue_hugetlb_folio_node_exact() check from the
upstream commit to skip isolated free hugetlb folios when dequeuing from the
hugetlb freelist, while omitting replace_free_hugepage_folios() and its call
site in alloc_contig_range_noprof().

Link: https://lkml.kernel.org/r/1734503588-16254-1-git-send-email-yangge1116@126.com
Link: https://lkml.kernel.org/r/1736582300-11364-1-git-send-email-yangge1116@126.com

>     mm: replace free hugepage folios after migration
>     
>     [ Upstream commit 04f13d241b8b146b23038bffd907cb8278391d07 ]
>     
>     My machine has 4 NUMA nodes, each equipped with 32GB of memory.  I have
>     configured each NUMA node with 16GB of CMA and 16GB of in-use hugetlb
>     pages.  The allocation of contiguous memory via cma_alloc() can fail
>     probabilistically.
>     
>     When there are free hugetlb folios in the hugetlb pool, during the
>     migration of in-use hugetlb folios, new folios are allocated from the free
>     hugetlb pool.  After the migration is completed, the old folios are
>     released back to the free hugetlb pool instead of being returned to the
>     buddy system.  This can cause test_pages_isolated() check to fail,
>     ultimately leading to the failure of cma_alloc().
>     
>     Call trace:
>     
>     cma_alloc()
>         __alloc_contig_migrate_range() // migrate in-use hugepage
>         test_pages_isolated()
>             __test_page_isolated_in_pageblock()
>                  PageBuddy(page) // check if the page is in buddy
>     
>     To address this issue, we introduce a function named
>     replace_free_hugepage_folios().  This function will replace the hugepage
>     in the free hugepage pool with a new one and release the old one to the
>     buddy system.  After the migration of in-use hugetlb pages is completed,
>     we will invoke replace_free_hugepage_folios() to ensure that these
>     hugepages are properly released to the buddy system.  Following this step,
>     when test_pages_isolated() is executed for inspection, it will
>     successfully pass.
>     
>     Additionally, when alloc_contig_range() is used to migrate multiple in-use
>     hugetlb pages, it can result in some in-use hugetlb pages being released
>     back to the free hugetlb pool and subsequently being reallocated and used
>     again.  For example:
>     
>     [huge 0] [huge 1]
>     
>     To migrate huge 0, we obtain huge x from the pool.  After the migration is
>     completed, we return the now-freed huge 0 back to the pool.  When it's
>     time to migrate huge 1, we can simply reuse the now-freed huge 0 from the
>     pool.  As a result, when replace_free_hugepage_folios() is executed, it
>     cannot release huge 0 back to the buddy system.  To address this issue, we
>     should prevent the reuse of isolated free hugepages during the migration
>     process.
>     
>     Link: https://lkml.kernel.org/r/1734503588-16254-1-git-send-email-yangge1116@126.com
>     Link: https://lkml.kernel.org/r/1736582300-11364-1-git-send-email-yangge1116@126.com
>     Signed-off-by: yangge <yangge1116@126.com>
>     Cc: Baolin Wang <baolin.wang@linux.alibaba.com>
>     Cc: Barry Song <21cnbao@gmail.com>
>     Cc: David Hildenbrand <david@redhat.com>
>     Cc: SeongJae Park <sj@kernel.org>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     [ 6.12: Take only the dequeue_hugetlb_folio_node_exact() hunk, so memory
>       offline stops migrating huge pages back into the isolated range. ]
>     Signed-off-by: Mark Bundschuh <mkbund@amazon.com>

[Severity: Medium]
The commit subject and message state that this patch introduces
replace_free_hugepage_folios() and invokes it after migrating in-use hugetlb
pages so that cma_alloc() and test_pages_isolated() succeed, but this backport
drops replace_free_hugepage_folios() and its call in
alloc_contig_range_noprof() even though the helper
alloc_and_dissolve_hugetlb_folio() is already present in mm/hugetlb.c.

When alloc_contig_range_noprof() runs on a range containing multiple in-use
hugetlb folios while non-isolated free hugetlb folios exist in
h->hugepage_freelists, migrate_pages() -> alloc_migration_target() ->
alloc_hugetlb_folio_nodemask() dequeues a persistent free pool folio (which
does not have HPG_temporary set). After migration, free_huge_folio() returns
the old source folio to h->hugepage_freelists instead of the buddy allocator:

mm/hugetlb.c:free_huge_folio() {
    ...
	} else {
		arch_clear_hugetlb_flags(folio);
		enqueue_hugetlb_folio(h, folio);
		spin_unlock_irqrestore(&hugetlb_lock, flags);
	}
}

With the new check in dequeue_hugetlb_folio_node_exact() skipping that newly
freed isolated folio, each subsequent in-use hugetlb folio migrated out of the
range consumes another non-isolated free pool folio from outside the range and
leaves its old folio on h->hugepage_freelists inside the isolated range.

Because alloc_contig_range_noprof() never calls
replace_free_hugepage_folios(start, end) after __alloc_contig_migrate_range():

mm/page_alloc.c:alloc_contig_range_noprof() {
    ...
	ret = __alloc_contig_migrate_range(&cc, start, end, migratetype);
	if (ret && ret != -EBUSY)
		goto done;
	ret = 0;

	outer_start = find_large_buddy(start);

	/* Make sure the range is really isolated. */
	if (test_pages_isolated(outer_start, end, 0)) {
		ret = -EBUSY;
		goto done;
	}
}

doesn't test_pages_isolated() still fail with -EBUSY on those freed hugetlb
folios, while pulling additional free hugetlb pool folios into the isolated
CMA/contiguous range?

> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 68e0cc0748cac..b4c6c926a71e2 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c

[ ... ]

> @@ -1350,6 +1351,9 @@ static struct folio *dequeue_hugetlb_folio_node_exact(struct hstate *h,
>  		if (folio_test_hwpoison(folio))
>  			continue;
>  
> +		if (is_migrate_isolate_page(&folio->page))
> +			continue;

[Severity: High]
Can unconditionally skipping isolated folios in
dequeue_hugetlb_folio_node_exact() while they remain counted in
h->free_huge_pages break hugetlb reservation accounting during memory
offlining and page faults?

Specifically, available_huge_pages(h) computes:

    h->free_huge_pages - h->resv_huge_pages

which still counts MIGRATE_ISOLATE folios on h->hugepage_freelists[nid] as
available.

During offline_pages(), start_isolate_page_range() marks the range
MIGRATE_ISOLATE and do_migrate_range() runs before
dissolve_free_hugetlb_folios(). If there are unreserved free hugetlb folios in
the isolated range (either initially free or placed on h->hugepage_freelists
when an earlier in-use hugetlb folio in the range was migrated into an
unreserved free pool folio) and non-isolated free folios outside the range
backing h->resv_huge_pages, available_huge_pages(h) is greater than 0. When
do_migrate_range() migrates another in-use hugetlb folio in the range:

mm/hugetlb.c:alloc_hugetlb_folio_nodemask() {
    ...
	spin_lock_irq(&hugetlb_lock);
	if (available_huge_pages(h)) {
		struct folio *folio;

		folio = dequeue_hugetlb_folio_nodemask(h, gfp_mask,
						preferred_nid, nmask);
    ...
}

dequeue_hugetlb_folio_node_exact() skips the isolated free folios and steals
one of the non-isolated free folios backing h->resv_huge_pages, while
free_huge_folio() places the old folio onto h->hugepage_freelists inside the
isolated range. When dissolve_free_hugetlb_folios() then runs on the isolated
range, after dissolving the unreserved count, the remaining isolated free
folio hits:

mm/hugetlb.c:dissolve_free_hugetlb_folio() {
    ...
		if (!available_huge_pages(h))
			goto out;
    ...
}

returning -EBUSY and failing offline_pages() after persistent hugepages have
already been dissolved.

Similarly, if the free folios backing h->resv_huge_pages happen to lie in a
MIGRATE_ISOLATE pageblock, alloc_hugetlb_folio() ->
dequeue_hugetlb_folio_vma() -> dequeue_hugetlb_folio_node_exact() and
alloc_hugetlb_folio_reserve() will skip them and return NULL:

mm/hugetlb.c:alloc_hugetlb_folio() {
    ...
	folio = dequeue_hugetlb_folio_vma(h, vma, addr, gbl_chg);
	if (!folio) {
		spin_unlock_irq(&hugetlb_lock);\
		folio = alloc_buddy_hugetlb_folio_with_mpol(h, vma, addr);
		if (!folio)
			goto out_uncharge_cgroup;
    ...
}

Won't that cause reserved page faults to fail with -ENOSPC (SIGBUS) whenever
surplus overcommit is disabled (nr_overcommit_huge_pages == 0) or the hstate
is gigantic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105070442.3101877-1-mkbund@amazon.com?part=1