Re: [RFC PATCH v1.1 11/16] mm/damon/core: get merge threshold from probe hits when weights are set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Region merge loop terminates prematurely because `max_thres` is unscaled for probe hit weighted sums, causing a failure to respect `max_nr_regions`.
- [High] Adding probe weights to contexts using operations without `apply_probes` (e.g., `vaddr`) silently breaks all monitoring.
--

commit f896f3e81bf75b446b16cb4f79f6673a154b512b
Author: SJ Park <sj@kernel.org>

mm/damon/core: get merge threshold from probe hits when weights are set
    
When probe weights are set, DAMON merges regions based on their probe
hits weighted sum.  But the merge threshold is calculated based on the
access frequency.  Update it to retrieve the maximum probe hits weighted
sum in the snapshot from apply_probes() ops callback, and generate the
threshold based on it.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index be026c86384e4..fa78d8d592aa3 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> @@ -3726,9 +3727,18 @@ static int kdamond_fn(void *data)
>  
>  		if (!access_check_disabled && ctx->ops.check_accesses)
>  			max_merge_score = ctx->ops.check_accesses(ctx);

[Severity: High]
Does setting probe weights on a context that lacks an apply_probes callback
(like vaddr) silently break monitoring here?

If a user sets probe weights via sysfs, access_check_disabled will be true.
This skips check_accesses(), but since apply_probes is NULL, neither callback
runs. The accesses are never updated, and all memory appears completely cold.

> -		if (ctx->ops.apply_probes)
> -			ctx->ops.apply_probes(ctx, access_check_disabled,
> -					false);
> +		if (ctx->ops.apply_probes) {
> +			if (time_after_eq(ctx->passed_sample_intervals,
> +						next_aggregation_sis) &&
> +					access_check_disabled)
> +				get_max_wsum = true;
> +			else
> +				get_max_wsum = false;
> +			max_wsum = ctx->ops.apply_probes(ctx,
> +					access_check_disabled, get_max_wsum);
> +			if (get_max_wsum)
> +				max_merge_score = max_wsum;

[Severity: High]
Can this cause kdamond_merge_regions() to fail to reduce the number of
regions below max_nr_regions?

When kdamond_fn() later passes this max_wsum / 10 as the initial threshold
to kdamond_merge_regions(), the merge loop termination condition compares
it against an unscaled max_thres:

mm/damon/core.c:kdamond_merge_regions() {
	...
	max_thres = c->attrs.aggr_interval /
		(c->attrs.sample_interval ?  c->attrs.sample_interval : 1);
	...
	} while (nr_regions > c->attrs.max_nr_regions &&
			threshold / 2 < max_thres);
}

Since a user-defined weighted sum can be vastly larger than the unweighted
max_thres, will the loop condition fail prematurely, leading to an
unbounded growth of memory regions?

> +		}
>  
>  		if (time_after_eq(ctx->passed_sample_intervals,
>  					next_aggregation_sis)) {

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