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

[SJ Park <sj@kernel.org>]

On Thu, 02 Jul 2026 17:29:05 +0000 sashiko-bot@kernel.org wrote:

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

This is indeed something that can better improved.

The function is not updating probe_hits, either, though.  So no regression.
Not a blocker of this patch.


Thanks,
SJ

[...]