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

[sashiko-bot@kernel.org]

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?

> -		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