Message-ID: <CACw3F51xvzo1YHW9OtUTcR2Yji1TR8_DkKyWR6AAsPaERZhVAQ@mail.gmail.com>
Date: Tue, 27 Jan 2026 21:20:44 -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 3/3] mm/memory-failure: refactor page_handle_poison()

On Wed, Jan 14, 2026 at 7:41 PM Miaohe Lin <linmiaohe@...wei.com> wrote:
>
> On 2026/1/12 8:49, Jiaqi Yan wrote:
> > Now that HWPoison page(s) within HugeTLB page will be rejected by
> > buddy allocator during dissolve_free_hugetlb_folio(), there is no
> > need to drain_all_pages() and take_page_off_buddy() anymore. In fact,
> > calling take_page_off_buddy() after dissolve_free_hugetlb_folio()
> > succeeded returns false, making caller think page_handl_poion() failed.
>
> s/page_handl_poion/page_handle_poison/
>
> >
> > On the other hand, for hardware corrupted pages in buddy allocator,
> > take_page_off_buddy() is still a must-have.
> >
> > Given hugepage and free buddy page should be treated differently,
> > refactor page_handle_poison() and __page_handle_poison():
> >
> > - __page_handle_poison() is unwind into page_handle_poison().
> >
> > - Callers of page_handle_poison() also need to explicitly tell if
> >   page is HugeTLB hugepage or free buddy page.
> >
> > - Add helper hugepage_handle_poison() for several existing HugeTLB
> >   specific callsites.
> >
> > Signed-off-by: Jiaqi Yan <jiaqiyan@...gle.com>
> > ---
> >  mm/memory-failure.c | 84 ++++++++++++++++++++++-----------------------
> >  1 file changed, 41 insertions(+), 43 deletions(-)
> >
> > diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> > index d204de6c9792a..1fdaee1e48bb8 100644
> > --- a/mm/memory-failure.c
> > +++ b/mm/memory-failure.c
> > @@ -162,54 +162,48 @@ static struct rb_root_cached pfn_space_itree = RB_ROOT_CACHED;
> >
> >  static DEFINE_MUTEX(pfn_space_lock);
> >
> > -/*
> > - * Return values:
> > - *   1:   the page is dissolved (if needed) and taken off from buddy,
> > - *   0:   the page is dissolved (if needed) and not taken off from buddy,
> > - *   < 0: failed to dissolve.
> > +/**
> > + * Handle the HugeTLB hugepage that @page belongs to. Return values:
> > + *   = 0: the hugepage is free hugepage and is dissolved.
>
> In soft offline scene, dissolve_free_hugetlb_folio would return 0 when the page becomes
> a normal page due to race.
>
> > + *   < 0: hugepage is in-use or failed to dissolve.
> >   */
> > -static int __page_handle_poison(struct page *page)
> > +static int hugepage_handle_poison(struct page *page)
> >  {
> > -     int ret;
> > +     return dissolve_free_hugetlb_folio(page_folio(page));
> > +}
> > +
> > +/**
> > + * Helper at the end of handling @page having hardware errors.
> > + * @huge: @page is part of a HugeTLB hugepage.
> > + * @free: @page is free buddy page.
> > + * @release: memory-failure module should release a pending refcount.
> > + */
> > +static bool page_handle_poison(struct page *page, bool huge, bool free,
> > +                            bool release)
> > +{
> > +     int ret = 0;
> >
> >       /*
> > -      * zone_pcp_disable() can't be used here. It will
> > -      * hold pcp_batch_high_lock and dissolve_free_hugetlb_folio() might hold
> > -      * cpu_hotplug_lock via static_key_slow_dec() when hugetlb vmemmap
> > -      * optimization is enabled. This will break current lock dependency
> > -      * chain and leads to deadlock.
> > -      * Disabling pcp before dissolving the page was a deterministic
> > -      * approach because we made sure that those pages cannot end up in any
> > -      * PCP list. Draining PCP lists expels those pages to the buddy system,
> > -      * but nothing guarantees that those pages do not get back to a PCP
> > -      * queue if we need to refill those.
> > +      * Buddy allocator will exclude the HWPoison page after hugepage
> > +      * is successfully dissolved.
> >        */
> > -     ret = dissolve_free_hugetlb_folio(page_folio(page));
> > -     if (!ret) {
> > +     if (huge)
> > +             ret = hugepage_handle_poison(page);
> > +
> > +     if (free) {
>
> Nit: huge and free won't be both true. So we could write it as:
>         if (huge) {
>                 ...
>         } else if (free) {
>
> >               drain_all_pages(page_zone(page));
> > -             ret = take_page_off_buddy(page);
> > +             ret = take_page_off_buddy(page) ? 0 : -1;
> >       }
> >
> > -     return ret;
> > -}
> > -
> > -static bool page_handle_poison(struct page *page, bool hugepage_or_freepage, bool release)
> > -{
> > -     if (hugepage_or_freepage) {
> > +     if ((huge || free) && ret < 0)
>
> Nit: ret won't be <0 if both huge and free are false. So I think we might simplify it as:
>
>         if (ret < 0)
>
> >               /*
> > -              * Doing this check for free pages is also fine since
> > -              * dissolve_free_hugetlb_folio() returns 0 for non-hugetlb folios as well.
> > +              * We could fail to take off the target page from buddy
> > +              * for example due to racy page allocation, but that's
> > +              * acceptable because soft-offlined page is not broken
> > +              * and if someone really want to use it, they should
> > +              * take it.
> >                */
> > -             if (__page_handle_poison(page) <= 0)
> > -                     /*
> > -                      * We could fail to take off the target page from buddy
> > -                      * for example due to racy page allocation, but that's
> > -                      * acceptable because soft-offlined page is not broken
> > -                      * and if someone really want to use it, they should
> > -                      * take it.
> > -                      */
> > -                     return false;
> > -     }
> > +             return false;
> >
> >       SetPageHWPoison(page);
> >       if (release)
> > @@ -1174,7 +1168,7 @@ static int me_huge_page(struct page_state *ps, struct page *p)
> >                * subpages.
> >                */
> >               folio_put(folio);
> > -             if (__page_handle_poison(p) > 0) {
> > +             if (!hugepage_handle_poison(p)) {
> >                       page_ref_inc(p);
> >                       res = MF_RECOVERED;
> >               } else {
> > @@ -2067,7 +2061,7 @@ static int try_memory_failure_hugetlb(unsigned long pfn, int flags, int *hugetlb
> >        */
> >       if (res == 0) {
> >               folio_unlock(folio);
> > -             if (__page_handle_poison(p) > 0) {
> > +             if (!hugepage_handle_poison(p)) {
> >                       page_ref_inc(p);
> >                       res = MF_RECOVERED;
> >               } else {
> > @@ -2815,7 +2809,7 @@ static int soft_offline_in_use_page(struct page *page)
> >
> >       if (ret) {
> >               pr_info("%#lx: invalidated\n", pfn);
> > -             page_handle_poison(page, false, true);
> > +             page_handle_poison(page, false, false, true);
> >               return 0;
> >       }
> >
> > @@ -2836,7 +2830,7 @@ static int soft_offline_in_use_page(struct page *page)
> >               if (!ret) {
> >                       bool release = !huge;
> >
> > -                     if (!page_handle_poison(page, huge, release))
> > +                     if (!page_handle_poison(page, huge, false, release))
>
> This might not work for soft offline. PageHWPoison is not yet set so folio_clear_hugetlb_hwpoison
> won't be called when dissolve hugetlb hugepages...

Thanks for pointing this problem (and the later problem) out, Miaohe!
You are right, and I think the reason for both problems is, soft
offline is a totally different case than memory_failure(): there is no
PG_HWPoison until the end of page_handle_poison(). So
free_has_hwpoisoned() can't help dissolve_free_hugetlb_folio() to
exclude the page that triggered soft_offline_page().

For free_has_hwpoisoned(), I should only change the call sites in the
memory_failure() path, and leave
soft_offline_page()/page_handle_poison()/__ __page_handle_poison()
alone. Looking at the current code, HWPoison hugetlb pages happens to
be handled by __page_handle_poison() in either me_huge_page() or
try_memory_failure_hugetlb(). So I think I can replace them with a new
function that doesn't do take_page_off_buddy(), something like:

 /*
+ * Only for a HugeTLB page being handled by memory_failure(). The key
+ * difference to soft_offline() is that, no HWPoison subpage will make
+ * into buddy allocator after a successful dissolve_free_hugetlb_folio(),
+ * so take_page_off_buddy() is unnecessary.
+ */
+static int __hugepage_handle_poison(struct page *page)
+{
+       struct folio *folio = page_folio(page);
+
+       VM_WARN_ON_FOLIO(!folio_test_hwpoison(folio), folio);
+
+       /*
+        * Can't use dissolve_free_hugetlb_folio() without a reliable
+        * raw_hwp_list telling which subpage is HWPoison.
+        */
+       if (folio_test_hugetlb_raw_hwp_unreliable(folio))
+               /* raw_hwp_list becomes unreliable when kmalloc() fails. */
+               return -ENOMEM;
+
+       return dissolve_free_hugetlb_folio(folio);
+}
+

On the other hand, just leave __page_handle_poison() and
page_handle_poison() as is to do take_page_off_buddy() for soft
offline case.

>
> >                               ret = -EBUSY;
> >               } else {
> >                       if (!list_empty(&pagelist))
> > @@ -2884,6 +2878,8 @@ int soft_offline_page(unsigned long pfn, int flags)
> >  {
> >       int ret;
> >       bool try_again = true;
> > +     bool huge;
> > +     bool free;
> >       struct page *page;
> >
> >       if (!pfn_valid(pfn)) {
> > @@ -2929,7 +2925,9 @@ int soft_offline_page(unsigned long pfn, int flags)
> >       if (ret > 0) {
> >               ret = soft_offline_in_use_page(page);
> >       } else if (ret == 0) {
> > -             if (!page_handle_poison(page, true, false)) {
> > +             huge = folio_test_hugetlb(page_folio(page));
>
> folio_test_hugetlb check is racy because there's no guarantee that hugetlb hugepage won't
> be dissolved before calling page_handle_poison. That will lead to problem...
>
> soft_offline_page
>   folio_test_hugetlb -- true now
>   page_handle_poison
>   /* Hugepage is dissolved somewhere. */
>     hugepage_handle_poison -- return 0 because page is normal page or free buddy page.
>     SetPageHWPoison(page);
>     page_ref_inc(page); -- refcnt is increased while page might be on buddy...
>
> > +             free = is_free_buddy_page(page);
> > +             if (!page_handle_poison(page, huge, free, false)) {
>
> We assume free is always true due to ret is 0. So we can write it as:
>         if (!page_handle_poison(page, huge, true, false)) {
>
> >                       if (try_again) {
> >                               try_again = false;
> >                               flags &= ~MF_COUNT_INCREASED;
> >
>
> Thanks.
> .
>

Powered by blists - more mailing lists