Message-ID: <CACw3F502Xb20NL6vZ7WFJG90B_=pSYzGtjpO+xX+ZaW_sH5UNg@mail.gmail.com>
Date: Fri, 23 Jan 2026 21:32:51 -0800
From: Jiaqi Yan <jiaqiyan@...gle.com>
To: Miaohe Lin <linmiaohe@...wei.com>
Cc: nao.horiguchi@...il.com, david@...hat.com, lorenzo.stoakes@...cle.com, 
	william.roche@...cle.com, tony.luck@...el.com, wangkefeng.wang@...wei.com, 
	jane.chu@...cle.com, akpm@...ux-foundation.org, osalvador@...e.de, 
	muchun.song@...ux.dev, rientjes@...gle.com, duenwen@...gle.com, 
	jthoughton@...gle.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org, 
	Liam.Howlett@...cle.com, vbabka@...e.cz, rppt@...nel.org, surenb@...gle.com, 
	mhocko@...e.com, jackmanb@...gle.com, hannes@...xchg.org, ziy@...dia.com, 
	harry.yoo@...cle.com, willy@...radead.org
Subject: Re: [PATCH v3 2/3] mm/page_alloc: only free healthy pages in
 high-order has_hwpoisoned folio

On Wed, Jan 14, 2026 at 7:05 PM Miaohe Lin <linmiaohe@...wei.com> wrote:
>
> 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@...gle.com>
>
> Thanks for your patch. This patch looks good to me. A few nits below.

Thanks for taking a look, Miaohe!

>
> > ---
> >  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.

Ack, but I probably won't need this change after merging
free_pages_prepare_has_hwpoisoned() into free_pages_prepare().

>
> >  }
> >
> >  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.

Ack.

>
> > +                     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;

Yeah, good catch! Will simplify.

>
> Thanks.
> .

Powered by blists - more mailing lists