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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] mmu_notifier_test_young() evaluates only a single base page, masking accesses within shattered huge pages.

Pre-existing issues:
- [High] Non-atomic read-modify-write of huge PTE in damon_hugetlb_mkold() silently drops hardware-managed dirty bits.
--

commit 69bb3f48e79ddddcf3eaf4001d7704e62df0da69
Author: Krishna Iyer <kiyer@crusoe.ai>

mm/damon/ops-common: handle hugetlb folios in folio mkold/young rmap walkers

This commit introduces hugetlb branches to DAMON's rmap walkers by reusing
damon_hugetlb_mkold() for clearing the young bit and introducing
damon_hugetlb_young() for checking it. It ensures huge PTEs are read and
aged correctly rather than using base page granularity helpers.

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index f5fe92b825bbc..62004206ca310 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.
> +			 */
> +			if (folio_test_hugetlb(folio))
> +				damon_hugetlb_mkold(pvmw.pte, vma->vm_mm, vma,
> +						addr);

[Severity: High]
This is a pre-existing issue, but does damon_hugetlb_mkold() perform a
non-atomic read-modify-write that could silently drop hardware-managed
dirty bits?

Looking at damon_hugetlb_mkold(), the code reads the PTE, clears the young
bit, and writes it back:

	pte_t entry = huge_ptep_get(mm, addr, pte);
...
	if (pte_young(entry)) {
		referenced = true;
		entry = pte_mkold(entry);
		set_huge_pte_at(mm, addr, pte, entry, psize);
	}

If a hardware write access occurs and sets the hardware A/D bit after
huge_ptep_get() but before set_huge_pte_at(), won't the dirty bit be
overwritten with the stale, clean value?

This could potentially lead to data loss if hugetlb pages are reclaimed
or migrated without writeback. Hooking this up for DAMON folio monitoring
means this path can be triggered frequently in the background.

> +			else
> +				damon_ptep_mkold(pvmw.pte, vma, addr);
> +		} else {
>  			damon_pmdp_mkold(pvmw.pmd, vma, addr);
> +		}
>  	}

[ ... ]

> +#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);

[Severity: Medium]
Will mmu_notifier_test_young() miss accesses within shattered huge pages here?

Since the kernel's mmu_notifier_test_young() API does not take a size
parameter, passing only the base address of the huge page means it will
only evaluate the first base page.

For secondary MMUs like KVM EPT that map huge pages as multiple smaller
pages, wouldn't accesses to other subpages within the huge page be completely
missed? This could cause DAMON to classify the huge page as colder than it
actually is, which might lead to inappropriate reclaim or migration decisions.

> +}
> +#else

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830051407.50008-1-kiyer@crusoe.ai?part=2