Re: [PATCH v4 5/7] mm/khugepaged: Refactor the PTE state checks into a helper

On Wed Aug 12, 2026 at 5:51 AM EDT, David Hildenbrand (Arm) wrote:
>> Huh, that looks odd.
>> 
>> That should just be a VM_WARN_ON_FOLIO(true, ..) or sth like that.
>> 
>> But in collapse_scan_pmd() that warning never existed? So this raises eyebrows.
>> 
>> [...]
>> 
>> I'll play with it to see if we can do better and will reply here later.
>
>
> Okay, I think below is what we should do.
>
> There is one behavioral change: we now longer trace the last folio, which is the right
> thing to do IMHO.
>
> And I think there is one fix we should pull out and evaluate first: A zeropage with a
> uffd-wp marker is not checked properly, IIUC.
>
>
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 5a06e3942e889..8a223659fce2d 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -119,6 +119,18 @@ struct collapse_control {
>  	DECLARE_BITMAP(mthp_present_ptes, MAX_PTRS_PER_PTE);
>  };
>  
> +struct collapse_anon_pte_check_ctx {
> +	struct collapse_control *cc;
> +	struct vm_area_struct *vma;
> +	unsigned int order;
> +	int none_or_zero_ptes;
> +	int nonpresent_ptes;
> +	int shared_ptes;
> +	unsigned int max_ptes_none;
> +	int max_ptes_swap;
> +	unsigned int max_ptes_shared;
> +};
> +
>  /**
>   * struct khugepaged_scan - cursor for scanning
>   * @mm_head: the head of the mm list to scan
> @@ -696,74 +708,104 @@ static void count_collapse_event(unsigned int order, enum vm_event_item vm_event
>  	count_mthp_stat(order, mthp_event);
>  }
>  
> +static enum scan_result collapse_anon_pte_check(pte_t pteval,
> +		unsigned long addr, struct collapse_anon_pte_check_ctx *ctx,
> +		struct folio **foliop)
> +{
> +	*foliop = NULL;
> +
> +	/*
> +	 * Don't collapse if any of the small PTEs are armed with uffd
> +	 * write protection. Marking the new huge pmd as write protected
> +	 * could bring userfault messages that fall outside of the
> +	 * registered range.
> +	 */
> +	if ((pte_present(pteval) && pte_uffd(pteval)) ||
> +	    (!pte_present(pteval) && pte_swp_uffd_any(pteval)))
> +		return SCAN_PTE_UFFD;
> +
> +	if (pte_none_or_zero(pteval)) {
> +		if (++ctx->none_or_zero_ptes > ctx->max_ptes_none) {
> +			count_collapse_event(ctx->order, THP_SCAN_EXCEED_NONE_PTE,
> +					     MTHP_STAT_COLLAPSE_EXCEED_NONE);
> +			return SCAN_EXCEED_NONE_PTE;
> +		}
> +		return SCAN_SUCCEED;
> +	}
> +	if (!pte_present(pteval)) {
> +		if (ctx->max_ptes_swap < 0)
> +			return SCAN_PTE_NON_PRESENT;
> +		if (++ctx->nonpresent_ptes > ctx->max_ptes_swap) {

So max_ptes_swap is actually max_ptes_nonpresent. But due to
khugepaged's max_ptes_swap config name, we just keep the variable and
related function names that way?


> +			count_collapse_event(ctx->order, THP_SCAN_EXCEED_SWAP_PTE,
> +					     MTHP_STAT_COLLAPSE_EXCEED_SWAP);
> +			return SCAN_EXCEED_SWAP_PTE;
> +		}
> +		return SCAN_SUCCEED;
> +	}
> +
> +	*foliop = vm_normal_folio(ctx->vma, addr, pteval);
> +	if (unlikely(!*foliop) || unlikely(folio_is_zone_device(*foliop)))
> +		return SCAN_PAGE_NULL;
> +
> +	/*
> +	 * If the vma has the VM_DROPPABLE flag, the collapse will
> +	 * preserve the lazyfree property without needing to skip.
> +	 */
> +	if (ctx->cc->is_khugepaged && !(ctx->vma->vm_flags & VM_DROPPABLE) &&
> +	    folio_test_lazyfree(*foliop) && !pte_dirty(pteval))
> +		return SCAN_PAGE_LAZYFREE;
> +
> +	if (!folio_test_anon(*foliop))
> +		return SCAN_PAGE_ANON;
> +
> +	if (folio_maybe_mapped_shared(*foliop)) {
> +		/*
> +		 * TODO: Support shared pages without leading to further
> +		 * mTHP collapses. Currently bringing in new pages via
> +		 * shared may cause a future higher order collapse on a
> +		 * rescan of the same range.
> +		 */
> +		if (++ctx->shared_ptes > ctx->max_ptes_shared) {
> +			count_collapse_event(ctx->order, THP_SCAN_EXCEED_SHARED_PTE,
> +					     MTHP_STAT_COLLAPSE_EXCEED_SHARED);
> +			return SCAN_EXCEED_SHARED_PTE;
> +		}
> +	}
> +
> +	return SCAN_SUCCEED;
> +}
> +
>  static enum scan_result __collapse_huge_page_isolate(struct vm_area_struct *vma,
>  		unsigned long start_addr, pte_t *pte, struct collapse_control *cc,
>  		unsigned int order, struct list_head *compound_pagelist)
>  {

<snip>

> +		result = collapse_anon_pte_check(pteval, addr, &ctx, &folio);
> +		if (result != SCAN_SUCCEED) {
> +			VM_WARN_ON_ONCE(result == SCAN_PAGE_ANON);
>  			goto out;
>  		}
> +		if (!folio)
> +			continue;

SCAN_SUCCEED + folio != NULL means to proceed with the pte, while
SCAN_SUCCEED + folio == NULL means to skip the pte.

collapse_anon_pte_check() probably needs to document this? Yes, it is
straightforward at the call sites, since without a folio the following
code cannot be executed. Or you think that is self-documented.



-- 
Best Regards,
Yan, Zi