Re: [PATCH] mm/damon: add a min_sz_region parameter to damon_set_region_biggest_system_ram_default()

[SeongJae Park <sj@kernel.org>]

On Fri, 17 Oct 2025 11:12:00 +0800 Quanmin Yan <yanquanmin1@huawei.com> wrote:

> Hi SJ,
> 
> 在 2025/10/17 3:48, SeongJae Park 写道:
> > On Thu, 16 Oct 2025 18:47:17 +0800 Quanmin Yan <yanquanmin1@huawei.com> wrote:
> >
> >> After adding addr_unit support for DAMON_LRU_SORT and DAMON_RECLAIM,
> >> the related region setup now requires alignment based on min_sz_region.
> >>
> >> Add min_sz_region to damon_set_region_biggest_system_ram_default()
> >> and use it when calling damon_set_regions(), replacing the previously
> >> hardcoded DAMON_MIN_REGION.
> > Can we add more detailed description of the end user issue on the commit
> > message?  My understanding of the issue is that the monitoring target address
> > ranges for DAMON_LRU_SORT and DAMON_RECLAIM would be aligned on
> > DAMON_MIN_REGION * addr_unit.
> >
> > For example, if user sets the monitoring target address range as [4, 8) and
> > addr_unit as 1024, the aimed monitoring target address range is [4 KiB, 8 KiB).
> > But damon_set_regions() will apply DAMON_MIN_REGION as the core address
> > alignment.  Assuming DAMON_MIN_REGION is 4096, so resulting target address
> > range will be [0, 4096) in the DAMON core layer address system, and [0, 4 MiB)
> > in the physical address space.
> >
> > So the end user effect is that DAMON_LRU_SORT and DAMON_RECLAIM could work for
> > unexpectedly large physical address ranges, when they 1) set addr_unit to a
> > value larger than 1, and 2) set the monitoring target address range as not
> > aligned in 4096*addr_unit.
> >
> > Let me know if I'm misunderstanding something.
> >
> > Also, if you encountered the issue in a real or a realistic use case, adding
> > that on the commit message together would be very helpful.
> 
> Thank you for the additional explanation! Your understanding and description of
> the issue are entirely correct.
> 
> Our ultimate goal is to have the monitoring target address range aligned to
> DAMON_MIN_REGION. In the original logic, however, damon_set_regions() did not
> take the newly introduced addr_unit into account, and directly performed core
> address alignment based only on DAMON_MIN_REGION. As a result, the monitoring
> target address range was aligned to DAMON_MIN_REGION * addr_unit, causing
> DAMON_LRU_SORT and DAMON_RECLAIM to potentially operate on incorrect physical
> address ranges. Therefore, we need to use min_sz_region instead of
> DAMON_MIN_REGION in damon_set_regions().

Thank you for confirming!

> 
> I will add the detailed commit description in the v2 patch.

Looking forward to the v2!

> 
> >> Fixes: 2e0fe9245d6b ("mm/damon/lru_sort: support addr_unit for DAMON_LRU_SORT")
> >> Fixes: 7db551fcfb2a ("mm/damon/reclaim: support addr_unit for DAMON_RECLAIM")
> > Let's break this patch into two patches, so that we have one fix per broken
> > commit.
> 
> Yes, I actually considered splitting it up before submitting this patch, but found that
> doing so might make the patches look odd. Since we're modifying the input parameters
> of damon_set_region_biggest_system_ram_default(), all the call sites of this function
> need to be updated accordingly. It seems we might need to split this into three patches:
> 1. Preparation patch: Add the min_sz_region parameter to
>     damon_set_region_biggest_system_ram_default(), passing ctx->min_sz_region in stat.c,
>     and passing DAMON_MIN_REGION when calling this function in reclaim.c/lru_sort.c?
> 2. Fixes patch 1: Modify lru_sort.c to pass param_ctx->min_sz_region.
> 3. Fixes patch 2: Modify reclaim.c to pass param_ctx->min_sz_region.
> 
> I'm not entirely comfortable with this approach. Would it be acceptable to submit this
> as a single patch?

I think you can merge the first and the second patch into one single patch,
resulting in two patches each fixing the issue on DAMON_LRU_SORT and
DAMON_RECLAIM in the order.  It doesn't look odd to me.


Thanks,
SJ

[...]