Re: [PATCH v3 2/3] mm/page_alloc: only free healthy pages in high-order has_hwpoisoned folio

[Miaohe Lin <linmiaohe@huawei.com>]

On 2026/1/12 8:49, Jiaqi Yan wrote:
> At the end of dissolve_free_hugetlb_folio(), a free HugeTLB folio
> becomes non-HugeTLB, and it is released to buddy allocator
> as a high-order folio, e.g. a folio that contains 262144 pages
> if the folio was a 1G HugeTLB hugepage.
> 
> This is problematic if the HugeTLB hugepage contained HWPoison
> subpages. In that case, since buddy allocator does not check
> HWPoison for non-zero-order folio, the raw HWPoison page can
> be given out with its buddy page and be re-used by either
> kernel or userspace.
> 
> Memory failure recovery (MFR) in kernel does attempt to take
> raw HWPoison page off buddy allocator after
> dissolve_free_hugetlb_folio(). However, there is always a time
> window between dissolve_free_hugetlb_folio() frees a HWPoison
> high-order folio to buddy allocator and MFR takes HWPoison
> raw page off buddy allocator.
> 
> One obvious way to avoid this problem is to add page sanity
> checks in page allocate or free path. However, it is against
> the past efforts to reduce sanity check overhead [1,2,3].
> 
> Introduce free_has_hwpoisoned() to only free the healthy pages
> and to exclude the HWPoison ones in the high-order folio.
> The idea is to iterate through the sub-pages of the folio to
> identify contiguous ranges of healthy pages. Instead of freeing
> pages one by one, decompose healthy ranges into the largest
> possible blocks having different orders. Every block meets the
> requirements to be freed via __free_one_page().
> 
> free_has_hwpoisoned() has linear time complexity wrt the number
> of pages in the folio. While the power-of-two decomposition
> ensures that the number of calls to the buddy allocator is
> logarithmic for each contiguous healthy range, the mandatory
> linear scan of pages to identify PageHWPoison() defines the
> overall time complexity. For a 1G hugepage having several
> HWPoison pages, free_has_hwpoisoned() takes around 2ms on
> average.
> 
> Since free_has_hwpoisoned() has nontrivial overhead, it is
> wrapped inside free_pages_prepare_has_hwpoisoned() and done
> only PG_has_hwpoisoned indicates HWPoison page exists and
> after free_pages_prepare() succeeded.
> 
> [1] https://lore.kernel.org/linux-mm/1460711275-1130-15-git-send-email-mgorman@techsingularity.net
> [2] https://lore.kernel.org/linux-mm/1460711275-1130-16-git-send-email-mgorman@techsingularity.net
> [3] https://lore.kernel.org/all/20230216095131.17336-1-vbabka@suse.cz
> 
> Signed-off-by: Jiaqi Yan <jiaqiyan@google.com>

Thanks for your patch. This patch looks good to me. A few nits below.

