Re: [RFC PATCH v2 1/4] mm/damon/core: add DAMOS_FILTER_TYPE_AGE_MS

[SJ Park <sj@kernel.org>]

On Thu, 05 Nov 2026 04:55:17 +0000 sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] mm/damon: 64-to-32-bit truncation of age_ms in damos_filter_match()
> --
> 
> commit 2459402d4dc794f740b6a5720a69fe4a1a61ae62
> Author: SJ Park <sj@kernel.org>
> 
> mm/damon/core: add DAMOS_FILTER_TYPE_AGE_MS
> 
> This commit introduces DAMOS_FILTER_TYPE_AGE_MS in the DAMON API and core
> layer to allow filtering DAMOS target regions by age in milliseconds. It
> converts the region age from aggregation intervals to milliseconds using
> div_u64() and compares the result against damos_filter->range_{min,max}.
> 
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index d36823f4d0928..0a056949604f9 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> [ ... ]
> > @@ -2552,7 +2554,7 @@ static bool damos_filter_match(struct damon_ctx *ctx, struct damon_target *t,
> >  	bool matched = false;
> >  	struct damon_target *ti;
> >  	int target_idx = 0;
> > -	unsigned long start, end, wsum;
> > +	unsigned long start, end, wsum, age_ms;
> >  
> >  	*err = 0;
> >  	switch (filter->type) {
> > @@ -2594,6 +2596,12 @@ static bool damos_filter_match(struct damon_ctx *ctx, struct damon_target *t,
> >  		matched = filter->range_min <= wsum &&
> >  			wsum <= filter->range_max;
> >  		break;
> > +	case DAMOS_FILTER_TYPE_AGE_MS:
> > +		age_ms = div_u64((u64)r->age * ctx->attrs.aggr_interval,
> > +				USEC_PER_MSEC);
> > +		matched = filter->range_min <= age_ms &&
> > +			age_ms <= filter->range_max;
> > +		break;
> 
> [Severity: Medium]
> In damos_filter_match(), age_ms is declared as unsigned long. On 32-bit
> architectures, unsigned long is 32 bits wide, so storing the u64 return value
> of div_u64() into age_ms truncates the upper 32 bits whenever the region age
> in milliseconds exceeds ULONG_MAX (about 49.7 days, or for example
> r->age >= 42,949,673 at the default 100,000 us aggr_interval, and
> proportionally earlier when aggr_interval is larger).
> 
> When that happens during kdamond_apply_schemes() -> damon_do_apply_schemes()
> -> damos_apply_scheme() -> damos_filter_out() -> damos_core_filter_out() ->
> damos_filter_match(), age_ms wraps modulo 2^32 and can misclassify old
> regions as young regions or fail to match old-region filters.
> 
> By comparison, damon_hot_score() in mm/damon/ops-common.c stores the result
> of div_u64() in a u64 variable (as fixed in commit 0284dff76e87 ("mm/damon/
> ops-common: fix 64-bit division in damon_hot_score()")):
> 
> mm/damon/ops-common.c:damon_hot_score() {
> 	...
> 	u64 age_in_sec;
> 	...
> 	age_in_sec = div_u64((u64)r->age * c->attrs.aggr_interval,
> 			     USEC_PER_SEC);
> 	...
> }
> 
> Should age_ms be declared as u64 (or saturated to ULONG_MAX before storing into
> age_ms so filters with range_max == ULONG_MAX on 32-bit continue to match) to
> avoid truncating the quotient on 32-bit systems?

The overflow could make DAMOS behave little bit suboptimum for certain cases.
I don't think that will be measurable.  Regions of age longer than 49.7 days
would be rare in my opinion.  Also, DAMOS resets age of regions to zero when an
action is applied.  Hence I think 49.7 days is practically long enough.  I'd
keep this as is for simplicity for now, and revisit if other human raises
concerns or a real issue from the possible overflow is found.

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


Thanks,
SJ