Re: [PATCH 1/6] mm/damon: move damon_hugetlb_mkold() from vaddr to ops-common

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Non-atomic update of HugeTLB PTE in damon_hugetlb_mkold() overwrites hardware-set dirty bits and triggers unintended BBM sequences.
--

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

    mm/damon: move damon_hugetlb_mkold() from vaddr to ops-common
    
    damon_hugetlb_mkold() clears the accessed bit of a hugetlb-mapping huge
    PTE and propagates the aging to secondary MMUs via
    mmu_notifier_clear_young(), spanning the whole huge page size.  It
    currently lives in vaddr.c, and is thus usable only by the virtual
    address space monitoring operations set.

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index fbda70d8ea4d0..f5fe92b825bbc 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c

[ ... ]

> @@ -98,6 +99,35 @@ void damon_pmdp_mkold(pmd_t *pmd, struct vm_area_struct *vma, unsigned long addr
>  #endif /* CONFIG_TRANSPARENT_HUGEPAGE */
>  }
>  
> +#ifdef CONFIG_HUGETLB_PAGE
> +void damon_hugetlb_mkold(pte_t *pte, struct mm_struct *mm,
> +		struct vm_area_struct *vma, unsigned long addr)
> +{
> +	bool referenced = false;
> +	pte_t entry = huge_ptep_get(mm, addr, pte);
> +	struct folio *folio = pfn_folio(pte_pfn(entry));
> +	unsigned long psize = huge_page_size(hstate_vma(vma));
> +
> +	folio_get(folio);
> +
> +	if (pte_young(entry)) {
> +		referenced = true;
> +		entry = pte_mkold(entry);
> +		set_huge_pte_at(mm, addr, pte, entry, psize);

[Severity: Critical]
This is a pre-existing issue, but does this non-atomic update of the hugetlb
PTE overwrite hardware-set dirty bits?

The hardware MMU can set the dirty bit (on x86, or arm64 with hardware managed
dirty bits) between the huge_ptep_get() read and this set_huge_pte_at()
write. This non-atomic read-modify-write drops the hardware dirty bit, leading
to data loss upon reclaim or sync.

Additionally, on arm64, using set_huge_pte_at() on an already valid contiguous
PTE triggers an unintended break-before-make sequence with a broadcast TLB
flush via clear_flush(), which causes severe performance regressions.

> +	}
> +
> +	if (mmu_notifier_clear_young(mm, addr,
> +				     addr + huge_page_size(hstate_vma(vma))))
> +		referenced = true;
> +
> +	if (referenced)
> +		folio_set_young(folio);
> +
> +	folio_set_idle(folio);
> +	folio_put(folio);
> +}
> +#endif	/* CONFIG_HUGETLB_PAGE */

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