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

[sashiko-bot@kernel.org]

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.

> 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;
	...
}

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.

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