Re: [RFC PATCH v1.1 12/16] mm/damon/core: implement damon_has_probe_weight()

[SJ Park <sj@kernel.org>]

On Mon, 06 Jul 2026 15:16:52 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Setting probe weights on a DAMON context whose operations do not support `apply_probes` (e.g., `vaddr`) silently disables all monitoring.
> 
> Pre-existing issues:
> - [High] The `probe_hits` array in `struct damon_region` uses `unsigned char` counters which wrap around at 256, corrupting monitoring data when the number of samples per aggregation interval exceeds 255.
> --
> 
> commit 3891f362834bef45987ebcee1cdb0abcd9df6ea2
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: implement damon_has_probe_weight()
> 
> DAMON monitors only data attributes when probe weights are set.
> However, the function for detecting it is intentionally returning false
> always.  The intention was to make changes for complete support of the
> mode in a safe and incremental way.  Now all the building blocks are
> made.  Complete the mode detection function so that the mode can really
> start working.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index fa78d8d592aa3..6e5a029989a0c 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -211,6 +211,12 @@ static struct damon_probe *damon_nth_probe(int n, struct damon_ctx *ctx)
> >  
> >  static bool damon_has_probe_weights(struct damon_ctx *c)
> >  {
> > +	struct damon_probe *p;
> > +
> > +	damon_for_each_probe(p, c) {
> > +		if (p->weight)
> > +			return true;
> 
> [Severity: High]
> Does returning true here silently disable monitoring if the context operations
> do not support probes (e.g., vaddr)?
> 
> In kdamond_fn(), if this returns true, access_check_disabled bypasses standard
> checks. However, if ctx->ops.apply_probes is NULL, the probes are skipped:
> 
> mm/damon/core.c:kdamond_fn() {
>     ...
> 		bool access_check_disabled = damon_has_probe_weights(ctx);
>     ...
> 		if (!access_check_disabled && ctx->ops.prepare_access_checks)
> 			ctx->ops.prepare_access_checks(ctx);
>     ...
> 		if (ctx->ops.apply_probes) {
>     ...
> }
> 
> Would it be better to ensure ctx->ops.apply_probes is also implemented before
> returning true?

I will fix this in the next revision.

> 
> [Severity: High]
> This is a pre-existing issue, but since this patch enables probe weights,
> can the 8-bit probe_hits counters overflow if the number of samples per
> aggregation interval exceeds 255?
> 
> Looking at struct damon_region in include/linux/damon.h, probe_hits is
> declared as an unsigned char array:
> 
> include/linux/damon.h:struct damon_region {
>     ...
> 	unsigned char probe_hits[DAMON_MAX_PROBES];
>     ...
> }
> 
> In damon_pa_apply_probes(), it is incremented during every sample interval
> without checking for overflow:
> 
> mm/damon/paddr.c:damon_pa_apply_probes() {
>     ...
> 			damon_for_each_probe(p, ctx) {
> 				if (damon_pa_filter_pass(pa, folio, p))
> 					r->probe_hits[i]++;
>     ...
> }
> 
> If a user configures an aggregation interval that is > 255 times the sample
> interval, could highly-hit regions wrap around to 0 and be falsely identified
> as cold memory?

Known low priority issue.  No blocker.

> 
> > +	}
> >  	return false;
> >  }
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260706141912.88445-1-sj@kernel.org?part=12


Thanks,
SJ

[...]