Re: [RFC PATCH v2.2 07/28] mm/damon/core: do data attributes monitoring

[SeongJae Park <sj@kernel.org>]

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] The `probe_hits` statistics are dropped or corrupted when DAMON regions are merged or split.
> - [Medium] The `probe_hits` array is defined as `unsigned char` and is susceptible to integer overflow.
> --
> 
> commit b280c3cfd980db677209be247ad602d23aa561f9
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/core: do data attributes monitoring
> 
> Implement the data attributes monitoring execution.  Update kdamond to
> invoke the probes application callback, and reset the aggregated number
> of per-region per-probe positive samples for every aggregation interval.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index f6b4beb4b8ecd..1c9d2fb69f98d 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1886,10 +1886,14 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
> >  		struct damon_region *r;
> >  
> >  		damon_for_each_region(r, t) {
> > +			int i;
> > +
> >  			trace_damon_aggregated(ti, r, damon_nr_regions(t));
> >  			damon_warn_fix_nr_accesses_corruption(r);
> >  			r->last_nr_accesses = r->nr_accesses;
> >  			r->nr_accesses = 0;
> > +			for (i = 0; i < DAMON_MAX_PROBES; i++)
> > +				r->probe_hits[i] = 0;
> 
> Is it possible for the probe_hits array values to wrap around?
> 
> The probe_hits array is defined as unsigned char in struct damon_region. It
> holds the count of positive probe hits over an aggregation interval, and gets
> incremented every sample interval.
> 
> Since the maximum number of samples per aggregation is dictated by
> attrs.aggr_interval / attrs.sample_interval, and this is fully configurable by
> users, what happens if the number of samples per aggregation exceeds 255?
> 
> Could the unsigned char array elements overflow and wrap around, yielding
> truncated hit counts for high-frequency sampling configurations?

Yes, but no problem as I previously replied [1] to the same question.
> 
> [ ... ]
> 
> > @@ -3172,6 +3176,8 @@ static int kdamond_fn(void *data)
> >  
> >  		if (ctx->ops.check_accesses)
> >  			max_nr_accesses = ctx->ops.check_accesses(ctx);
> > +		if (ctx->ops.apply_probes)
> > +			ctx->ops.apply_probes(ctx);
> >  
> >  		if (time_after_eq(ctx->passed_sample_intervals,
> >  					next_aggregation_sis)) {
> 
> Does this miss updating probe_hits when regions are merged or split?
> 
> Looking at damon_merge_two_regions(), it averages nr_accesses and age when
> merging adjacent regions, but ignores the probe_hits array. This leaves the
> left region's array unmodified and discards the right region's hits.
> 
> Similarly, damon_split_region_at() creates a new sub-region but does not
> appear to copy the probe_hits from the parent, leaving the newly split
> right half initialized to zero.
> 
> Since splitting and merging execute at the end of an aggregation interval,
> will the probe_hits statistics be under-reported or corrupted?

Correct, and I will fix those as I previous replied [1] to the same question.

[1] https://lore.kernel.org/20260516003559.141476-1-sj@kernel.org


Thanks,
SJ

[...]