Re: [PATCH v6 0/5] Only free healthy pages in high-order has_hwpoisoned folio

On 8/4/26 21:51, David Hildenbrand (Arm) wrote:
> On 7/28/26 19:18, Vlastimil Babka (SUSE) wrote:
>> On 7/27/26 16:20, David Hildenbrand (Arm) wrote:
>>> On 7/22/26 10:27, Vlastimil Babka (SUSE) wrote:
>>>>
>>>> Would it truly fix the issue, or rather there would still be a race window
>>>> left where we check that there's no hwpoison flag in the re-enabled check,
>>>> and only then someone sets it?
>>>
>>> Why are we checking PageHWPoison at all then in check_new_page()?
>> 
>> We check all kinds of unexpected state, when that's enable. PageHWPoison
>> could have been considered unexpected too, when the checks were made more
>> and more optional (first by Mel and then me).
>> 
>> But indeed it seems the PageHWPoison check is supposed to be load-bearing
>> (hi, Lorenzo!). It's intentionally handled before bad_page() (with a taint)
>> in check_new_page_bad(). Commits 2a7684a23e9c and f4c18e6f7b5b are also a hint.
>> 
>>> I think we created a mess.
>> 
>> Yes, the PageHWPoison handling was made part of debugging sanity check and
>> then not recognized properly as load-bearing later.
> 
> BTW, I'd assume we can check for PageHWPoison at free time fairly efficiently, as we
> touch all page flags already.
> 
> Something along the lines of:
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 520ebd2fa40b5..57f89f8684168 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1321,6 +1321,7 @@ static __always_inline bool __free_pages_prepare(struct page *page,
>         bool init = want_init_on_free();
>         bool compound = PageCompound(page);
>         struct folio *folio = page_folio(page);
> +       unsigned int hwpoisoned = 0;
>  
>         if (fpi_flags & FPI_PREPARED)
>                 return true;
> @@ -1347,21 +1348,6 @@ static __always_inline bool __free_pages_prepare(struct page *page,
>                 count_vm_events(UNEVICTABLE_PGCLEARED, nr_pages);
>         }
>  
> -       if (unlikely(PageHWPoison(page)) && !order) {
> -               /* Do not let hwpoison pages hit pcplists/buddy */
> -               reset_page_owner(page, order);
> -               page_table_check_free(page, order);
> -               pgalloc_tag_sub(page, 1 << order);
> -
> -               /*
> -                * The page is isolated and accounted for.
> -                * Mark the codetag as empty to avoid accounting error
> -                * when the page is freed by unpoison_memory().
> -                */
> -               clear_page_tag_ref(page);
> -               return false;
> -       }
> -
>         VM_BUG_ON_PAGE(compound && compound_order(page) != order, page);
>  
>         /*
> @@ -1395,8 +1381,26 @@ static __always_inline bool __free_pages_prepare(struct page *page,
>                                 }
>                         }
>                         tail_page->flags.f &= ~PAGE_FLAGS_CHECK_AT_PREP;
> +                       hwpoisoned += PageHWPoison(tail_page);
>                 }
>         }
> +       hwpoisoned += PageHWPoison(page);
> +
> +       if (unlikely(hwpoisoned)) {
> +               /* Do not let hwpoison pages hit pcplists/buddy */
> +               reset_page_owner(page, order);
> +               page_table_check_free(page, order);
> +               pgalloc_tag_sub(page, 1 << order);
> +
> +               /*
> +                * The page is isolated and accounted for.
> +                * Mark the codetag as empty to avoid accounting error
> +                * when the page is freed by unpoison_memory().
> +                */
> +               clear_page_tag_ref(page);
> +               return false;
> +       }
> +
>         if (folio_test_anon(folio)) {
>                 mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
>                 folio->mapping = NULL;
> diff --git a/tools/testing/selftests/mm/Makefile b/tools/testing/selftests/mm/Makefile
> 
> 
> Of course, we could also fallback for order>0 to do some smart splitting.

I think the freeing part is relatively fine, as tackled by this series.

> Checking on allocation efficiently is indeed a bit more tricky. For compound
> pages prep_compound_page() will already walk all pages.

Yeah and non-compound high-order pages should just go away eventually, so I
wouldn't worry about making them slower now.
But it's tricky because prep_compound_page() happens later when we don't
expect anything to fail anymore, so it would take some refactoring.

> Having hwpoison logic to just sync with the buddy when setting hwpoison flags
> would be nicer.

I suggested that (soft-offline does it). There might be some downsides, but
I can't judge how much they would manifest in practice:

https://lore.kernel.org/all/CACw3F50nVECJ%2Bk4g%[email protected]/

> Then we could (maybe) have the rule that no hwpoisoned page can enter the buddy, and no
> page can become hwpoisoned while in the buddy. Consequently, no hwpoisoned page can
> leave the buddy (except weird races while allocating, but then it's just an allocated
> page).

I'd love if that approach was feasible, yeah.

> Maybe.
>