Message-ID: <CACw3F53YOMWam5OsktJLtw0bhx4Btq-1vnz+ZgcZi43rH4j0wA@mail.gmail.com>
Date: Fri, 9 Jan 2026 21:48:39 -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 Mon, Jan 5, 2026 at 11:35 PM Harry Yoo <harry.yoo@...cle.com> wrote:
>
> On Mon, Jan 05, 2026 at 07:40:00PM -0800, Jiaqi Yan wrote:
> > On Tue, Dec 30, 2025 at 8:37 PM Harry Yoo <harry.yoo@...cle.com> wrote:
> > > On Tue, Dec 30, 2025 at 04:19:50PM -0800, Jiaqi Yan wrote:
> > > > 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:
> > > > > 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().
> > >
> > > Yeah, that will work.
> > >
> > > The difference is that pgalloc_tag_split(), split_page_owner(), and
> > > split_page_memcg() only support splitting the whole page into many (> 1)
> > > smaller pieces, but we're trying to create only one smaller page from the
> > > original page.
> > >
> > > > 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):
> >
> > Actually I had another idea for page metadata like page alloc tag and
> > page owner - how about we do the special handling AFTER
> > free_pages_prepare()?
>
> Sounds good to me.
>
> > Looking at free_pages_prepare, it actually breaks down compound page
> > (if it is compound, using free_tail_page_prepare), adjust page flags,
> > and deals page metadata:
> >
> > for (i = 1; i < (1 << order); i++) {
> >     if (compound)
> >         bad += free_tail_page_prepare(page, page + i);
> > ...
> > 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);
> >
> > Even the original compound page has some HWPoison pages, these
> > operations should be done in the same way when no HWPoison page exist.
>
> and we're not losing hwpoison flag after free_pages_prepare()...
> that actually sounds fine.
>
> > The compound structure will be gone after free, the alloc tag will be
> > reduced after free, the page owner will be reset after free.
>
> Right.
>
> > So instead of splitting these stuff into temporary compound pages, why
> > not getting rid of them in the first place with free_pages_prepare()?
> >
> > This is unique to the freeing path, so we don't need to mimic
> > everything __split_unmapped_folio() does.
>
> Not 100% sure how it'd work in the memdesc world,
> but for today, right.
>
> > At high level, applying the idea to __free_pages_ok (or
> > __free_frozen_pages) looks like this:
> >
> > +static bool compound_has_hwpoisoned(struct page *page, unsigned int order)
> >  {
> > +       if (order == 0 || !PageCompound(page))
> > +               return false;
> > +
> > +       return folio_test_has_hwpoisoned(page_folio(page));
> > +}
> >
> > static void __free_pages_ok(struct page *page, unsigned int order,
> >  {
> >         unsigned long pfn = page_to_pfn(page);
> >         struct zone *zone = page_zone(page);
> > +       bool should_fhh = compound_has_hwpoisoned(page, order);
> >
> > -       if (free_pages_prepare(page, order))
> > -               free_one_page(zone, page, pfn, order, fpi_flags);
> > +       if (!free_pages_prepare(page, order))
> > +               return;
> > +
> > +       if (should_fhh) {
> > +               free_has_hwpoisoned(page, order, fpi_flags);
> > +               return;
> > +       }
> > +
> > +       free_one_page(zone, page, pfn, order, fpi_flags);
> >  }
> >
> > Under this idea, free_has_hwpoisoned() doesn't need
> > prepare_compound_page_to_free() anymore. It can just directly call
> > free_one_page() when it selects the right order.
>
> Right.
>
> > The only imperfect part is compound_has_hwpoisoned(), that we need to
> > make a call to whether free_has_hwpoisoned() or not ealier than
> > free_pages_prepare().
>
> Right.
>
> > free_pages_prepare() clears PAGE_FLAGS_SECOND
> > flags on the first tail page of a compound, which clears
> > PG_has_hwpoisoned. On the other hand, we can't easily exclude
> > PG_has_hwpoisoned from PAGE_FLAGS_SECOND. Because PG_has_hwpoisoned ==
> > PG_active, free_page_is_bad() will confuse and complaint that the
> > first tail page is still active.
>
> Adding a comment like this wouldn't hurt:

ack.

>
> /*
>  * Should be called before decomposing compound page in
>  * free_pages_prepare().
>  */
> bool should_fhh = compound_has_hwpoisoned(page, order);
>
> > I have implemented a working prototype and tested just fine. I can
> > send it out as v3 after clean it up.
> >
> > > > +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);
> > >
> > > Do we need a similar thing for memcg? Probably not, since it should have
> >
> > Yes, I think we don't need to do anything for memgc, it is already
> > uncharged in __folio_put().
> >
> > > been uncharged before freeing (as long as we're not using it for kmem pages)
> >
> > You mean the folio was charged to kernel memory, right? From my
> > reading of uncharge_folio(), it covers the kmem case.
>
> Yes but I think not all kmem pages go through uncharge_folio() given
> free_pages_prepare() handles uncharging kmem pages.

I see. Then doing free_has_hwpoisoned() *after* free_pages_prepare()
also address this concern, right?

>
> > > Perhaps a comment on why we don't need to split memcg will be enough.
> > >
> > > Do we need a similar thing for page_owner? I think yes, although it won't
> > > crash or cause a warning, it will be inconsistent if we split page_owner in
> > > split_page()/__split_unmapped_folio()/etc but not in
> > > prep_compound_page_to_free().
> > >
> > > > +}
> > > >
> > > > 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.
> > >
> > > I was thinking that once free_pages_prepare() returns false, it already
> > > has done all the work to isolate HWPoison pages and freed healthy portions,
> > > so the caller doesn't have to do anything and can bail out completely.
> >
> > Does this change the meaning of free_pages_prepare()'s return value?
> > My first take is it returns if the preparation+check works are good.
> >
> > I don't know if doing the freeing work in free_pages_prepare() is a good
> > idea.
>
> As long as it's documented in a comment, why not.
>
> It doesn't make callers' code less readable. The semantic is the same;
> callers should free the page only if free_pages_prepare() returns true.
>
> They don't have to care why they should not free it.
> (either it should not be freed back to buddy because it's bad page,
>  or it had hwpoison page(s) and healthy portions are already freed)
>
> > But if it is fine, we can actually hide the ugly
> > compound_has_hwpoisoned() entirely inside free_pages_prepare():
> > - true means preparation + check work are all good but caller needs to
> > free prepared+checked pages itself
> > - false means one of the two:
> >   - some check failed and it is impossible to safely free pages for callers
> >   - this is a compound page with some HWPoison pages, and healthy
> >     pages are already freed safely
>
> Yes, something like this could be documented in free_pages_prepare().
>
> > Doesn't seem very clean with a boolean return type...
>
> The return type could be an enum type, but that wouldn't affect
> the caller's behavior (bailing out) anyway?
>
> No strong opinion from me on the type.

I just noticed free_pages_prepare() does have an external caller
compaction_free(), so I want to be cautious about adding new freeing
behavoir to it.

Maybe I can wrap compound_has_hwpoisoned(), free_pages_prepare(), and
free_has_hwpoisoned() into free_pages_prepare_has_hwpoisoned() and
make it private to page_alloc.c.

Let me put this together in V3. I also want to correct my previous
latency measurement. Both the compared group and baseline take less
time (2ms vs 0.066ms), but it is not ~2x but ~30x.

>
> --
> Cheers,
> Harry / Hyeonggon

Powered by blists - more mailing lists