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

[sashiko-bot@kernel.org]

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.

>  	sz = damon_sz_region(r);
>  	return s->pattern.min_sz_region <= sz &&

[ ... ]

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