Re: [RFC PATCH v1.2 12/19] mm/damon/core: set samples in apply_probes() if probe weights are set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] When access checks are disabled, `max_nr_accesses` is not updated but is still used for the region merge threshold. `apply_probes()` is incorrectly called without requesting or capturing the maximum weighted sum.
- [Medium] Setting probe weights on a context whose operations do not implement `.apply_probes` (e.g., `vaddr`) silently disables all monitoring.
--

--- Patch [12]: [RFC PATCH v1.2 12/19] mm/damon/core: set samples in apply_probes() if probe weights are set ---
commit ffce494dcbbb1125634a81b14be56b2f89aefe7e
Author: SJ Park <sj@kernel.org>

    mm/damon/core: set samples in apply_probes() if probe weights are set

When probe weights are set, the prepare_access_checks() ops callback is
not invoked, so sampling addresses of regions are unset.  Ask
apply_probes() to set those together in the case.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index e250b7080708d..f1be748e9a674 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -3777,7 +3777,8 @@ static int kdamond_fn(void *data)
>  		if (!access_check_disabled && ctx->ops.check_accesses)
>  			max_nr_accesses = ctx->ops.check_accesses(ctx);
>  		if (ctx->ops.apply_probes)
> -			ctx->ops.apply_probes(ctx, false, false);
> +			ctx->ops.apply_probes(ctx, access_check_disabled,
> +					false);

[Severity: Medium]
When access checks are disabled, check_accesses() is bypassed and
max_nr_accesses is not updated.

Later in this function, kdamond_merge_regions() uses max_nr_accesses to
calculate the merge threshold:

		if (time_after_eq(ctx->passed_sample_intervals,
					next_aggregation_sis)) {
			kdamond_merge_regions(ctx,
					max_nr_accesses / 10,
					sz_limit);

Since apply_probes() is called with return_max_wsum=false, it doesn't
update or return a max weighted sum.  Will kdamond_merge_regions() use an
invalid or stale threshold (often 0), breaking the merge algorithm?

[Severity: Medium]
If a user sets probe weights on a context whose operations do not implement
.apply_probes (e.g., vaddr), access_check_disabled will be true and
check_accesses() will be bypassed.

Since .apply_probes is not implemented, this block is also skipped.

Could this completely disable all monitoring for the target with no error
feedback to the user?

Validation in damon_valid_probe_params() successfully validates this state:

mm/damon/core.c:damon_valid_probe_params() {
...
	if (!damon_has_probe_weights(ctx))
		return true;
...
}

Should damon_valid_probe_params() check if ctx->ops.apply_probes is supported?

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

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