Re: [PATCH 2/6] mm/damon/ops-common: handle hugetlb folios in folio mkold/young rmap walkers

[SJ Park]

On Sat, 29 Aug 2026 22:14:03 -0700 Krishna Iyer <kiyer@xxxxxxxxx> wrote:

> damon_folio_mkold_one() and damon_folio_young_one() assume the folios
> they walk are mapped by normal PTEs or THP PMDs.  When the folio is a
> hugetlb folio, page_vma_mapped_walk() returns the huge PTE in pvmw.pte
> with its page table lock held, but the walkers treat it as a normal
> PTE: they read and age it with PAGE_SIZE-granularity helpers, which is
> wrong for huge PTEs (up to PUD level), and notify secondary MMUs for
> only PAGE_SIZE of the mapping.
> 
> Add hugetlb branches to both walkers.  The mkold walker reuses
> damon_hugetlb_mkold(), which the virtual address space operations set
> has been using for hugetlb aging: it clears the young bit of the huge
> PTE via set_huge_pte_at() and calls mmu_notifier_clear_young() spanning
> the whole huge page size.  The young walker gets an equivalent new
> helper, damon_hugetlb_young(), which reads the huge PTE with
> huge_ptep_get() and consults the page idle flag and
> mmu_notifier_test_young() like the existing PTE branch.
> 
> Locking mirrors what page_vma_mapped_walk() provides: the huge PTE's
> page table lock is held inside the walk, and for shared hugetlb
> mappings (the only ones subject to huge PMD sharing), rmap_walk_file()
> already holds i_mmap_rwsem, satisfying hugetlb_walk()'s locking
> requirements.
> 
> This is currently dead code: both rmap walkers are only reachable
> through damon_get_folio(), which rejects hugetlb folios since they are
> not on the LRU lists.  A following commit will let the physical address
> space monitoring primitives opt in to hugetlb folios.
> 
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Krishna Iyer <kiyer@xxxxxxxxx>
> ---
>  mm/damon/ops-common.c | 63 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 52 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index f5fe92b825bb..62004206ca31 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -193,10 +193,20 @@ static bool damon_folio_mkold_one(struct folio *folio,
>  
>  	while (page_vma_mapped_walk(&pvmw)) {
>  		addr = pvmw.address;
> -		if (pvmw.pte)
> -			damon_ptep_mkold(pvmw.pte, vma, addr);
> -		else
> +		if (pvmw.pte) {
> +			/*
> +			 * For hugetlb folios, page_vma_mapped_walk() sets
> +			 * pvmw.pte to the huge PTE with its page table lock
> +			 * held.
> +			 */

This comment looks too verbose.  Let's drop.

> +			if (folio_test_hugetlb(folio))
> +				damon_hugetlb_mkold(pvmw.pte, vma->vm_mm, vma,
> +						addr);
> +			else
> +				damon_ptep_mkold(pvmw.pte, vma, addr);
> +		} else {
>  			damon_pmdp_mkold(pvmw.pmd, vma, addr);
> +		}
>  	}
>  	return true;
>  }
> @@ -221,6 +231,24 @@ void damon_folio_mkold(struct folio *folio)
>  
>  }
>  
> +#ifdef CONFIG_HUGETLB_PAGE
> +static bool damon_hugetlb_young(pte_t *pte, struct vm_area_struct *vma,
> +		unsigned long addr, struct folio *folio)
> +{
> +	pte_t entry = huge_ptep_get(vma->vm_mm, addr, pte);
> +
> +	return (pte_present(entry) && pte_young(entry)) ||
> +		!folio_test_idle(folio) ||
> +		mmu_notifier_test_young(vma->vm_mm, addr);
> +}
> +#else
> +static bool damon_hugetlb_young(pte_t *pte, struct vm_area_struct *vma,
> +		unsigned long addr, struct folio *folio)
> +{
> +	return false;
> +}
> +#endif	/* CONFIG_HUGETLB_PAGE */
> +
>  static bool damon_folio_young_one(struct folio *folio,
>  		struct vm_area_struct *vma, unsigned long addr, void *arg)
>  {
> @@ -232,16 +260,29 @@ static bool damon_folio_young_one(struct folio *folio,
>  	while (page_vma_mapped_walk(&pvmw)) {
>  		addr = pvmw.address;
>  		if (pvmw.pte) {
> -			pte = ptep_get(pvmw.pte);
> -
>  			/*
> -			 * PFN swap PTEs, such as device-exclusive ones, that
> -			 * actually map pages are "old" from a CPU perspective.
> -			 * The MMU notifier takes care of any device aspects.
> +			 * For hugetlb folios, page_vma_mapped_walk() sets
> +			 * pvmw.pte to the huge PTE with its page table lock
> +			 * held.
>  			 */

Again, this new comment looks unnecessary.  Let's drop.

> -			*accessed = (pte_present(pte) && pte_young(pte)) ||
> -				!folio_test_idle(folio) ||
> -				mmu_notifier_test_young(vma->vm_mm, addr);
> +			if (folio_test_hugetlb(folio)) {
> +				*accessed = damon_hugetlb_young(pvmw.pte, vma,
> +						addr, folio);
> +			} else {
> +				pte = ptep_get(pvmw.pte);
> +
> +				/*
> +				 * PFN swap PTEs, such as device-exclusive
> +				 * ones, that actually map pages are "old"
> +				 * from a CPU perspective. The MMU notifier
> +				 * takes care of any device aspects.
> +				 */
> +				*accessed = (pte_present(pte) &&
> +						pte_young(pte)) ||
> +					!folio_test_idle(folio) ||
> +					mmu_notifier_test_young(vma->vm_mm,
> +							addr);
> +			}

I feel like the indentation becomes too deep.  Could we split out this into
another static function, say, damon_pte_young()?

>  		} else {
>  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>  			pmd_t pmd = pmdp_get(pvmw.pmd);
> -- 
> 2.54.0

Other than the above two simple things, this patch looks good to me.


Thanks,
SJ