Message-ID: <CACw3F53p9TyAZs2pn+aHo85=aZx0+unaoLjeFtEK8DJ8A5TD4A@mail.gmail.com>
Date: Tue, 30 Dec 2025 16:19:50 -0800
From: Jiaqi Yan <jiaqiyan@...gle.com>
To: Harry Yoo <harry.yoo@...cle.com>
Cc: jackmanb@...gle.com, hannes@...xchg.org, linmiaohe@...wei.com, 
	ziy@...dia.com, willy@...radead.org, 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
Subject: Re: [PATCH v2 2/3] mm/page_alloc: only free healthy pages in
 high-order HWPoison folio

On Sun, Dec 28, 2025 at 5:15 PM Harry Yoo <harry.yoo@...cle.com> wrote:
>
> On Fri, Dec 26, 2025 at 05:50:59PM -0800, Jiaqi Yan wrote:
> > On Mon, Dec 22, 2025 at 9:14 PM Harry Yoo <harry.yoo@...cle.com> wrote:
> > >
> > > On Fri, Dec 19, 2025 at 06:33:45PM +0000, Jiaqi Yan wrote:
> > > > At the end of dissolve_free_hugetlb_folio that a free HugeTLB
> > > > folio becomes non-HugeTLB, 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_hwpoison_pages to only free the healthy
> > > > pages and excludes 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. Each block meets the requirements to be freed
> > > > to buddy allocator (__free_frozen_pages).
> > > >
> > > > free_has_hwpoison_pages has linear time complexity O(N) 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.
> > >
> > > Hi Jiaqi, thanks for the patch!
> >
> > Thanks for your review/comments!
> >
> > >
> > > Have you tried measuring the latency of free_has_hwpoison_pages() when
> > > a few pages in a 1GB folio are hwpoisoned?
> > >
> > > Just wanted to make sure we don't introduce a possible soft lockup...
> > > Or am I worrying too much?
> >
> > In my local tests, freeing a 1GB folio with 1 / 3 / 8 HWPoison pages,
> > I never run into a soft lockup. The 8-HWPoison-page case takes more
> > time than other cases, meaning that handling the additional HWPoison
> > page adds to the time cost.
> >
> > After adding some instrument code, 10 sample runs of
> > free_has_hwpoison_pages with 8 HWPoison pages:
> > - observed mean is 7.03 ms (5.97 ms when 3 HWPoison pages)
> > - observed standard deviation is 0.76 ms (0.18 ms when 3 HWPoison pages)
> >
> > In comparison, freeing a 1G folio without any HWPoison pages 10 times
> > (with same kernel config):
> > - observed mean is 3.39 ms
> > - observed standard deviation is 0.16ms
>
> Thanks for the measurement!
>
> > So it's around twice the baseline. It should be far from triggering a
> > soft lockup, and the cost seems fair for handling exceptional hardware
> > memory errors.
>
> Yeah it looks fine to me.
>
> > I can add these measurements in future revisions.
>
> That would be nice, thanks.
>
> > > > [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>
> > > > ---
> > > >  mm/page_alloc.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 101 insertions(+)
> > > >
> > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > > index 822e05f1a9646..20c8862ce594e 100644
> > > > --- a/mm/page_alloc.c
> > > > +++ b/mm/page_alloc.c
> > > > @@ -2976,8 +2976,109 @@ static void __free_frozen_pages(struct page *page, unsigned int order,
> > > >       }
> > > >  }
> > > >
> > > > +static void prepare_compound_page_to_free(struct page *new_head,
> > > > +                                       unsigned int order,
> > > > +                                       unsigned long flags)
> > > > +{
> > > > +     new_head->flags.f = flags & (~PAGE_FLAGS_CHECK_AT_FREE);
> > > > +     new_head->mapping = NULL;
> > > > +     new_head->private = 0;
> > > > +
> > > > +     clear_compound_head(new_head);
> > > > +     if (order)
> > > > +             prep_compound_page(new_head, order);
> > > > +}
> > >
> > > Not sure why it's building compound pages, just to decompose them
> > > when freeing via __free_frozen_pages()?
> >
> > prepare_compound_page_to_free() borrowed the idea from
> > __split_folio_to_order(). Conceptually the original folio is split
> > into new compound pages with different orders;
>
> I see, and per the previous discussion we don't want to split it
> to 262,144 4K pages in the future, anyway...
>
> > here this is done on
> > the fly in free_contiguous_pages() when order is decided.
> >
> > > > +/*
> > > > + * Given a range of pages physically contiguous physical, efficiently
> > > > + * free them in blocks that meet __free_frozen_pages's requirements.
> > > > + */
> > > > +static void free_contiguous_pages(struct page *curr, struct page *next,
> > > > +                               unsigned long flags)
> > > > +{
> > > > +     unsigned int order;
> > > > +     unsigned int align_order;
> > > > +     unsigned int size_order;
> > > > +     unsigned long pfn;
> > > > +     unsigned long end_pfn = page_to_pfn(next);
> > > > +     unsigned long remaining;
> > > > +
> > > > +     /*
> > > > +      * 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 (curr < next) {
> > > > +             pfn = page_to_pfn(curr);
> > > > +             remaining = end_pfn - pfn;
> > > > +
> > > > +             align_order = ffs(pfn) - 1;
> > > > +             size_order = fls_long(remaining) - 1;
> > > > +             order = min(align_order, size_order);
> > > > +
> > > > +             prepare_compound_page_to_free(curr, order, flags);
> > > > +             __free_frozen_pages(curr, order, FPI_NONE);
> > > > +             curr += (1UL << order);
> > > > +     }
> > > > +
> > > > +     VM_WARN_ON(curr != next);
> > > > +}
> > > > +
> > > > +/*
> > > > + * Given a high-order compound page containing certain number of HWPoison
> > > > + * pages, free only the healthy ones to buddy allocator.
> > > > + *
> > > > + * It calls __free_frozen_pages O(2^order) times and cause nontrivial
> > > > + * overhead. So only use this when compound page really contains HWPoison.
> > > > + *
> > > > + * This implementation doesn't work in memdesc world.
> > > > + */
> > > > +static void free_has_hwpoison_pages(struct page *page, unsigned int order)
> > > > +{
> > > > +     struct page *curr = page;
> > > > +     struct page *end = page + (1 << order);
> > > > +     struct page *next;
> > > > +     unsigned long flags = page->flags.f;
> > > > +     unsigned long nr_pages;
> > > > +     unsigned long total_freed = 0;
> > > > +     unsigned long total_hwp = 0;
> > > > +
> > > > +     VM_WARN_ON(flags & PAGE_FLAGS_CHECK_AT_FREE);
> > > > +
> > > > +     while (curr < end) {
> > > > +             next = curr;
> > > > +             nr_pages = 0;
> > > > +
> > > > +             while (next < end && !PageHWPoison(next)) {
> > > > +                     ++next;
> > > > +                     ++nr_pages;
> > > > +             }
> > > > +
> > > > +             if (PageHWPoison(next))
> > > > +                     ++total_hwp;
> > > > +
> > > > +             free_contiguous_pages(curr, next, flags);
> > >
> > > page_owner, memory profiling (anything else?) will be confused
> > > because it was allocated as a larger size, but we're freeing only
> > > some portion of it.
> >
> > I am not sure, but looking at __split_unmapped_folio, it calls
> > pgalloc_tag_split(folio, old_order, split_order) when splitting an
> > old_order-order folio into a new split_order.
> >
> > Maybe prepare_compound_page_to_free really should
> > update_page_tag_ref(), I need to take a closer look at this with
> > CONFIG_MEM_ALLOC_PROFILING (not something I usually enable).
> >
> > > Perhaps we need to run some portion of this code snippet
> > > (from free_pages_prepare()), before freeing portions of it:
> > >
> > >         page_cpupid_reset_last(page);
> > >         page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> > >         reset_page_owner(page, order);
> > >         page_table_check_free(page, order);
> > >         pgalloc_tag_sub(page, 1 << order);
> >
> > Since they come from free_pages_prepare, I believe these lines are
> > already executed via free_contiguous_pages()=> __free_frozen_pages()=>
> > free_pages_prepare(), right? Or am I missing something?
>
> But they're called with order that is smaller than the original order.
> That's could be problematic; for example, memory profiling stores metadata
> only on the first page. If you pass anything other than the first page
> to free_pages_prepare(), it will not recognize that metadata was stored
> during allocation.
>

Right, with MEM_ALLOC_PROFILING enabled, I ran into the following
WARNING when freeing all blocks except the 1st one (which contains the
original head page):

[ 2101.713669] ------------[ cut here ]------------
[ 2101.713670] alloc_tag was not set
[ 2101.713671] WARNING: ./include/linux/alloc_tag.h:164 at
__pgalloc_tag_sub+0xdf/0x160, CPU#18: hugetlb-mfr-3pa/33675
[ 2101.713693] CPU: 18 UID: 0 PID: 33675 Comm: hugetlb-mfr-3pa
Tainted: G S W O 6.19.0-smp-DEV #2 NONE
[ 2101.713698] Tainted: [S]=CPU_OUT_OF_SPEC, [W]=WARN, [O]=OOT_MODULE
[ 2101.713702] RIP: 0010:__pgalloc_tag_sub+0xdf/0x160
...
[ 2101.713723] Call Trace:
[ 2101.713725] <TASK>
[ 2101.713727] free_has_hwpoison_pages+0xbc/0x370
[ 2101.713731] free_frozen_pages+0xb3/0x100
[ 2101.713733] __folio_put+0xd5/0x100
[ 2101.713739] dissolve_free_hugetlb_folio+0x17f/0x1a0
[ 2101.713743] filemap_offline_hwpoison_folio+0x193/0x4c0
[ 2101.713747] ? __pfx_workingset_update_node+0x10/0x10
[ 2101.713751] remove_inode_hugepages+0x209/0x690
[ 2101.713757] ? on_each_cpu_cond_mask+0x1a/0x20
[ 2101.713760] ? __cond_resched+0x23/0x60
[ 2101.713768] ? n_tty_write+0x4c7/0x500
[ 2101.713773] hugetlbfs_setattr+0x127/0x170
[ 2101.713776] notify_change+0x32e/0x390
[ 2101.713781] do_ftruncate+0x12c/0x1a0
[ 2101.713786] __x64_sys_ftruncate+0x3e/0x70
[ 2101.713789] do_syscall_64+0x6f/0x890
[ 2101.713792] entry_SYSCALL_64_after_hwframe+0x76/0x7e
[ 2101.713811] </TASK>
[ 2101.713812] ---[ end trace 0000000000000000 ]---

This is because in free_pages_prepare(), pgalloc_tag_sub() found there
is no alloc tag on the compound page being freeing.

> In general, I think they're not designed to handle cases where
> the allocation order and the free order differ (unless we split
> metadata like __split_unmapped_folio() does).

I believe the proper way to fix this is to do something similar to
pgalloc_tag_split(), used by __split_unmapped_folio(). When we split a
new block from the original folio, we create a compound page from the
block (just the way prep_compound_page_to_free does) and link the
alloc tag of the original head page to the head of the new compound
page. Something like copy_alloc_tag (to be added in v3) below to demo
my idea, assuming tag=pgalloc_tag_get(original head page):

+/*
+ * Point page's alloc tag to an existing one.
+ */
+static void copy_alloc_tag(struct page *page, struct alloc_tag *tag)
+{
+       union pgtag_ref_handle handle;
+       union codetag_ref ref;
+       unsigned long pfn = page_to_pfn(page);
+
+       if (!mem_alloc_profiling_enabled())
+               return;
+
+       /* tag is NULL if HugeTLB page is allocated in boot process. */
+       if (!tag)
+               return;
+
+       if (!get_page_tag_ref(page, &ref, &handle))
+               return;
+
+       /* Avoid overriding existing alloc tag from page. */
+       if (!ref.ct || is_codetag_empty(&ref)) {
+               alloc_tag_ref_set(&ref, tag);
+               update_page_tag_ref(handle, &ref);
+       }
+       put_page_tag_ref(handle);
+}
+
+static void prep_compound_page_to_free(struct page *head, unsigned int order,
+                                      unsigned long flags, struct
alloc_tag *tag)
+{
+       head->flags.f = flags & (~PAGE_FLAGS_CHECK_AT_FREE);
+       head->mapping = NULL;
+       head->private = 0;
+
+       clear_compound_head(head);
+       if (order)
+               prep_compound_page(head, order);
+
+       copy_alloc_tag(head, tag);
+}

I tested now the WARNING from include/linux/alloc_tag.h:164 is gone
for both 2M and 1G pages. BTW we also need to copy_alloc_tag() for
HWPoison pages before pgalloc_tag_sub().

>
> > > > +             total_freed += nr_pages;
> > > > +             curr = PageHWPoison(next) ? next + 1 : next;
> > > > +     }
> > > > +
> > > > +     pr_info("Excluded %lu hwpoison pages from folio\n", total_hwp);
> > > > +     pr_info("Freed %#lx pages from folio\n", total_freed);
> > > > +}
> > > > +
> > > >  void free_frozen_pages(struct page *page, unsigned int order)
> > > >  {
> > > > +     struct folio *folio = page_folio(page);
> > > > +
> > > > +     if (order > 0 && unlikely(folio_test_has_hwpoisoned(folio))) {
> > > > +             folio_clear_has_hwpoisoned(folio);
> > > > +             free_has_hwpoison_pages(page, order);
> > > > +             return;
> > > > +     }
> > > > +
> > >
> > > It feels like it's a bit random place to do has_hwpoisoned check.
> > > Can we move this to free_pages_prepare() where we have some
> > > sanity checks (and also order-0 hwpoison page handling)?
> >
> > While free_pages_prepare() seems to be a better place to do the
> > has_hwpoisoned check, it is not a good place to do
> > free_has_hwpoison_pages().
>
> Why is it not a good place for free_has_hwpoison_pages()?
>
> Callers of free_pages_prepare() are supposed to avoid freeing it back to
> the buddy or using the page when it returns false.

What I mean is, callers of free_pages_prepare() wouldn't know from the
single false return value whether 1) they should completely bail out
or 2) they should retry with free_has_hwpoison_pages. So for now I
think it'd better that free_frozen_pages does the check and act upon
the check result. Not sure if there is a better place, or worthy to
change free_pages_prepare()'s return type?

>
> ...except compaction_free(), which I don't have much idea what it's
> doing.
>
> > > >       __free_frozen_pages(page, order, FPI_NONE);
> > > >  }
>
> --
> Cheers,
> Harry / Hyeonggon

Powered by blists - more mailing lists