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

[sashiko-bot@kernel.org]

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

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.

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

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

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?

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