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

[SJ Park <sj@kernel.org>]

On Fri, 03 Jul 2026 17:24:47 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

No blocker for this patch.  Read below for more details.

> 
> New issues:
> - [Medium] `damos_apply_target()` merges regions without validating or averaging the newly added `last_probe_hits` array.
> - [Medium] Missing reset of `probe_hits` when monitoring attributes change dynamically mid-aggregation corrupts the new `last_probe_hits`.
> 
> Pre-existing issues:
> - [High] `last_nr_accesses` and newly added `last_probe_hits` are not updated or averaged when regions are merged in `damon_merge_two_regions()`.
> --
> 
> commit 43369d5eda0b4a1301ca211bc77bce0681b92eac
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon: add damon_region->last_probe_hits
> 
> This commit adds a new last_probe_hits array to the damon_region struct.
> It maintains fully accumulated probe_hits values from the last aggregation
> interval in this new 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];
> >  };
> 
> [Severity: High]
> This is a pre-existing issue, but does damon_merge_two_regions() also need
> to average last_nr_accesses and this newly added last_probe_hits array?
> 
> In 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->age = (l->age * sz_l + r->age * sz_r) / (sz_l + sz_r);
>     l->ar.end = r->ar.end;
> 
> Because last_nr_accesses and last_probe_hits are left unmodified as the left
> region's values, it seems this can cause spurious age resets in later loop
> passes. If the left region's last_nr_accesses was 0, but the newly averaged
> nr_accesses is non-zero, the condition ((r->nr_accesses == 0) !=
> (r->last_nr_accesses == 0)) evaluates to true and spuriously resets r->age
> to 0.

Good finding.  Too mild issue to Cc stable@, though.  And not a blocker of this
patch.  I will separately work on this.

> 
> [Severity: Medium]
> Should this newly added field also be validated in damos_apply_target()?
> 
> When damos_apply_target() iterates over regions, it merges adjacent ones if
> their age, nr_accesses, and last_nr_accesses match:
> 
>     if (prev_r->last_nr_accesses != r->last_nr_accesses)
>         continue;
>     if (prev_r->nr_accesses != r->nr_accesses)
>         continue;
> 
> Because last_probe_hits isn't compared, regions with different probe statistics
> might be erroneously merged, and the right region's probe hits would be
> silently discarded.

DAMOS works only for nr_accesses.  At the moment, probe_hits are only
additional information.  So this is not an issue.

> 
> [ ... ]
> 
> > 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;
> > +			}
> >  		}
> >  		ti++;
> >  	}
> 
> [Severity: Medium]
> Could this copy un-reset, partial-interval probe_hits into last_probe_hits
> when monitoring attributes change dynamically?
> 
> When damon_set_attrs() dynamically changes intervals mid-aggregation,
> damon_update_monitoring_result() explicitly resets nr_accesses to drop the
> partial state:
> 
>     if (!aggregating)
>         r->nr_accesses = damon_nr_accesses_for_new_attrs(r->nr_accesses,
>                                                          old_attrs, new_attrs);
>     else
>         r->nr_accesses = 0;
> 
> However, probe_hits is not reset. When kdamond_reset_aggregated() fires next,
> will it populate last_probe_hits with mixed-interval garbage, violating the
> fully accumulated semantic guarantee?

At the moment, probe_hits are only additional information.  So this is not an
issue.

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

Thanks,
SJ

[...]