Re: [RFC PATCH v2.2 05/28] mm/damon/core: introduce damon_region->probe_hits

[SeongJae Park <sj@kernel.org>]

On Fri, 15 May 2026 00:54:46 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [High] The `probe_hits` array is not propagated to the new sub-region during a region split.
> - [High] The `probe_hits` array is not recalculated during a region merge.
> - [High] The `probe_hits` counters are susceptible to integer overflow.
> - [Medium] The `probe_hits` array is not scaled when monitoring attributes change dynamically.
> --
> 
> commit 5ada6fcb6176988079396fb12d1bf709aa9d2522
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: introduce damon_region->probe_hits
>     
> Add an array for the per-region per-probe positive samples count.  For
> simple and efficient implementation, add a limit to the number of data
> probes and set the array to support only the limited number of counters.
> 
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 9c2576f103767..bf78ad849a9be 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> [ ... ]
> > @@ -80,6 +83,7 @@ struct damon_region {
> >  	unsigned long sampling_addr;
> >  	unsigned int nr_accesses;
> >  	unsigned int nr_accesses_bp;
> > +	unsigned char probe_hits[DAMON_MAX_PROBES];
> >  	struct list_head list;
> 
> Can this overflow?
> 
> Since it is an 8-bit unsigned char, it can hold a maximum of 255.
> The number of samples per aggregation interval is aggr_interval /
> sample_interval. Both are user-configurable via damon_set_attrs(), so
> configurations that yield more than 255 samples per interval could
> overflow this counter and corrupt the exposed data.

Yes, but such configurations may be rare.  The default and recommended
configuration is to have 20 as the max number.  And users who runs proper
testing before deploying changes to the production may find the issue.

But better to avoid that if possible.  I will consider returning error when
such configuration is committed.

> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 15f2795a1156f..f6b4beb4b8ecd 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -235,6 +236,8 @@ struct damon_region *damon_new_region(unsigned long start, unsigned long end)
> >  	region->ar.end = end;
> >  	region->nr_accesses = 0;
> >  	region->nr_accesses_bp = 0;
> > +	for (i = 0; i < DAMON_MAX_PROBES; i++)
> > +		region->probe_hits[i] = 0;
> >  	INIT_LIST_HEAD(&region->list);
> >  
> >  	region->age = 0;
> 
> Are we missing a copy of probe_hits in damon_split_region_at()?
> 
> When a region is split, the new right-hand sub-region gets zeroed
> counters from damon_new_region(), losing its historical positive sample
> counts for the current aggregation interval:
> 
> mm/damon/core.c:damon_split_region_at() {
> 	...
> 	new = damon_new_region(r->ar.start + sz_r, r->ar.end);
> 	if (!new)
> 		return;
> 
> 	r->ar.end = new->ar.start;
> 
> 	new->age = r->age;
> 	new->last_nr_accesses = r->last_nr_accesses;
> 	new->nr_accesses_bp = r->nr_accesses_bp;
> 	new->nr_accesses = r->nr_accesses;
> 	...
> }

Nice catch!  I will do that in the next revision.

> 
> Should probe_hits also be merged in damon_merge_two_regions()?
> 
> When adjacent regions are merged, l->nr_accesses and l->age are
> recalibrated using a size-weighted average. However, the probe_hits
> array of l is left untouched, and the probe_hits data from r is entirely
> discarded:
> 
> mm/damon/core.c:damon_merge_two_regions() {
> 	...
> 	l->nr_accesses = (l->nr_accesses * sz_l + r->nr_accesses * sz_r) /
> 			(sz_l + sz_r);
> 	l->nr_accesses_bp = l->nr_accesses * 10000;
> 	l->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
> 	l->ar.end = r->ar.end;
> 	...
> 	damon_destroy_region(r, t);
> }
> 
> Does probe_hits need to be scaled in damon_update_monitoring_result()
> when monitoring attributes change dynamically?
> 
> Since it tracks positive samples over the aggregation interval just like
> nr_accesses, leaving it unscaled might result in stale values that
> misalign with the new interval constraints.

Good finding!  I will do that in the next revision.


Thanks,
SJ

[...]