Re: [RFC PATCH v1.2 08/19] mm/damon/core: extend merge function to work with probe hits

[SJ Park <sj@kernel.org>]

On Thu, 09 Jul 2026 14:32:15 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Region `age` is incorrectly updated multiple times per aggregation interval and evaluated against a stale `last_score` when the `kdamond_merge_regions` retry loop is triggered.
> --
> 
> --- Patch [8]: [RFC PATCH v1.2 08/19] mm/damon/core: extend merge function to work with probe hits ---
> commit 938ea22fc6c1fec3c909339503398aa70ea047c8
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: extend merge function to work with probe hits
> 
> When probe weights are set, users may want DAMON monitoring results to
> be optimized for the weights.  For that, regions adjustment should work
> for the weighted sum of probe hits.  Extend damon_merge_regions_of() to
> detect if the weights are set, and work with probe hits in the case.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> 
> [ ... ]
> 
> > @@ -3256,24 +3271,38 @@ static void damon_merge_regions_of(struct damon_target *t, unsigned int thres,
> > -				   unsigned long sz_limit)
> > +		unsigned long sz_limit, struct damon_ctx *ctx)
> >  {
> >  	struct damon_region *r, *prev = NULL, *next;
> > +	bool use_probe_hits = damon_has_probe_weights(ctx);
> >  
> >  	damon_for_each_region_safe(r, next, t) {
> > -		if (abs_diff(r->nr_accesses, r->last_nr_accesses) > thres)
> > +		unsigned int score, last_score, diff;
> > +
> > +		score = damon_merge_score(r, false, ctx, use_probe_hits);
> > +		last_score = damon_merge_score(r, true, ctx, use_probe_hits);
> > +
> > +		if (abs_diff(score, last_score) > thres)
> >  			r->age = 0;
> > -		else if ((r->nr_accesses == 0) != (r->last_nr_accesses == 0))
> > +		else if ((score == 0) != (last_score == 0))
> >  			r->age = 0;
> >  		else
> >  			r->age++;
> 
> [Severity: High]
> This is a pre-existing issue, but does this logic in damon_merge_regions_of()
> incorrectly increment or reset r->age multiple times per aggregation interval?
> 
> When the number of regions exceeds max_nr_regions, kdamond_merge_regions() 
> repeatedly calls damon_merge_regions_of() with doubled thresholds. During 
> these retries, r->age is incremented multiple times within a single 
> aggregation interval.
> 
> Furthermore, because damon_merge_two_regions() updates the current 
> nr_accesses and probe_hits but fails to merge their historical counterparts 
> (last_nr_accesses and last_probe_hits), these values become stale.
> 
> On subsequent iterations of the retry loop, damon_merge_score() computes 
> last_score using these stale unmerged values. This can cause 
> abs_diff(score, last_score) to spuriously exceed the threshold, resetting 
> r->age to 0 inappropriately.
> 
> Could this cause DAMOS age-based schemes to trigger prematurely or fail to 
> trigger entirely?

I already promised to separately work on this.

> 
> > -		if (prev && prev->ar.end == r->ar.start &&
> > -		    abs_diff(prev->nr_accesses, r->nr_accesses) <= thres &&
> > -		    damon_sz_region(prev) + damon_sz_region(r) <= sz_limit)
> > -			damon_merge_two_regions(t, prev, r);
> > -		else
> > -			prev = r;
> > +		if (!prev)
> > +			goto set_prev_continue;
> > +		if (prev->ar.end != r->ar.start)
> > +			goto set_prev_continue;
> > +		diff = abs_diff(score, damon_merge_score(prev, false, ctx,
> > +					use_probe_hits));
> > +		if (diff > thres)
> > +			goto set_prev_continue;
> > +		if (damon_sz_region(prev) + damon_sz_region(r) > sz_limit)
> > +			goto set_prev_continue;
> > +		damon_merge_two_regions(t, prev, r);
> > +		continue;
> > +set_prev_continue:
> > +		prev = r;
> >  	}
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260709140600.90950-1-sj@kernel.org?part=8


Thanks,
SJ