Re: [RFC PATCH v1.1 04/16] mm/damon/core: ask apply_probe() to return max probe hits weighted sum

[SJ Park <sj@kernel.org>]

On Mon, 06 Jul 2026 14:39:37 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Advancing physical addresses using `addr += folio_size(folio)` in large folio iteration paths skips memory when `addr` is not aligned to the start of the folio.
> - [High] Static local variables used for caching are shared across concurrent kernel threads without synchronization.
> --
> 
> commit 1ecdc723ec9bfdc97493a942107d11622114a694
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: ask apply_probe() to return max probe hits weighted sum
> 
> check_accesses() DAMON ops callback returns the maximum nr_accesses of
> regions.  DAMON core uses it to calculate a reasonable region merge
> threshold.  The core will need to adjust regions for not nr_accesses but
> probe hits weighted sum in future.  For that, the core needs to know the
> maximum weighted sum of the regions.  Update the protocol for the task.
> 
> > diff --git a/mm/damon/paddr.c b/mm/damon/paddr.c
> > index ff4fcca949660..3eba3564a31fc 100644
> > --- a/mm/damon/paddr.c
> > +++ b/mm/damon/paddr.c
> > @@ -166,7 +166,8 @@ static bool damon_pa_filter_pass(phys_addr_t pa, struct folio *folio,
> >  	return pass;
> >  }
> >  
> > -static void damon_pa_apply_probes(struct damon_ctx *ctx, bool set_samples)
> > +static unsigned int damon_pa_apply_probes(struct damon_ctx *ctx,
> > +		bool set_samples, bool return_max_wsum)
> >  {
> >  	struct damon_target *t;
> >  	struct damon_region *r;
> > @@ -194,6 +195,7 @@ static void damon_pa_apply_probes(struct damon_ctx *ctx, bool set_samples)
> >  				folio_put(folio);
> >  		}
> >  	}
> > +	return 0;
> >  }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing the changes in
> mm/damon/paddr.c, I noticed that __damon_pa_check_access() declares static
> local variables for caching:
> 
> mm/damon/paddr.c:__damon_pa_check_access() {
> 	static phys_addr_t last_addr;
> 	static unsigned long last_folio_sz = PAGE_SIZE;
> 	static bool last_accessed;
> 	...
> }
> 
> Because DAMON allows multiple contexts to be created and run concurrently,
> each in its own kdamond thread, could these static variables be modified
> concurrently without locking? Does this data race risk corrupting the cache
> state and misapplying access results between different monitoring contexts?

Correct.  I'm separately working [1] on this.

> 
> [Severity: High]
> This is a pre-existing issue, but I also noticed a potential overshooting bug
> in multiple physical address iteration paths in mm/damon/paddr.c, such as in
> damon_pa_pageout():
> 
> mm/damon/paddr.c:damon_pa_pageout() {
> 	...
> 	addr += folio_size(folio);
> 	...
> }
> 
> If a region start address is not aligned to the size of the folio (for
> instance, starting in the middle of a 2MB THP), advancing the unaligned addr
> by the full folio size will overshoot the folio boundary. Could this silently
> skip valid physical memory that falls in the gap?
> 
> The same pattern appears to be used in damon_pa_de_activate(),
> damon_pa_migrate(), and damon_pa_stat().

correct.  I'm separately working [2] on this.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260706141912.88445-1-sj@kernel.org?part=4

[1] https://lore.kernel.org/20260621204050.10993-1-sj@kernel.org
[2] https://lore.kernel.org/20260517234112.89245-1-sj@kernel.org

Thanks,
SJ