Re: [PATCH v6 4/5] mm/memory-failure: skip take_page_off_buddy after dissolving HWPoison HugeTLB page

On 2026/8/18 11:30, Jiaqi Yan wrote:
> On Mon, Aug 17, 2026 at 12:23 AM Miaohe Lin <[email protected]> wrote:
>>
>> On 2026/8/17 8:29, Jiaqi Yan wrote:
>>> On Fri, Jul 17, 2026 at 12:37 AM Miaohe Lin <[email protected]> wrote:
>>>>
>>>> On 2026/7/6 2:07, Jiaqi Yan wrote:
>>>>> Now that HWPoison subpage(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_handle_poison()
>>>>> failed.
>>>>>
>>>>> Add __hugepage_handle_poison() and replace __page_handle_poison() at
>>>>> HugeTLB specific call sites. The being handled HugeTLB page either
>>>>> is free at the moment of try_memory_failure_hugetlb(), or becomes
>>>>> free at the moment of me_huge_page().
>>>>>
>>>>> Signed-off-by: Jiaqi Yan <[email protected]>
>>>>> ---
>>>>>  mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++------
>>>>>  1 file changed, 30 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>> index 3d15b4c1b694..a37b67550718 100644
>>>>> --- a/mm/memory-failure.c
>>>>> +++ b/mm/memory-failure.c
>>>>> @@ -174,6 +174,30 @@ static struct rb_root_cached pfn_space_itree = RB_ROOT_CACHED;
>>>>>  static DEFINE_MUTEX(pfn_space_lock);
>>>>>
>>>>>  /*
>>>>> + * 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);
>>>>> +
>>>>> +     /*
>>>>> +      * Can't use dissolve_free_hugetlb_folio() without a reliable
>>>>> +      * raw_hwp_list telling which subpage is HWPoison. So do not free
>>>>> +      * them to the buddy allocator. dequeue_hugetlb_folio_node_exact()
>>>>> +      * will ensure to never re-allocate this hugepage.
>>>>> +      */
>>>>> +     if (folio_test_hugetlb_raw_hwp_unreliable(folio))
>>>>> +             /* raw_hwp_list becomes unreliable when kmalloc() fails. */
>>>>> +             return -ENOMEM;
>>>>
>>>> There are some branches in __update_and_free_hugetlb_folio that will leave hugetlb
>>>> folio untouched:
>>>>
>>>> static void __update_and_free_hugetlb_folio(struct hstate *h,
>>>>                                                 struct folio *folio)
>>>> {
>>>>         bool clear_flag = folio_test_hugetlb_vmemmap_optimized(folio);
>>>>
>>>>         if (hstate_is_gigantic_no_runtime(h))
>>>>                 return;<-- 1
>>>
>>> Thanks for catching this, Miaohe.
>>>
>>> I think the most challenging part is that
>>> update_and_free_hugetlb_folio() must support deferring freeing (via
>>> schedule_work()), so adding a return value isn't that straightforward
>>> without some refactoring...
>>>
>>> If making __hugepage_handle_poison() check
>>> hstate_is_gigantic_no_runtime() == 0 (or
>>> gigantic_page_runtime_supported() == 1) isn't an absurd idea, we can
>>
>> I'm afraid this might not be a good idea. Maybe we could re-check page state after
> 
> Any specific reason? It seems to me that code that included
> "hugetlb.h" can access gigantic_page_runtime_supported().

I am concerned that with the changes of the implementation of
dissolve_free_hugetlb_folio(), gigantic_page_runtime_supported()
might not be the sole reason causing dissolve_free_hugetlb_folio()
to fail. This would require __hugepage_handle_poison() to be modified
accordingly but we might miss that in the future... But still no strong
opinion here. :)

Thanks.
.

> 
> I feel checking gigantic_page_runtime_supported() upfront is cleaner
> than checking page state after dissolve:
> 
> if (!gigantic_page_runtime_supported())
>     return -EOPNOTSUPP;
> 
> vs
> 
> /* Direct state check after dissolve */
> rc = dissolve_free_hugetlb_folio(folio);
> if (!rc && folio_test_hugetlb(folio)) {
>     /* Dissolve failed silently due to unsupported gigantic runtime freeing */
>     rc = -EOPNOTSUPP;
> }
> 
> 
>> calling dissolve_free_hugetlb_folio?
>>
>>> just do that and avoid adding return value to
>>> __update_and_free_hugetlb_folio().
>>>
>>>>
>>>>         /*
>>>>          * If we don't know which subpages are hwpoisoned, we can't free
>>>>          * the hugepage, so it's leaked intentionally.
>>>>          */
>>>>         if (folio_test_hugetlb_raw_hwp_unreliable(folio))
>>>>                 return;<-- 2
>>>
>>> __hugepage_handle_poison() already checked this, and with mf_mutex no
>>> one can set raw_hwp_unreliable.
>>
>> Agreed.
>>
>>>
>>>>
>>>>         /*
>>>>          * If folio is not vmemmap optimized (!clear_flag), then the folio
>>>>          * is no longer identified as a hugetlb page.  hugetlb_vmemmap_restore_folio
>>>>          * can only be passed hugetlb pages and will BUG otherwise.
>>>>          */
>>>>         if (clear_flag && hugetlb_vmemmap_restore_folio(h, folio)) {
>>>>                 spin_lock_irq(&hugetlb_lock);
>>>>                 /*
>>>>                  * If we cannot allocate vmemmap pages, just refuse to free the
>>>>                  * page and put the page back on the hugetlb free list and treat
>>>>                  * as a surplus page.
>>>>                  */
>>>>                 add_hugetlb_folio(h, folio, true);
>>>>                 spin_unlock_irq(&hugetlb_lock);
>>>>                 return;<-- 3
>>>
>>> __hugepage_handle_poison() should not get into this if-block because
>>> dissolve_free_hugetlb_folio() must have
>>> hugetlb_vmemmap_restore_folio()-ed successfully, so clear_flag must be
>>> false here. Otherwise dissolve_free_hugetlb_folio() already returns
>>> early without update_and_free_hugetlb_folio().
>>
>> Agreed.
>>
>> Thanks.
>> .
> .
>