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

[SJ Park <sj@kernel.org>]

On Tue, 07 Jul 2026 14:05:15 +0000 sashiko-bot@kernel.org wrote:

> 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;

Known low priority issue.

> 
> > -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.

Good catch.  I will fix in the next revision.

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


Thanks,
SJ