Re: [PATCH mm-unstable v3 5/5] mm/khugepaged: unify khugepaged and madv_collapse with collapse_single_pmd()

[Wei Yang <richard.weiyang@gmail.com>]

On Wed, Mar 18, 2026 at 10:54:17AM -0600, Nico Pache wrote:
>
>
>On 3/11/26 8:04 PM, Wei Yang wrote:
>> On Wed, Mar 11, 2026 at 03:13:15PM -0600, Nico Pache wrote:
>> [..]
>>> @@ -2823,46 +2855,20 @@ int madvise_collapse(struct vm_area_struct *vma, unsigned long start,
>>> 			hend = min(hend, vma->vm_end & HPAGE_PMD_MASK);
>>> 		}
>>> 		mmap_assert_locked(mm);
>>> -		if (!vma_is_anonymous(vma)) {
>>> -			struct file *file = get_file(vma->vm_file);
>>> -			pgoff_t pgoff = linear_page_index(vma, addr);
>>>
>>> -			mmap_read_unlock(mm);
>>> -			mmap_locked = false;
>>> -			*lock_dropped = true;
>>> -			result = collapse_scan_file(mm, addr, file, pgoff, cc);
>>> -
>>> -			if (result == SCAN_PAGE_DIRTY_OR_WRITEBACK && !triggered_wb &&
>>> -			    mapping_can_writeback(file->f_mapping)) {
>>> -				loff_t lstart = (loff_t)pgoff << PAGE_SHIFT;
>>> -				loff_t lend = lstart + HPAGE_PMD_SIZE - 1;
>>> +		result = collapse_single_pmd(addr, vma, &mmap_locked, cc);
>>>
>>> -				filemap_write_and_wait_range(file->f_mapping, lstart, lend);
>>> -				triggered_wb = true;
>>> -				fput(file);
>>> -				goto retry;
>>> -			}
>>> -			fput(file);
>>> -		} else {
>>> -			result = collapse_scan_pmd(mm, vma, addr, &mmap_locked, cc);
>>> -		}
>>> 		if (!mmap_locked)
>>> 			*lock_dropped = true;
>>>
>>> -handle_result:
>>> 		switch (result) {
>>> 		case SCAN_SUCCEED:
>>> 		case SCAN_PMD_MAPPED:
>>> 			++thps;
>>> 			break;
>>> -		case SCAN_PTE_MAPPED_HUGEPAGE:
>>> -			BUG_ON(mmap_locked);
>>> -			mmap_read_lock(mm);
>>> -			result = try_collapse_pte_mapped_thp(mm, addr, true);
>>> -			mmap_read_unlock(mm);
>>> -			goto handle_result;
>>> 		/* Whitelisted set of results where continuing OK */
>>> 		case SCAN_NO_PTE_TABLE:
>>> +		case SCAN_PTE_MAPPED_HUGEPAGE:
>> 
>> It looks we won't have this case after refactor?
>> 
>> Current code flow is like this:
>> 
>>   result = collapse_single_pmd()
>>       result = collapse_scan_file()
>>           result = collapse_file()
>> 
>>       if (result == SCAN_PTE_MAPPED_HUGEPAGE) {            --- (1)
>>           result = SCAN_ANY_PROCESS;
>> 	  or 
>> 	  result = try_collapse_pte_mapped_thp();
>>       }
>> 
>> Only collapse_scan_file() and collapse_file() may return
>> SCAN_PTE_MAPPED_HUGEPAGE, and then handled in (1). After this, result is set
>> to another value to indicate whether we collapse it or not.
>> 
>> So I am afraid we don't expect to see SCAN_PTE_MAPPED_HUGEPAGE here. Do I miss
>> something?
>
>No your assessment is correct, should I remove it from the list? I've been quite
>confused about requests to list all the available ENUMs, does that mean we want
>all the enums that are reachable or all the enums that are available as a
>result? Im guessing the former based on your comment.
>

If it is me, I would remove it :-) Otherwise, I will think
collapse_single_pmd() may return SCAN_PTE_MAPPED_HUGEPAGE.

But, yeah, this is really trivial.

>Cheers,
>-- Nico
>
>> 
>>> 		case SCAN_PTE_NON_PRESENT:
>>> 		case SCAN_PTE_UFFD_WP:
>>> 		case SCAN_LACK_REFERENCED_PAGE:
>>> -- 
>>> 2.53.0
>>>
>> 

-- 
Wei Yang
Help you, Help me