Re: [PATCH v3] mm/damon/ops-common: use a page-aligned address in damon_ptep_mkold()

[SJ Park]

On Wed, 2 Sep 2026 10:34:39 +0800 Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> wrote:

> 
> 
> On 9/2/26 8:11 AM, SJ Park wrote:
> > On Tue, 1 Sep 2026 13:10:01 -0700 Nathan Gao <zcgao@xxxxxxxxxx> wrote:
> > 
> >> __damon_va_prepare_access_check() picks a random byte address within the
> >> region and stores it in r->sampling_addr. damon_va_mkold() passes it into
> >> a page table walk, which hands it to damon_ptep_mkold() as the address of
> >> the page to sample:
> >>
> >>    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)
> >>
> >> For arm64, before commit 6f0e1142173a ("arm64: mm: support batch
> >> clearing of the young flag for large folios"), 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)
> >>
> >> Now the range to walk is derived from @addr instead: end = addr +
> >> nr * PAGE_SIZE, rounded up to CONT_PTE_SIZE. For a sample in the last
> >> page of a contpte block, the sub-page offset puts end just past the
> >> block boundary, so the round-up lands a whole block further and the
> >> walk clears PTE_AF in CONT_PTES entries beyond the sampled block. For
> >> the last block in a page table page, those entries are past the end of
> >> that page, so the walk writes into the page that follows.
> > 
> > I just wanted to call out again that I'm wondering if we could restore the
> > unaligned address support in the helper.  E.g., as a very dirty hack that I can
> > imagine off the top of my head,
> > 
> > '''
> > --- a/arch/arm64/mm/contpte.c
> > +++ b/arch/arm64/mm/contpte.c
> > @@ -30,6 +30,7 @@ static inline pte_t *contpte_align_addr_ptep(unsigned long *start,
> >                                               unsigned long *end, pte_t *ptep,
> >                                               unsigned int nr)
> >   {
> > +       *start = PAGE_ALIGN_DOWN(*start);
> >          /*
> >           * Note: caller must ensure these nr PTEs are consecutive (present)
> >           * PTEs that map consecutive pages of the same large folio within a
> > '''
> > 
> > I and Nathan have no strong clue, so we are looking for Baolin and others'
> > opinion.
> > 
> > While waiting for the opinions, I and Nathan agree we should stop bleeding with
> > a pinpoint hotfix change in DAMON.
> 
> Thanks for the reporting.
> 
> IMO, we could let the arch low-level functions handle the alignment of 
> addr, but that would also require changing functions like 
> contpte_clear_young_dirty_ptes(), contpte_set_ptes() and so on, which 
> would cause a lot of churn? (they also assume that addr is page aligned).
> 
> Since the arch low-level functions basically assume that addr is 
> page-size aligned, and the addr handling in the mm core is also mostly 
> page-size aligned, I think the caller guaranteeing that addr is 
> page-size aligned is a reasonable fix. So:

Thank you for your opinion, Baolin.  That makes sense to me.  I will try to
further revisit DAMON code to make alignment be more complete and consistent
whenever needed.

> 
> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>


Thanks,
SJ