> ---
>  mm/page_alloc.c | 157 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 154 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 822e05f1a9646..9393589118604 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -215,6 +215,9 @@ gfp_t gfp_allowed_mask __read_mostly = GFP_BOOT_MASK;
>  unsigned int pageblock_order __read_mostly;
>  #endif
>  
> +static bool free_pages_prepare_has_hwpoisoned(struct page *page,
> +					      unsigned int order,
> +					      fpi_t fpi_flags);
>  static void __free_pages_ok(struct page *page, unsigned int order,
>  			    fpi_t fpi_flags);
>  
> @@ -1568,8 +1571,10 @@ static void __free_pages_ok(struct page *page, unsigned int order,
>  	unsigned long pfn = page_to_pfn(page);
>  	struct zone *zone = page_zone(page);
>  
> -	if (free_pages_prepare(page, order))
> -		free_one_page(zone, page, pfn, order, fpi_flags);
> +	if (!free_pages_prepare_has_hwpoisoned(page, order, fpi_flags))
> +		return;
> +
> +	free_one_page(zone, page, pfn, order, fpi_flags);

It might be better to write as:

if (free_pages_prepare_has_hwpoisoned(page, order, fpi_flags))
	free_one_page(zone, page, pfn, order, fpi_flags);

just like previous one.

>  }
>  
>  void __meminit __free_pages_core(struct page *page, unsigned int order,
> @@ -2923,6 +2928,152 @@ static bool free_frozen_page_commit(struct zone *zone,
>  	return ret;
>  }
>  
> +/*
> + * Given a range of physically contiguous pages, efficiently free them
> + * block by block. Block order is chosen to meet the PFN alignment
> + * requirement in __free_one_page().
> + */
> +static void free_contiguous_pages(struct page *curr, unsigned long nr_pages,
> +				  fpi_t fpi_flags)
> +{
> +	unsigned int order;
> +	unsigned int align_order;
> +	unsigned int size_order;
> +	unsigned long remaining;
> +	unsigned long pfn = page_to_pfn(curr);
> +	const unsigned long end_pfn = pfn + nr_pages;
> +	struct zone *zone = page_zone(curr);
> +
> +	/*
> +	 * This decomposition algorithm at every iteration chooses the
> +	 * order to be the minimum of two constraints:
> +	 * - Alignment: the largest power-of-two that divides current pfn.
> +	 * - Size: the largest power-of-two that fits in the current
> +	 *   remaining number of pages.
> +	 */
> +	while (pfn < end_pfn) {
> +		remaining = end_pfn - pfn;
> +		align_order = ffs(pfn) - 1;
> +		size_order = fls_long(remaining) - 1;
> +		order = min(align_order, size_order);
> +
> +		free_one_page(zone, curr, pfn, order, fpi_flags);
> +		curr += (1UL << order);
> +		pfn += (1UL << order);
> +	}
> +
> +	VM_WARN_ON(pfn != end_pfn);
> +}
> +
> +/*
> + * Given a high-order compound page containing certain number of HWPoison
> + * pages, free only the healthy ones to buddy allocator.
> + *
> + * Pages must have passed free_pages_prepare(). Even if having HWPoison
> + * pages, breaking down compound page and updating metadata (e.g. page
> + * owner, alloc tag) can be done together during free_pages_prepare(),
> + * which simplifies the splitting here: unlike __split_unmapped_folio(),
> + * there is no need to turn split pages into a compound page or to carry
> + * metadata.
> + *
> + * It calls free_one_page O(2^order) times and cause nontrivial overhead.
> + * So only use this when the compound page really contains HWPoison.
> + *
> + * This implementation doesn't work in memdesc world.
> + */
> +static void free_has_hwpoisoned(struct page *page, unsigned int order,
> +				fpi_t fpi_flags)
> +{
> +	struct page *curr = page;
> +	struct page *next;
> +	unsigned long nr_pages;
> +	/*
> +	 * Don't assume end points to a valid page. It is only used
> +	 * here for pointer arithmetic.
> +	 */
> +	struct page *end = page + (1 << order);
> +	unsigned long total_freed = 0;
> +	unsigned long total_hwp = 0;
> +
> +	VM_WARN_ON(order == 0);
> +	VM_WARN_ON(page->flags.f & PAGE_FLAGS_CHECK_AT_PREP);
> +
> +	while (curr < end) {
> +		next = curr;
> +		nr_pages = 0;
> +
> +		while (next < end && !PageHWPoison(next)) {
> +			++next;
> +			++nr_pages;
> +		}
> +
> +		if (next != end && PageHWPoison(next)) {

A comment why clear_page_tag_ref is needed here should be helpful.

> +			clear_page_tag_ref(next);
> +			++total_hwp;
> +		}
> +
> +		free_contiguous_pages(curr, nr_pages, fpi_flags);
> +		total_freed += nr_pages;
> +		if (next == end)
> +			break;
> +
> +		curr = PageHWPoison(next) ? next + 1 : next;

IIUC, when code reaches here, we must have found a hwpoison page or next will equal to end.
So I think PageHWPoison(next) is always true and above code can be simplified as:

	curr = next + 1;

Thanks.
.