Re: [PATCH 6.12.y v3 1/2] mm/damon/core: do non-safe region walk on kdamond_apply_schemes()

[SJ Park <sj@kernel.org>]

On Fri, 09 Oct 2026 12:55:36 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] mm: damon: double application of DAMOS schemes due to non-safe region walk
> --
> 
> commit 7c60a2f66acad38f06b09b2a9e484519168800bc
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: do non-safe region walk on kdamond_apply_schemes()
> 
> This commit changes kdamond_apply_schemes() to use the non-safe
> damon_for_each_region() iterator instead of damon_for_each_region_safe().
> It also drops the double-pointer workaround in damos_skip_charged_region()
> to let newly split regions be seen on the next iteration of the region walk.
> 
> Link: https://lkml.kernel.org/r/20260227170623.95384-3-sj@kernel.org
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 32737cc5f877b..76f23c1d2f56b 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1749,7 +1749,7 @@ static void kdamond_apply_schemes(struct damon_ctx *c)
> >  		return;
> >  
> >  	damon_for_each_target(t, c) {
> > -		damon_for_each_region_safe(r, next_r, t)
> > +		damon_for_each_region(r, t)
> >  			damon_do_apply_schemes(c, t, r);
> >  	}
> 
> [Severity: High]
> Does this change cause earlier schemes to double-apply their actions when a
> subsequent scheme splits the region?
> 
> In kdamond_apply_schemes(), changing to the non-safe iterator means the loop
> dynamically visits newly created fragments on the very next iteration.
> 
> When damon_do_apply_schemes() runs, it evaluates all schemes sequentially on
> the region:
> 
> mm/damon/core.c:damon_do_apply_schemes() {
> 	...
> 	damon_for_each_scheme(s, c) {
> 		...
> 		if (damos_skip_charged_region(t, r, s))
> 			continue;
> 		...
> 		damos_apply_scheme(c, t, r, s);
> 	}
> }
> 
> If an earlier scheme evaluates the region, it may apply its action and update
> its statistics for the full size of the region.
> 
> If a subsequent scheme then evaluates the same region and splits it (for
> example via damos_skip_charged_region()), it shrinks the original region and
> inserts a new region immediately after it.
> 
> Because damon_for_each_region() is no longer safe against modifications to the
> list, the next iteration of the outer loop will process the newly inserted
> region across all schemes.
> 
> Since the newly inserted region retains the exact properties (age, access
> counts) of the original region, will the earlier scheme re-evaluate it and
> apply its action a second time, double-charging its statistics and applying
> redundant actions to the memory?

Good finding.  But the issue is also in the mainline.  Also, because DAMOS
quota is a sort of best effort, the user impact is minor.  I think this is not
a blocker of this backport patch.  I will separately work on it.

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


Thanks,
SJ