Re: [PATCH 7.2.y] mm/damon/ops-common: use nr_accesses moving sum for quota score

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The commit message claims to fix the issue using a specific helper function (`damon_nr_accesses_mvsum()`), but the patch open-codes the calculation instead.

Pre-existing issues:
- [High] Switching to `nr_accesses_bp` exposes a pre-existing 32-bit integer overflow when calculating access scores for large `max_nr_accesses`.
--

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

mm/damon/ops-common: use nr_accesses moving sum for quota score

This commit changes the quota prioritization score calculation to use the
moving sum of access counts instead of the fully aggregated count. This
aims to improve DAMOS performance when schemes are applied at any time.

Link: https://lore.kernel.org/20260719161136.90191-1-sj@kernel.org

>     Since commit 42f994b71404 ("mm/damon/core: implement scheme-specific apply
>     interval"), DAMOS scheme can be applied at any time.  At that time,
>     nr_accesses may not be fully aggregated.  But the quota prioritization
>     score is calculated using the not fully aggregated count.  As a result,
>     the performance of DAMOS could be degraded.  Fix by using
>     damon_nr_accesses_mvsum() instead.
      ^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
The commit message claims to fix the issue using a specific helper function
(damon_nr_accesses_mvsum()), but the patch appears to open-code the
calculation instead. Does the patch mismatch the commit message?

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 0bcad6b1e5b9e..6a969b1d29871 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -111,8 +111,9 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
>  	unsigned int age_weight = s->quota.weight_age;
>  	int hotness;
>  
> -	freq_subscore = r->nr_accesses * DAMON_MAX_SUBSCORE /
> -		damon_max_nr_accesses(&c->attrs);
> +	freq_subscore = mult_frac(r->nr_accesses_bp / 10000,
> +			DAMON_MAX_SUBSCORE,
> +			damon_max_nr_accesses(&c->attrs));

[Severity: High]
This is a pre-existing issue, but does switching to nr_accesses_bp newly
expose the hotness score calculation to an upstream 32-bit integer overflow
bug when max_nr_accesses is large?

The overflow occurs in mm/damon/core.c:damon_update_region_access_rate()
where r->last_nr_accesses is multiplied by 10000:

    r->nr_accesses_bp = damon_moving_sum(r->nr_accesses_bp,
			r->last_nr_accesses * 10000, len_window,
			accessed ? 10000 : 0);

If a user configures a large ratio of aggr_interval to sample_interval
(e.g., yielding max_nr_accesses > 429,496), r->last_nr_accesses * 10000
will overflow 32-bit unsigned arithmetic because both operands are 32-bit.
The corrupted value is then stored in nr_accesses_bp.

By switching from r->nr_accesses to r->nr_accesses_bp / 10000 here, this
overflowed value is now used to calculate the hotness score, potentially
leading to artificially low scores for heavily accessed memory regions.

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