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

[sashiko-bot@kernel.org]

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?

[ ... ]

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

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