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

[sashiko-bot@kernel.org]

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?

[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().

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