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

[Nathan Gao <zcgao@amazon.com>]

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.
> 
> > 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, 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)


> 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.

Thanks,
Nathan