Re: [PATCH 1/2] mm/damon/core: split age==0 regions when nr_regions exceeds max/2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The `damon_split_zero_age_regions` function processes targets and regions sequentially, potentially starving later targets if the budget is exhausted early.
- [Medium] The calculation of `sz_sub` can overflow on 32-bit architectures for large regions, resulting in incorrect sub-region sizes or failing to split.
--

commit 8c8f86e64dc7143fb89435db427efa2adca6f33c
Author: Jiayuan Chen <jiayuan.chen@shopee.com>

mm/damon/core: split age==0 regions when nr_regions exceeds max/2

This commit modifies the region splitting logic to handle scenarios where
the region count exceeds half the maximum limit. Instead of returning early,
it now splits regions whose access pattern has just changed (age == 0) up
to the remaining budget, which helps discover internal access variations in
large regions.

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

[ ... ]

> +static void damon_split_zero_age_regions(struct damon_ctx *ctx,
> +					 unsigned long budget)
> +{
> +	struct damon_target *t;
> +	struct damon_region *r, *next;
> +
> +	damon_for_each_target(t, ctx) {
> +		damon_for_each_region_safe(r, next, t) {
> +			unsigned long sz_region, sz_sub;
> +
> +			if (!budget)
> +				return;

[Severity: Medium]
Will this sequential processing potentially starve later targets if the
budget is exhausted early?

Since budget is a context-wide limit shared across all targets, a highly
active first target with many zero-age regions could consume the entire
available split budget. 

This might permanently prevent subsequent targets in the same context from
refining their monitoring resolution under dynamically changing access
patterns. Could a round-robin or proportionate distribution be used here
instead to ensure uniform monitoring fidelity?

> +			if (r->age != 0)
> +				continue;
> +			sz_region = damon_sz_region(r);
> +			if (sz_region < 2 * ctx->min_region_sz)
> +				continue;
> +
> +			sz_sub = ALIGN_DOWN(damon_rand(ctx, 1, 10) *
> +					sz_region / 10, ctx->min_region_sz);

[Severity: Medium]
Can the calculation for sz_sub overflow on 32-bit architectures for large
regions?

Since unsigned long is 32 bits on those architectures, if sz_region is
large (e.g., > 477 MB), multiplying it by a random value up to 9 could
wrap around ULONG_MAX. 

This overflow would result in drastically smaller sub-region sizes. If the
wrapped product divided by 10 is smaller than min_region_sz, ALIGN_DOWN
evaluates to 0. This would trigger the blank region check below and skip
the split entirely. 

Should the division occur before the multiplication to prevent this?

> +			/* Do not allow blank region */
> +			if (sz_sub == 0 || sz_sub >= sz_region)
> +				continue;
> +
> +			damon_split_region_at(t, r, sz_sub);
> +			budget--;
> +		}
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521045236.115749-1-jiayuan.chen@linux.dev?part=1