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

[Nico Pache]

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.

Cheers,
-- Nico

> 
>> 		case SCAN_PTE_NON_PRESENT:
>> 		case SCAN_PTE_UFFD_WP:
>> 		case SCAN_LACK_REFERENCED_PAGE:
>> -- 
>> 2.53.0
>>
>