Re: [PATCH 14/19] 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:
- [Medium] Silent monitoring failure when probe weights are set on operations lacking apply_probes.
- [Medium] kdamond_merge_regions loop termination condition still uses unweighted access frequency as max_thres.
--

--- Patch [14]: [PATCH 14/19] mm/damon/core: get merge threshold from probe hits when weights are set ---
commit 0b09a73e4b353610f5bc3d4f305131a2f9d7fea8
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 f7782c69037da..91964336ce31e 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3776,9 +3777,18 @@ static int kdamond_fn(void *data)
>  
>  		if (!access_check_disabled && ctx->ops.check_accesses)
>  			max_merge_score = ctx->ops.check_accesses(ctx);
> -		if (ctx->ops.apply_probes)
> -			ctx->ops.apply_probes(ctx, access_check_disabled,
> -					false);
> +		if (ctx->ops.apply_probes) {

[Severity: Medium]
If a user configures a DAMON context with ops that lack apply_probes
(such as vaddr), adds a probe with a weight, and starts monitoring,
could this result in a silent monitoring failure?

Since damon_valid_probe_params() allows this configuration:

mm/damon/core.c:damon_valid_probe_params() {
...
	return true;
}

access_check_disabled will be true, causing check_accesses() to be
skipped. With ctx->ops.apply_probes being NULL, no monitoring data
would be collected and max_merge_score would remain 0.

> +			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;
> +		}
>  
>  		if (time_after_eq(ctx->passed_sample_intervals,
>  					next_aggregation_sis)) {

[Severity: Medium]
When kdamond_merge_regions() is called with this new max_merge_score based
on weighted probe hits, could the fallback loop terminate prematurely?

mm/damon/core.c:kdamond_fn() {
...
			kdamond_merge_regions(ctx,
					max_merge_score / 10,
					sz_limit);
...
}

Looking at kdamond_merge_regions(), max_thres is hardcoded to the
unweighted maximum access frequency:

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

If a user sets a probe weight such that max_merge_score / 10 > max_thres * 2,
the loop condition threshold / 2 < max_thres will evaluate to false immediately,
failing to enforce the max_nr_regions limit during region merging.

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