Re: [RFC PATCH v1.1 1/3] mm/damon: add damon_region->last_probe_hits

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] When `damon_update_monitoring_result()` is called to apply new attributes mid-aggregation, it resets `r->nr_accesses` but fails to clear `r->probe_hits`. As a result, accumulated probe hits leak across the aborted interval boundary. Furthermore, the newly introduced `r->last_probe_hits` field is completely ignored by this function—it is neither scaled for the new interval attributes (unlike `last_nr_accesses`) nor protected from the `probe_hits` leak. This causes the newly introduced moving sum statistics exposed to userspace to become artificially inflated and mathematically incorrect after an attribute update.
--

commit c9f161ddb2f122eeb9bddfba63cb77e65fe86f9c
Author: SJ Park <sj@kernel.org>

mm/damon: add damon_region->last_probe_hits
    
Add new damon_region field, last_probe_hits. Maintain fully accumulated
probe_hits values from the last aggregation interval in the field.

> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 805e089ff4f2f..4f7e305b261c3 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -68,10 +68,10 @@ struct damon_region {
>  	unsigned int nr_accesses;
>  	unsigned char probe_hits[DAMON_MAX_PROBES];
>  	struct list_head list;
> -
>  	unsigned int age;
>  /* private: Internal value for age calculation. */
>  	unsigned int last_nr_accesses;
> +	unsigned char last_probe_hits[DAMON_MAX_PROBES];
>  };
>  
>  /**

[ ... ]

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index c6fc35be633b3..60be3e9d87887 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> @@ -2040,8 +2042,10 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
>  					damon_nr_regions(t), nr_probes);
>  			r->last_nr_accesses = r->nr_accesses;
>  			r->nr_accesses = 0;
> -			for (i = 0; i < DAMON_MAX_PROBES; i++)
> +			for (i = 0; i < DAMON_MAX_PROBES; i++) {
> +				r->last_probe_hits[i] = r->probe_hits[i];
>  				r->probe_hits[i] = 0;
> +			}

[Severity: Medium]
Is there a regression in how statistics are updated when attributes change
mid-aggregation?

If damon_update_monitoring_result() is called to apply new attributes, it
resets nr_accesses but doesn't clear the probe_hits array:

mm/damon/core.c:damon_update_monitoring_result() {
    ...
	if (!aggregating)
		r->nr_accesses = damon_nr_accesses_for_new_attrs(
				r->nr_accesses, old_attrs, new_attrs);
	else
		/*
		 * if this is called in the middle of the aggregation, reset
		 * the aggregations we made so far for this aggregation
		 * interval.  In other words, make the status like
		 * kdamond_reset_aggregated() is called.
		 */
		r->nr_accesses = 0;
    ...
}

This appears to completely ignore the newly introduced last_probe_hits array.

Since last_nr_accesses is scaled for the new interval attributes, should
last_probe_hits also be scaled to maintain accurate moving sum statistics?

Also, should probe_hits be cleared for mid-aggregation updates to prevent
leaking accumulated counts into the aborted interval?

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