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

Wei Yang
2026-03-20T07:53:50Z

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