Re: [RFC PATCH v3 4/9] mm/damon/ops-common: use probe-weighted score when probe weights are set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mm/damon: integer overflow and flawed signed cast in hotness calculation inverts scores
- [Medium] mm/damon: unwarranted +1 divisor guard distorts probe-based frequency subscore
--

commit 789313bd7b3378340e31414deb770dbabf9ae379
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon/ops-common: use probe-weighted score when probe weights are set

This commit updates damon_hot_score() to use probe-weighted hardware
event counts for the frequency subscore when probe weights are configured.
It also exports damon_has_probe_weights() and attempts to guard against
integer overflow when calculating the new subscore.

> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c

[ ... ]

> @@ -172,9 +172,24 @@ int damon_hot_score(struct damon_ctx *c, struct damon_region *r,
>  	unsigned int age_weight = s->quota.weight_age;
>  	int hotness;
>  
> -	freq_subscore = mult_frac(damon_nr_accesses_mvsum(r, c),
> -			DAMON_MAX_SUBSCORE,
> -			damon_nr_samples_per_aggr(&c->attrs));
> +	if (damon_has_probe_weights(c)) {
> +		unsigned int wsum = damon_probe_hits_wsum(r, false, true, c);
> +
> +		/*
> +		 * Route perf-event hardware event counts into the score.
> +		 * Clamp to DAMON_MAX_SUBSCORE so a large weighted-hit sum
> +		 * cannot overflow the subscore range.
> +		 */
> +		freq_subscore = min_t(int,
> +				mult_frac(wsum, DAMON_MAX_SUBSCORE,

[Severity: High]
Could this combination of mult_frac and min_t cause highly active regions
to be scored as completely cold? 

Since wsum is an unsigned int accumulating raw hardware events from perf
probes, it can reach very large values. The mult_frac macro uses 32-bit
unsigned math for its internal scaling. If the intermediate result of the
scaling exceeds the 32-bit maximum, it will silently wrap around.

Furthermore, min_t forces this unsigned result into a signed 32-bit integer.
If the wrapped intermediate result has the sign bit set, the signed cast
produces a large negative number. Since this negative value is less than
DAMON_MAX_SUBSCORE, min_t will incorrectly return it instead of clamping.
This negative freq_subscore could then corrupt the subsequent hotness
calculation, paradoxically scoring highly active regions as zero.

> +					/* +1 guards divide-by-zero: samples-per-aggr can be 0 */
> +					damon_nr_samples_per_aggr(&c->attrs) + 1),

[Severity: Medium]
Is this +1 divisor guard necessary here? The comment mentions that
samples-per-aggr can be 0, but looking at damon_nr_samples_per_aggr(),
it appears explicitly designed to never return 0:

include/linux/damon.h:damon_nr_samples_per_aggr() {
        ...
        return nr_samples ? : 1;
}

Because the function already guards against zero, adding 1 introduces an
unwarranted off-by-one division error. For example, it could change a
divisor of 1 to 2, halving the score. Could this permanently and
disproportionately dampen the frequency subscores of probed regions
compared to unprobed regions in the else block, mathematically distorting
the tiering decisions?

> +				DAMON_MAX_SUBSCORE);
> +	} else {
> +		freq_subscore = mult_frac(damon_nr_accesses_mvsum(r, c),
> +				DAMON_MAX_SUBSCORE,
> +				damon_nr_samples_per_aggr(&c->attrs));
> +	}
>  
>  	age_in_sec = div_u64((u64)r->age * c->attrs.aggr_interval,
>  			     USEC_PER_SEC);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-damon-perf-rfc-v3-send-2026-10-03-v3-0-0f00417b41bc@gmail.com?part=4