Re: [PATCH 1/7] mm/damon: introduce damon_set_region_system_rams_default()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The default region setup creates a single contiguous bounding box encompassing all System RAM resources, incorrectly including unpopulated memory holes.
- [High] Integer overflow during physical address boundary calculation (`arg.res.end + 1`) can cause DAMON initialization to fail.
--

commit cd855b0d7d334ccf9a13e1331105a3281e5b0aeb
Author: SeongJae Park <sj@kernel.org>

mm/damon: introduce damon_set_region_system_rams_default()

Introduces a default target setup for DAMON that sets a physical address
range covering all System RAM resources when the caller doesn't specify
a region.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 05e4bef367db..980a31cd3498 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[ ... ]
> +static bool damon_find_system_rams_range(unsigned long *start,
> +		unsigned long *end, unsigned long addr_unit)
> +{
> +	struct damon_system_ram_range_walk_arg arg = {};
> +
> +	walk_system_ram_res(0, -1, &arg, damon_system_ram_walk_fn);
> +	if (!arg.walked)
> +		return false;
> +	*start = damon_res_to_core_addr(arg.res.start, addr_unit);
> +	*end = damon_res_to_core_addr(arg.res.end + 1, addr_unit);

Can arg.res.end + 1 overflow here?

If a 32-bit system has physical memory extending to the end of the address
space, arg.res.end will be the maximum representable value. Adding 1 would
overflow to 0, making *end 0. The subsequent check if (*end <= *start)
would then evaluate to true, causing the function to silently return false
and abort initialization with -EINVAL.

> +	if (*end <= *start)
> +		return false;
> +	return true;
> +}
[ ... ]
> +int damon_set_region_system_rams_default(struct damon_target *t,
> +			unsigned long *start, unsigned long *end,
> +			unsigned long addr_unit, unsigned long min_region_sz)
> +{
> +	struct damon_addr_range addr_range;
> +
> +	if (*start > *end)
> +		return -EINVAL;
> +
> +	if (!*start && !*end &&
> +		!damon_find_system_rams_range(start, end, addr_unit))
> +		return -EINVAL;
> +
> +	addr_range.start = *start;
> +	addr_range.end = *end;
> +	return damon_set_regions(t, &addr_range, 1, min_region_sz);

Does this create a single bounding box encompassing all System RAM resources,
including unpopulated memory holes?

On systems with sparse physical memory layouts, this combined range will
encompass massive memory holes. Because DAMON uses uniform random sampling
within a region to estimate access frequencies, samples landing in the gaps
will inherently register as unaccessed, diluting the perceived access rates
and preventing effective adaptive region splitting and merging.

Could the implementation instead construct an array of multiple ranges,
one for each distinct System RAM resource, and pass them to
damon_set_regions()?

> +}

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