Re: [PATCH v2 1/4] mm/damon/ops-common: optimize damon_hot_score() using ilog2()

[SeongJae Park <sj@kernel.org>]

On Sun, 26 Apr 2026 23:28:10 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 3 potential issue(s):
> - [High] Arithmetic overflow on 32-bit architectures during `age_in_sec` calculation.
> - [Low] The logic using `ilog2()` with a zero-check branch can be simplified by directly using `fls()`.
> - [Low] Unnecessary double blank line introduced.
> --
> 
> commit c096ebdf3dd6d7a6e783e2ac65dec1740282929f
> Author: Liew Rui Yan <aethernet65535@gmail.com>
> 
>     mm/damon/ops-common: optimize damon_hot_score() using ilog2()
> 
> This commit replaces the manual for-loop used to calculate age_in_log
> in damon_hot_score() with the ilog2() function. This optimization aims to
> significantly reduce the latency of the hotness score calculation.
> 
> > diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> > index 8c6d613425c1..3a0ddc3ac719 100644
> > --- a/mm/damon/ops-common.c
> > +++ b/mm/damon/ops-common.c
> > @@ -117,9 +117,12 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
> >  		damon_max_nr_accesses(&c->attrs);
> >  
> >  	age_in_sec = (unsigned long)r->age * c->attrs.aggr_interval / 1000000;
> 
> This isn't a bug in your patch, but does the multiplication of
> (unsigned long)r->age * c->attrs.aggr_interval overflow on 32-bit
> architectures?
> 
> Since both variables evaluate as 32-bit types on 32-bit architectures,
> if aggr_interval is 1,000,000 (1 second), the multiplication will wrap
> around when r->age exceeds 4294. This wrap-around could result in a very
> small age_in_sec value for extremely cold regions, causing them to be
> falsely evaluated as young and hot.
> 
> Should this calculation be cast to a 64-bit integer prior to multiplication,
> perhaps using div_u64()?

Good point, but as Sashiko also notes, not a blocker of this patch.  I will add
a todo item for thinking more on this.

> 
> > -	for (age_in_log = 0; age_in_log < DAMON_MAX_AGE_IN_LOG && age_in_sec;
> > -			age_in_log++, age_in_sec >>= 1)
> > -		;
> > +	if (age_in_sec)
> > +		age_in_log = min_t(int, ilog2(age_in_sec) + 1,
> > +				DAMON_MAX_AGE_IN_LOG);
> > +	else
> > +		age_in_log = 0;
> 
> Instead of using ilog2() with a zero-check branch, could this be simplified
> by directly using fls()?
> 
> The fls() function safely returns 0 when the value is 0 and correctly
> calculates the 1-based log2 value for positive numbers, allowing this
> branch to be removed entirely while preserving the exact mathematical
> result.

We believe ilog2() is easier to read.

> 
> > +
> >  
> >  	/* If frequency is 0, higher age means it's colder */
> 
> This isn't a bug, but an extra blank line appears to be introduced here,
> creating two consecutive blank lines.

Good cactch.  As this is a quite minor issue, I will ask Andrew to fix this.

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


Thanks,
SJ