Re: [RFC PATCH 3/7] mm/damon/lru_sort: cover all system rams

[SeongJae Park <sj@kernel.org>]

On Wed, 15 Apr 2026 02:36:28 +0000 sashiko-bot@kernel.org wrote:

> Sashiko AI review found 3 potential issue(s):
> - [High] Monitoring the bounding box of all System RAM resources encompasses massive physical address holes, fundamentally breaking DAMON's sampling-based access check on sparse memory systems.
> - [High] Applying schemes to the newly-enlarged bounding box can cause a soft lockup panic due to unbounded page iteration without yielding.
> - [Low] Misspelled 'phyiscal' in module parameter documentation.

Other than the typo, no real issues in my opinion.  Read below for more
detailed answers.

> --
> 
> commit 11d2e4765abf8999777a27c53c055f1c43ca53bf
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/lru_sort: cover all system rams
> 
> This commit updates DAMON_LRU_SORT to cover all system RAM instead of
> just the biggest System RAM resource. The default monitor region start
> and end parameter descriptions and initializations are modified to
> reflect the entire physical memory space.
> 
> > diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> > index 554559d729760..7f8c26890ffb2 100644
> > --- a/mm/damon/lru_sort.c
> > +++ b/mm/damon/lru_sort.c
> > @@ -140,7 +140,8 @@ DEFINE_DAMON_MODULES_MON_ATTRS_PARAMS(damon_lru_sort_mon_attrs);
> >   * Start of the target memory region in physical address.
> >   *
> >   * The start physical address of memory region that DAMON_LRU_SORT will do work
> > - * against.  By default, biggest System RAM is used as the region.
> > + * against.  By default, the system's entire phyiscal memory is used as the
> 
> This isn't a bug, but there is a typo (phyiscal) here.

Nice catch, I will fix this in the next revision.

> 
> > + * region.
> >   */
> >  static unsigned long monitor_region_start __read_mostly;
> >  module_param(monitor_region_start, ulong, 0600);
> > @@ -149,7 +150,8 @@ module_param(monitor_region_start, ulong, 0600);
> >   * End of the target memory region in physical address.
> >   *
> >   * The end physical address of memory region that DAMON_LRU_SORT will do work
> > - * against.  By default, biggest System RAM is used as the region.
> > + * against.  By default, the system's entire phyiscal memory is used as the
> 
> There is another typo (phyiscal) here as well.

I will fix this in the next revision.

> 
> > + * region.
> >   */
> >  static unsigned long monitor_region_end __read_mostly;
> >  module_param(monitor_region_end, ulong, 0600);
> 
> [ ... ]
> 
> > @@ -336,7 +338,7 @@ static int damon_lru_sort_apply_parameters(void)
> >  	if (err)
> >  		goto out;
> >  
> > -	err = damon_set_region_biggest_system_ram_default(param_target,
> > +	err = damon_set_region_system_rams_default(param_target,
> >  					&monitor_region_start,
> >  					&monitor_region_end,
> >  					param_ctx->addr_unit,
> 
> Will monitoring the bounding box of all System RAM resources encompass massive
> physical address holes on sparse memory systems?
> 
> Because DAMON estimates access frequency by sampling uniformly at random from
> the region:
> 
> r->sampling_addr = damon_rand(r->ar.start, r->ar.end);
> 
> If the bounding box spans across terabytes of address gaps between NUMA nodes,
> would the overwhelming majority of samples hit unpopulated holes?
> 
> Since sampling a hole returns no folio, does this artificially dilute the
> apparent access frequency of valid hot RAM within the region and misidentify
> it as cold?

Adaptive regions adjustment should avoid this issue.

> 
> Can this also cause soft lockups during scheme application?
> 
> When applying a scheme, the region is iterated page-by-page. For unpopulated
> gaps, the folio check is skipped and the address is incremented:
> 
> while (addr < damon_pa_phys_addr(r->ar.end, addr_unit)) {
>     folio = damon_get_folio(PHYS_PFN(addr));
>     if (damon_pa_invalid_damos_folio(folio, s)) {
>         addr += PAGE_SIZE;
>         continue;
>     }
>     ...
> 
> Since there is no cond_resched() inside this tight loop, and because valid
> folios are not found (meaning quota variables don't increment to abort early),
> could iterating over terabytes of unpopulated gaps trigger the soft lockup
> watchdog on PREEMPT_NONE or PREEMPT_VOLUNTARY kernels?

As I replied to the previous patch review, though it is theoretically possible,
I don't think such cases are common.  Also if it is really such a corner cases,
users can manually set the monitoring regions.

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


Thanks,
SJ