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

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 5/21/26 1:22 PM, sashiko-bot@kernel.org wrote:
> 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?



The budget here is not meant to be an internal fairness mechanism;
it exists so that this helper honours the operator's max_nr_regions
configuration.  max_nr_regions is the contract: under it, this
helper tries to use whatever room is left.  It does not try to
re-allocate that room across targets, and DAMON's existing
per-target split/merge loops do not do that either.

If a multi-target deployment is hitting the budget cap with
unsatisfied age==0 candidates left over, that is information about
the configuration -- max_nr_regions is undersized for the workload
the operator wants to observe -- rather than something the helper
should silently work around with a fairness policy.

Just like I mention in cover letter, we actually need expose some
metrics for observability like nr_regions


>> +			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?


Well, same style as damon_split_regions_of() and we use 'if (sz_sub == 0 
|| sz_sub >= sz_region)' to protect it.

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