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

[David Hildenbrand <david@redhat.com>]

On 14.10.25 17:12, David Hildenbrand wrote:
> On 14.10.25 17:01, Lance Yang wrote:
>>
>>
>> On 2025/10/14 22:39, Lorenzo Stoakes wrote:
>>> On Tue, Oct 14, 2025 at 10:26:20PM +0800, Lance Yang wrote:
>>>>
>>>>
>>>> On 2025/10/14 19:08, Lorenzo Stoakes wrote:
>>>>> On Wed, Oct 08, 2025 at 11:26:57AM +0800, Lance Yang wrote:
>>>>>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>>>>>> index abe54f0043c7..bec3e268dc76 100644
>>>>>> --- a/mm/khugepaged.c
>>>>>> +++ b/mm/khugepaged.c
>>>>>> @@ -1020,6 +1020,11 @@ static int __collapse_huge_page_swapin(struct mm_struct *mm,
>>>>>>      		if (!is_swap_pte(vmf.orig_pte))
>>>>>>      			continue;
>>>>>>
>>>>>> +		if (non_swap_entry(pte_to_swp_entry(vmf.orig_pte))) {
>>>>>> +			result = SCAN_PTE_NON_PRESENT;
>>>>>> +			goto out;
>>>>>> +		}
>>>>>
>>>>> OK seems in line with what we were discussing before...
>>>>
>>>> Yep. That's the idea :)
>>>>
>>>>>
>>>>>> +
>>>>>>      		vmf.pte = pte;
>>>>>>      		vmf.ptl = ptl;
>>>>>>      		ret = do_swap_page(&vmf);
>>>>>> @@ -1281,7 +1286,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))) {
>>>>>
>>>>
>>>> Thanks for pointing that out!
>>>
>>> You've deleted what I've said here and also not indicated whether you'll do what
>>> I asked :)
>>>
>>> Please be clearer...
>>
>> Oh, I didn't delete your comment at all ... It's just below ...
>>
>>>
>>>>>>> Hm but can't this be pte_protnone() at this stage (or something
>> else)? And then <-- Here!
>>>>
>>>> Yeah. The funny thing is, a protnone pte cannot actually get here, IIUC.
>>>>
>>>> ```
>>>> static inline int pte_protnone(pte_t pte)
>>>> {
>>>> 	return (pte_flags(pte) & (_PAGE_PROTNONE | _PAGE_PRESENT))
>>>> 		== _PAGE_PROTNONE;
>>>> }
>>>>
>>>> static inline int pte_present(pte_t a)
>>>> {
>>>> 	return pte_flags(a) & (_PAGE_PRESENT | _PAGE_PROTNONE);
>>>> }
>>>> ```
>>>>
>>>> On x86, pte_present() returns true for a protnone pte. And I'd assume
>>>> other archs behave similarly ...
>>>
>>> This was one example, we may make changes in the future that result in entries
>>> that are non-present but also non-swap.
>>>
>>> I don't see the point in eliminating this check based on an implicit, open-coded
>>> assumption that this can never be the case, this is just asking for trouble.
>>>
>>>>
>>>>> we're just assuming pte_to_swp_entry() is operating on a swap entry when it in
>>>>> fact might not be?
>>>>>
>>>>> Couldn't we end up with false positives here?
>>>>
>>>> Emm, I think we're good here and the code is doing the right thing.
>>>
>>> I mean sorry but just - NO - to doing swap operations based on open-coded checks
>>> that you implicitly feel must imply a swap entry.
>>>
>>> This makes the code a lot more confusing, it opens us up to accidentally
>>> breaking things in future and has little to no benefit, I don't see why we're
>>> doing it.
>>>
>>> I don't think every little 'aha X must imply Y so just eliminate Z' idea need be
>>> implemented, this feels like a sort of 'mathematical reduction of code ignoring
>>> all other factors'.
>>
>> Understood. Changing !pte_present() to is_swap_pte() will resolve all your
>> concerns, right?
>>
>> ```
>> if (pte_none(pteval) || is_zero_pfn(pte_pfn(pteval))) {
>> [...]
>> } else if (is_swap_pte(pteval)) { <-- Here
>> 	if (non_swap_entry(pte_to_swp_entry(pteval))) {
>> 		[...]
>> 	}
>> [...]}
> 
> Can we please take a step back and make sure we are not starting to do
> stuff differently than elswehere in the kernel, please?
> 

For the sake of progress, I assume the compiler will optimize out the 
additional pte_none() stuff.

I absolutely, absolutely hate is_swap_pte(). To me, it makes the code 
more confusing that talking about something that is !present but also 
!none: there is something that is not an ordinary page table mapping.

The underlying problem is how we hacked in non-swap into swap (and 
that's exactly where it gets confusing). Well, which this series is all 
about.

So, I don't care in the end here.

-- 
Cheers

David / dhildenb