Re: [PATCH mm-new 1/1] mm/khugepaged: abort collapse scan on non-swap entries

[Lance Yang <lance.yang@linux.dev>]

On 2025/9/29 18:29, David Hildenbrand wrote:
> On 24.09.25 13:47, Lance Yang wrote:
>>
>>
>> On 2025/9/24 18:10, David Hildenbrand wrote:
>>> On 24.09.25 12:02, Lance Yang wrote:
>>>> From: Lance Yang <lance.yang@linux.dev>
>>>>
>>>> The existing check in hpage_collapse_scan_pmd() is specific to uffd-wp
>>>> markers. Other special markers (e.g., GUARD, POISONED) would not be
>>>> caught
>>>> early, leading to failures deeper in the swap-in logic.
>>>>
>>>> hpage_collapse_scan_pmd()
>>>>    `- collapse_huge_page()
>>>>        `- __collapse_huge_page_swapin() -> fails!
>>>>
>>>> As David suggested[1], this patch skips any such non-swap entries 
>>>> early.
>>>> If a special marker is found, the scan is aborted immediately with the
>>>> SCAN_PTE_NON_PRESENT result, as Lorenzo suggested[2], avoiding wasted
>>>> work.
>>>
>>> Note that I suggested to skip all non-present entries except swap
>>> entries, which includes migration entries, hwpoisoned entries etc.
>>
>> Oops, I completely misunderstood your suggestion :(
>>
>> It should be to handle all special non-present entries (migration,
>> hwpoison, markers), not just a specific type of marker ...
>>
>> How about this version, which handles all non-swap entries as you
>> suggested?
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index 7ab2d1a42df3..27f432e7f07c 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -1284,7 +1284,23 @@ static int hpage_collapse_scan_pmd(struct
>> mm_struct *mm,
>>           for (addr = start_addr, _pte = pte; _pte < pte + HPAGE_PMD_NR;
>>                _pte++, addr += PAGE_SIZE) {
>>                   pte_t pteval = ptep_get(_pte);
>> -               if (is_swap_pte(pteval)) {
>> +               if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
>> +                       ++none_or_zero;
>> +                       if (!userfaultfd_armed(vma) &&
>> +                           (!cc->is_khugepaged ||
>> +                            none_or_zero <= khugepaged_max_ptes_none)) {
>> +                               continue;
>> +                       } else {
>> +                               result = SCAN_EXCEED_NONE_PTE;
>> +                               count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
>> +                               goto out_unmap;
>> +                       }
>> +               } else if (!pte_present(pteval)) {
>> +                       if (non_swap_entry(pte_to_swp_entry(pteval))) {
>> +                               result = SCAN_PTE_NON_PRESENT;
>> +                               goto out_unmap;
>> +                       }
>> +
>>                           ++unmapped;
>>                           if (!cc->is_khugepaged ||
>>                               unmapped <= khugepaged_max_ptes_swap) {
>> @@ -1293,7 +1309,7 @@ static int hpage_collapse_scan_pmd(struct
>> mm_struct *mm,
>>                                    * enabled swap entries.  Please see
>>                                    * comment below for pte_uffd_wp().
>>                                    */
>> -                               if (pte_swp_uffd_wp_any(pteval)) {
>> +                               if (pte_swp_uffd_wp(pteval)) {
>>                                           result = SCAN_PTE_UFFD_WP;
>>                                           goto out_unmap;
>>                                   }
>> @@ -1304,18 +1320,6 @@ static int hpage_collapse_scan_pmd(struct
>> mm_struct *mm,
>>                                   goto out_unmap;
>>                           }
>>                   }
>> -               if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
>> -                       ++none_or_zero;
>> -                       if (!userfaultfd_armed(vma) &&
>> -                           (!cc->is_khugepaged ||
>> -                            none_or_zero <= khugepaged_max_ptes_none)) {
>> -                               continue;
>> -                       } else {
>> -                               result = SCAN_EXCEED_NONE_PTE;
>> -                               count_vm_event(THP_SCAN_EXCEED_NONE_PTE);
>> -                               goto out_unmap;
>> -                       }
>> -               }
>>                   if (pte_uffd_wp(pteval)) {
> 
>  From a quick glimpse, this should work. And as raised, we might be able 
> to unify later the scanning with the almost-duplicated code when we do 
> the second scan.

Sounds good! Let's get this one merged first, and I'll send a follow-up
patch to unify the duplicated code as you suggested ;)