Re: [PATCH] mm/damon: use a page-aligned sampling address

[SJ Park <sj@kernel.org>]

On Fri, 28 Aug 2026 18:04:55 -0700 Nathan Gao <zcgao@amazon.com> wrote:

> Hi SJ,
> 
> Thanks for your review!
> 
> On Thu, 27 Aug 2026 17:22:11 -0700 SJ Park <sj@kernel.org> wrote:
> 
> > Hello Nathan,
> > 
> > On Thu, 27 Aug 2026 12:38:21 -0700 Nathan Gao <zcgao@amazon.com> wrote:
> > 
> > > __damon_va_prepare_access_check() picks a random byte address within the
> > > region and stores it in r->sampling_addr. There are two users of
> > > r->sampling_addr in vaddr.c that pass it into a page table walk, and
> > > both use it as the address of a page.
> > > 
> > >   damon_va_mkold(mm, r->sampling_addr)
> > >     damon_va_walk_page_range(mm, addr, addr + 1)
> > >       damon_mkold_pmd_entry()
> > >         damon_ptep_mkold(pte, vma, addr)
> > >           ptep_test_and_clear_young(vma, addr, pte)
> > >           mmu_notifier_clear_young(mm, addr, addr + PAGE_SIZE)
> > > 
> > >   damon_va_young(mm, r->sampling_addr, &folio_sz)
> > >     damon_va_walk_page_range(mm, addr, addr + 1)
> > >       damon_young_pmd_entry()
> > >         ptep_get(pte)
> > >         mmu_notifier_test_young(walk->mm, addr)
> > > 
> > > test_and_clear_young_ptes(), which backs ptep_test_and_clear_young() on
> > > arm64, documents @addr as "Address the first page is mapped at".
> > > 
> > > For arm64, before commit 6f0e1142173a ("arm64: mm: support batch
> > > clearing of the young flag for large folios"),
> > 
> > The @addr documentation is also introduced by this commit.  This commit is
> > authored at 2026-02-09.

I was wrong.  The documentation was introduced by commit 6d7237dda44f ("mm: add
a batched helper to clear the young flag for large folios"), which was authored
by Baolin on 2026-03-06.

> > 
> > > the contpte helper walked
> > > exactly CONT_PTES entries from the aligned-down page table pointer and
> > > used @addr only to pass down to each entry, so an unaligned value was
> > > harmless:
> > > 
> > >         ptep = contpte_align_down(ptep);
> > >         addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
> > >         for (i = 0; i < CONT_PTES; i++, ptep++, addr += PAGE_SIZE)
> > 
> > So, there was no issue before the commit.
> > 
> 
> Right. Before 6f0e1142173a, unaligned addresses were tolerated but I don't
> think this is guaranteed.
> 
> > > 
> > > Align the sampled address down to a page boundary. It is the address of
> > > the page to sample, so this matches its intended meaning and fixes both
> > > users in vaddr.c.
> > 
> > This indeed sounds like can fix the issue to me.  However, was it a clear rule
> > that we should pass only contepte-aligned addrss to
> > ptep_test_and_clear_young()?  And is DAMON the only ptep_test_and_clear_young()
> > caller that is mistakenly passing the unaligned address?
> > 
> 
> It is not spelled out as an explicit rule, but the documented "Address
> the first page is mapped at" implies it,

You mean the comment on test_and_clear_young_ptes(), right?  But as I mentioned
above, the comment was introduced by Baolin's patch that was authored on
2026-03-06.  I'd still appreciate Baolin's opinion.

> and these callers are using
> aligned addresses:
> 
>  mm/page_idle.c: page_idle_clear_pte_refs_one()
>  fs/proc/task_mmu.c: clear_refs_pte_range()
> 
> > If not, it might make sense to make contpte_test_and_clear_young_ptes() support
> > unaligned adress again in my opinion.  May I ask your opinion, Baolin?
> > 
> > > 
> > > Fixes: 3f49584b262c ("mm/damon: implement primitives for the virtual memory address spaces")
> > 
> > I think 6f0e1142173a ("arm64: mm: support batch clearing of the young flag for
> > large folios") would be mroe correct 'Fixes:', if there was no issue before the
> > commit.
> > 
> 
> Will use that in v2.
> 
> > > -	r->sampling_addr = damon_rand(ctx, r->ar.start, r->ar.end);
> > > +	r->sampling_addr = PAGE_ALIGN_DOWN(damon_rand(ctx, r->ar.start,
> > > +						      r->ar.end));
> > 
> > If we need to have the fix in DAMON, this kind of change would be needed.
> > 
> > However, what happens if the address is backed by large folios?
> > 
> > Before the commit 6f0e1142173a, also, it was aligning to CONT_PTE_SIZE.  Should
> > we do same?
> 
> Passing a page-aligned address restores the pre-6f0e1142173a behavior.
> Before the change, the helper aligned ptep down to the block start and
> walked a fixed CONT_PTES entries, regardless of addr.  After the
> change, the walk covers [ALIGN_DOWN(addr, CONT_PTE_SIZE),
> ALIGN(addr + nr * PAGE_SIZE, CONT_PTE_SIZE)).  With a sub-page offset,
> addr + PAGE_SIZE lands just past the block boundary, so the round-up
> extends the walk a whole block further.  With a page-aligned addr,
> addr + PAGE_SIZE is at most the block end, so the round-up lands
> exactly on the block end and the walk covers the same CONT_PTES
> entries as before the commit.
> 
> We also can't align to CONT_PTE_SIZE in DAMON since it's defined only under
> arch/arm64/:
> 
>   #define CONT_PTES		(1 << (CONT_PTE_SHIFT - PAGE_SHIFT))
>   #define CONT_PTE_SIZE		(CONT_PTES * PAGE_SIZE)

DAMON cares only exactly the byte of the address, so I agree this would work
for DAMON and be safe.  But, still the behavior is not exactly same to
pre-6f0e1142173a, isn't it?  I'm not really sure if this is really the correct
use of the function.  Again, I'd appreciate Baolin's comment.

> 
> 
> > Also, I think we should pass aligned address to only the functions that
> > require alignement.  Making the alignment to the sampling address in general
> > sounds too much to me.  Particularly, we are working on supporting new page
> > access check primitives other than PTE Accessed bit, like AMd IBS.  In the
> > case, we might support <PAGE_SIZE granularity monitoring.  Aligning sampling
> > address in general will make it more complicated.
> 
> Makes sense. I will keep sampling_addr as is and align inside damon_va_mkold()
> and damon_va_young() in v2.

Regardless of Baolin's comment, let's fix this issue.  So the v2 would be
appreciated.  In the v2, could you also add more details about how the issue
can be reproduced, and the user impact?  You mentioned you found memory
corruption from DAMON selftets.  It would be nice if you could make it more
detailed, such as what selftest reproduces the issue and what symptoms it
showed you.

Nevertheless I'm also wondering if supporting unaligned address again, like
below also works.

'''
--- a/arch/arm64/mm/contpte.c
+++ b/arch/arm64/mm/contpte.c
@@ -519,9 +519,12 @@ bool contpte_test_and_clear_young_ptes(struct vm_area_struct *vma,
         * of the same large folio in a single VMA and a single page table.
         */

-       unsigned long end = addr + nr * PAGE_SIZE;
+       unsigned long end;
        bool young = false;

+       ptep = contpte_align_down(ptep);
+       addr = ALIGN_DOWN(addr, CONT_PTE_SIZE);
+       end = addr + nr * PAGE_SIZE;
        ptep = contpte_align_addr_ptep(&addr, &end, ptep, nr);
        for (; addr != end; ptep++, addr += PAGE_SIZE)
                young |= __ptep_test_and_clear_young(vma, addr, ptep);
'''

Nathan, what do you think?  If it makes sense to you, could you also test this?


Thanks,
SJ

[...]