Re: [PATCH v5 1/4] mm/page_alloc: only free healthy pages in high-order has_hwpoisoned folio

[Zi Yan]

On Mon Jun 22, 2026 at 4:35 AM EDT, Vlastimil Babka (SUSE) wrote:
> On 6/22/26 03:13, Jiaqi Yan wrote:
>> On Thu, Jun 18, 2026 at 9:04 AM Zi Yan <ziy@xxxxxxxxxx> wrote:
>>> >> when copy fails with EHWPOISON), the next list_add() is going to cause
>>> >
>>> > I think we should fix compaction_free() (as a separate patch preceding this
>>> > one) to not proceed if prepare returns false. It could in theory already
>>> > happen before this patch due to a random memory corruption of struct page
>>> > causing some of the existing checks to fail.
>>>
>>> Something like below should do the job, plus
>>> Fixes: 733aea0b3a7bb ("mm/compaction: add support for >0 order folio memory compaction.")
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index b776f35ad0200..b2104cbe63477 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -1876,10 +1876,12 @@ static void compaction_free(struct folio *dst, unsigned long data)
>>>         struct page *page = &dst->page;
>>>
>>>         if (folio_put_testzero(dst)) {
>>> -               free_pages_prepare(page, order);
>>> +               if (!free_pages_prepare(page, order))
>> 
>> ... for other cases that free_pages_prepare() fails, this is
>> absolutely necessary for correctness. Do you mind sending it out for
>> formal review (I will be happy to help review).
>
> I think it would be faster if you just added it to your series with a
> Suggested-by: or something? Unless Zi objects. The change is straightforward.

Let me send it as a hotfix separately, since Andrew prefers to have a
standalone hotfix to avoid splitting a series.

>
> I'd perhaps do just a
>
> if (folio_put_testzero(dst) && free_pages_prepare(page, order))
>
> to avoid the goto?

Will do.
>
>>> +                       goto skip;
>>>                 list_add(&dst->lru, &cc->freepages[order]);
>>>                 cc->nr_freepages += 1 << order;
>>>         }
>>> +skip:
>>>         cc->nr_migratepages += 1 << order;
>>>         /*
>>>          * someone else has referenced the page, we cannot take it back to our
>
> Maybe in the comment here add also the possibility that _prepare failed().

Sure.
>
>>>
>>> >
>>> >> trouble. Or you can rename the function to
>>> >> __free_pages_prepare_and_free_has_hwpoison()? At least, caller knows the
>>> >> potential side effect.
>>> >
>>> > Uh that's long. All callers are from PAGE ALLOCATOR mm-subsystem, it's not a
>>> > driver API so we'll know what we're doing (famous last words :)
>>>
>>> The name above is a half joke. :)
>>>
>>> BTW, I do not even trust myself sometimes. ;) Just look at the
>>> compaction_free() issue I introduced myself. But I do not want to be too
>>> pedantic here. So a comment above __free_pages_prepare() should be
>>> sufficient.
>> 
>> There is already a comment about the possible freeing case.
>> 
>>>
>>> --
>>> Best Regards,
>>> Yan, Zi
>>>




-- 
Best Regards,
Yan, Zi