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

[SJ Park <sj@kernel.org>]

On Thu, 09 Jul 2026 14:48:55 +0000 sashiko-bot@kernel.org wrote:

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

That will be implemented by a later patch of this series.

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

This is an intentional design.  The user should config DAMON correctly.

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


Thanks,
SJ