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

[SeongJae Park <sj@kernel.org>]

On Wed, 29 Apr 2026 04:39:02 +0000 sashiko-bot@kernel.org wrote:

> 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.

Both are not a blocker of this patch in my opinion.  Please read below for
details.

> --
> 
> 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.

Yes, such corner case could exist.  But, similar overflow-caused initialization
could happen even before this patch, if the biggest System RAM's end address is
same to the U32T_MAX.  And even in the corner case, failing initialization
shouldn't cause a real problem.  The user could manually set the monitoring
regions and work.

So, not a blocker of this patch.

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

As also answered to the same previous questions, DAMON's adaptive regions
adjustment mechanism should solve this accuracy issue.

So, not a blocker for this patch.

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

When the number of such distinct areas is high, because distict regions cannot
be merged, it will degrade the accuracy.  So, no.  Like vaddr, adding a finite
number of biggest gaps might make sense.  But, it wouldn't be late to revist
after such problematic real use case is found.


Thanks,
SJ

[...]