Re: [RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Incomplete refactoring: the parameter name `nr_accesses` was not updated to reflect the new generic purpose.

Pre-existing issues:
- [High] 32-bit arithmetic overflow in `damon_sample_bp_to_count` and `damon_sample_count_to_bp` corrupts interval conversions for large aggregation windows.
--

--- Patch [5]: [RFC PATCH v1.1 5/7] mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/ ---
commit 17e2132821e6ed9a2213f8cf4fa6bdeba54db3b6
Author: SJ Park <sj@kernel.org>

mm/damon/core: s/nr_accesses_for_new_attrs/nr_samples_for_new_attrs/

damon_nr_accesses_for_new_attrs() can be used for not only nr_accesses
but also any positive sample count, like probe_hits.  Rename to be able
to be used for such general uses without confusion.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 718268df27fa7..cda318806b65b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -890,7 +890,7 @@ static unsigned int damon_sample_count_to_bp(
>  	return mult_frac(count, 10000, damon_nr_samples_per_aggr(attrs));
>  }
>  

[Severity: High]
This is a pre-existing issue, but does this calculation silently overflow for
large aggregation intervals?

When users configure large aggregation intervals (or very small sample
intervals) such that samples_per_aggr exceeds 429,496, calculating the access
frequency ratio overflows 32-bit math.

The mult_frac() macro calculates (count % samples) * 10000 using the 32-bit
type of count. If samples_per_aggr > 429,496, the remainder can be large
enough that multiplying by 10000 causes an overflow.

A similar 32-bit arithmetic overflow can happen in damon_sample_bp_to_count():

    return bp * damon_nr_samples_per_aggr(attrs) / 10000;

> -static unsigned int damon_nr_accesses_for_new_attrs(unsigned int nr_accesses,
> +static unsigned int damon_nr_samples_for_new_attrs(unsigned int nr_accesses,
>  		struct damon_attrs *old_attrs, struct damon_attrs *new_attrs)
>  {

[Severity: Low]
Is the nr_accesses parameter name still appropriate here?

Since the function is being renamed to be used for general sample counts
(like probe_hits) without confusion, leaving the first parameter named
nr_accesses creates a semantic mismatch that limits the readability benefits
of this refactoring.

>  	return damon_sample_bp_to_count(

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