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

On 8/5/26 10:58, Vlastimil Babka (SUSE) wrote:
> On 8/4/26 21:51, David Hildenbrand (Arm) wrote:
>> On 7/28/26 19:18, Vlastimil Babka (SUSE) wrote:
>>>
>>> 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.
>>>
>>>
>>> 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.

I think we should really do something like the above to detect hwpoison, to then
fallback to a slow path.

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

Agreed.

> 
>> 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]/

Agreed.

-- 
Cheers,

David