Re: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization

On Mon, 13 Jul 2026 14:06:53 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Incomplete fix: identical data race in `mm/damon/paddr.c` causes access statistics corruption
> --
> 
> --- Patch [3]: [RFC PATCH v1.2 3/5] mm/damon/vaddr: drop last same folio access check optimization ---
> commit d42bea4a157d94e45112b2190d4aba2a5eeffaa2
> Author: SJ Park <[email protected]>
> 
> mm/damon/vaddr: drop last same folio access check optimization
> 
> This patch removes the "last same folio access check" optimization in
> vaddr.c to fix a data race when multiple kdamonds are running.
> 
> Link: https://lore.kernel.org/[email protected]
> 
> > diff --git a/mm/damon/vaddr.c b/mm/damon/vaddr.c
> > index d10b8042adb5b..d487b7a4a1042 100644
> > --- a/mm/damon/vaddr.c
> > +++ b/mm/damon/vaddr.c
> 
> [ ... ]
> 
> > @@ -494,28 +487,17 @@ static bool damon_va_young(struct mm_struct *mm, unsigned long addr,
> >   * r	the region to be checked
> >   */
> >  static void __damon_va_check_access(struct mm_struct *mm,
> > -				struct damon_region *r, bool same_target)
> > +				struct damon_region *r)
> >  {
> > -	static unsigned long last_addr;
> > -	static unsigned long last_folio_sz = PAGE_SIZE;
> > -	static bool last_accessed;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the same data race still
> exist in mm/damon/paddr.c?
> 
> Looking at __damon_pa_check_access() in mm/damon/paddr.c, it seems to have the
> exact same caching optimization using static variables:
> 
> 	static phys_addr_t last_addr;
> 	static unsigned long last_folio_sz = PAGE_SIZE;
> 	static bool last_accessed;
> 
> Since these static variables aren't protected by any locks, could concurrent
> kdamond threads using DAMON_OPS_PADDR clobber each other's state in the exact
> same way?
> 
> For example, if Thread A processes a 2MB huge page and updates last_folio_sz
> to 2MB, could Thread B evaluate its next sampling_addr using Thread A's 2MB
> last_folio_sz?
> 
> This would cause ALIGN_DOWN(last_addr, 2MB) == ALIGN_DOWN(sampling_addr, 2MB)
> to incorrectly evaluate to true for up to 512 consecutive 4KB pages, assigning
> them all the last_accessed value without actually checking them. Should this
> identical optimization be removed from paddr.c as well to completely resolve
> the data race?

Yes, that's what the next patch in this series is fixing.

> 
> > +	bool accessed;
> >  
> >  	if (!mm) {
> >  		damon_update_region_access_rate(r, false);
> >  		return;
> >  	}
> >  
> > -	/* If the region is in the last checked page, reuse the result */
> > -	if (same_target && (ALIGN_DOWN(last_addr, last_folio_sz) ==
> > -				ALIGN_DOWN(r->sampling_addr, last_folio_sz))) {
> > -		damon_update_region_access_rate(r, last_accessed);
> > -		return;
> > -	}
> > -
> > -	last_accessed = damon_va_young(mm, r->sampling_addr, &last_folio_sz);
> > -	damon_update_region_access_rate(r, last_accessed);
> > -
> > -	last_addr = r->sampling_addr;
> > +	accessed = damon_va_young(mm, r->sampling_addr);
> > +	damon_update_region_access_rate(r, accessed);
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3


Thanks,
SJ