Re: [RFC PATCH v1.1 03/13] mm/damon/core: use damon_nr_accesses_mvsum() in __damos_valid_target()

[SeongJae Park <sj@kernel.org>]

On Sat, 20 Jun 2026 17:41:36 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The new __damos_valid_target implementation uses damon_nr_accesses_mvsum(), which is vulnerable to time-of-check state desynchronization if ctx->next_aggregation_sis is updated asynchronously via kdamond_call(). This causes massive over-estimation of the target access frequency.
> --
> 
> commit 2274522ec6ea648db508697b0a1f57a8bf4e864d
> Author: SeongJae Park <sj@kernel.org>
> Subject: mm/damon/core: use damon_nr_accesses_mvsum() in __damos_valid_target()
> 
> This commit replaces the direct read of r->nr_accesses_bp with a call
> to damon_nr_accesses_mvsum() inside __damos_valid_target(). The author
> notes that while execution is slightly more expensive, the function is
> simpler and more tolerant to errors.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index af95783d3402a..ee4f0f0e583bd 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -2120,10 +2120,11 @@ static void kdamond_tune_intervals(struct damon_ctx *c)
> >  	damon_set_attrs(c, &new_attrs);
> >  }
> >  
> > -static bool __damos_valid_target(struct damon_region *r, struct damos *s)
> > +static bool __damos_valid_target(struct damon_region *r, struct damos *s,
> > +		struct damon_ctx *c)
> >  {
> >  	unsigned long sz;
> > -	unsigned int nr_accesses = r->nr_accesses_bp / 10000;
> > +	unsigned int nr_accesses = damon_nr_accesses_mvsum(r, c);
> 
> [Severity: High]
> Could this lead to double-counting access scores if the context attributes
> are updated asynchronously?
> 
> In kdamond_fn(), when an aggregation interval finishes, pending API requests
> are processed via kdamond_call(ctx, false) right before
> kdamond_apply_schemes() is called. If a processed request triggers
> damon_set_attrs() (e.g., via sysfs), ctx->next_aggregation_sis is eagerly
> advanced to the end of the newly configured interval.
> 
> When __damos_valid_target() subsequently calls damon_nr_accesses_mvsum(),
> it calculates the time window based on ctx->next_aggregation_sis:
> 
> mm/damon/core.c:damon_nr_accesses_mvsum() {
>     left_window = ctx->next_aggregation_sis - ctx->passed_sample_intervals;
>     left_window_bp = mult_frac(left_window, 10000, window_len);
>     return damon_mvsum(r->nr_accesses, r->last_nr_accesses, left_window_bp);
> }
> 
> Because ctx->next_aggregation_sis was just advanced, left_window equals the
> full new window length, resulting in a left_window_bp of 10000.
> 
> Since damon_mvsum() assumes it is at the start of the window, it will add
> the unscaled r->last_nr_accesses to the current r->nr_accesses. Because the
> aggregation interval just finished, r->nr_accesses already holds the
> fully-accumulated access count. Adding last_nr_accesses effectively doubles
> the access score and could misclassify regions during target validation.
> 
> The pre-patch code avoided this by safely reading the decoupled
> r->nr_accesses_bp, which preserved the correct scaled value across
> kdamond_call() boundary transitions.

Nice finding.  This should be a minor impact that persists for only one
aggregation interval, but I think the issue is bettter to not introduced.  I
will scale r->last_nr_accesses together to avoid this issue in the next
revision.


Thanks,
SJ

[...]