Re: [PATCH v3 2/7] mm/khugepaged: extract young page check into pte_is_referenced() helper

[David Hildenbrand (Arm)]

On 8/4/26 21:25, Nico Pache (Red Hat) wrote:
> This change deduplicates the "is this PTE/folio referenced enough to be
> considered for a collapse" condition that was repeated in both
> __collapse_huge_page_isolate() and collapse_scan_pmd(), extracting it into
> a single inline helper function.
> 
> Also move the comment and use it as the function header. While we are at
> it, updated the comment to clarify that a young pte is a recently accessed
> one.
> 
> Acked-by: Usama Arif <usama.arif@xxxxxxxxx>
> Signed-off-by: Nico Pache (Red Hat) <nico.pache@xxxxxxxxx>
> ---
>  mm/khugepaged.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> index 1e26ea97381a..9b15c1740943 100644
> --- a/mm/khugepaged.c
> +++ b/mm/khugepaged.c
> @@ -672,6 +672,23 @@ static void release_pte_pages(pte_t *pte, pte_t *_pte,
>  	}
>  }
>  
> +/*
> + * pte_is_referenced() - Check for enough referenced PTEs to justify collapsing
> + *
> + * If collapse was initiated by khugepaged, check that the page has been
> + * recently accessed (young pte) to justify collapsing the page.

That's rather imprecise.

> + *
> + * Return: true if the page has been recently accessed.
> + */

Ideally we'd find a helper that doesn't need any of this documentation :

(1) Just keep the cc->is_khugepaged out of this helper. It over-complicates the
    semantic.

(2) Find name that incorporates that we are dealing with a PTE mapping of a
    folio

> +static inline bool pte_is_referenced(struct collapse_control *cc, pte_t pteval,
> +	struct folio *folio, struct vm_area_struct *vma, unsigned long addr)
> +{
> +	return cc->is_khugepaged &&
> +	       (pte_young(pteval) || folio_test_young(folio) ||
> +		folio_test_referenced(folio) ||
> +		mmu_notifier_test_young(vma->vm_mm, addr));
> +}

static inline bool folio_pte_referenced(struct folio *folio,
		struct vm_area_struct *vma, unsigned long addr, pte_t pteval)
{
	/* The folio was referenced previously ... */
	if (folio_test_young(folio) ||  folio_test_referenced(folio))
		return true;
	/* ... or just now through the (given) PTE. */
	return pte_young(pteval) || mmu_notifier_test_young(vma->vm_mm, addr)
}

?

-- 
Cheers,

David