Re: [RFC PATCH v1.1 08/16] mm/damon/core: disable access monitoring when probe weights are set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The region merging loop termination condition is incompatible with weighted probe hits, leading to unbounded region growth.
- [High] Unconditionally disabling access checks forces `nr_accesses` to 0, breaking access-based DAMOS schemes and interval auto-tuning.
--

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

mm/damon/core: disable access monitoring when probe weights are set

When users set probe weights, they are interested in the data attributes
rather than data access patterns. This commit disables data access
monitoring when probe weights are set.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 7aff741d43bd4..a2f768bd07223 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3713,17 +3713,18 @@ static int kdamond_fn(void *data)
>  		unsigned long next_aggregation_sis = ctx->next_aggregation_sis;
>  		unsigned long next_ops_update_sis = ctx->next_ops_update_sis;
>  		unsigned long sample_interval = ctx->attrs.sample_interval;
> +		bool access_check_disabled = damon_has_probe_weights(ctx);

[Severity: High]
Can configuring probe weights lead to unbounded region growth during merging?

When damon_has_probe_weights(ctx) is true, the region merging loop in
kdamond_merge_regions() compares a threshold scaled with weighted probe hits
against max_thres:

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);
}

Since max_thres is hardcoded to the maximum unscaled accesses
(aggr_interval / sample_interval) and doesn't account for probe weights, the
loop could terminate prematurely.

If weighted probe hits result in score differences exceeding this unscaled cap,
wouldn't this prevent regions from merging back down below the user-configured
max_nr_regions limit?

>  
>  		if (kdamond_wait_activation(ctx))
>  			break;
>  
> -		if (ctx->ops.prepare_access_checks)
> +		if (!access_check_disabled && ctx->ops.prepare_access_checks)
>  			ctx->ops.prepare_access_checks(ctx);
>  
>  		kdamond_usleep(sample_interval);
>  		ctx->passed_sample_intervals++;
>  
> -		if (ctx->ops.check_accesses)
> +		if (!access_check_disabled && ctx->ops.check_accesses)
>  			max_nr_accesses = ctx->ops.check_accesses(ctx);

[Severity: High]
Does disabling the access checks here break DAMOS schemes and interval
auto-tuning?

By preventing ctx->ops.check_accesses from running when probe weights are
configured, r->nr_accesses will remain 0.

Since damon_nr_accesses_mvsum() consumes these 0s, it seems this would cause
interval auto-tuning to exponentially increase the sampling interval until it
hits the maximum allowed value.

Additionally, wouldn't this force all regions to instantly match any DAMOS
scheme configured to target low-access memory?

>  		if (ctx->ops.apply_probes)
>  			ctx->ops.apply_probes(ctx, false, false);

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