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

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

[Quanmin Yan]

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.

Fixes: 2e0fe9245d6b ("mm/damon/lru_sort: support addr_unit for DAMON_LRU_SORT")
Fixes: 7db551fcfb2a ("mm/damon/reclaim: support addr_unit for DAMON_RECLAIM")
Signed-off-by: Quanmin Yan <yanquanmin1@huawei.com>
---
 include/linux/damon.h | 3 ++-
 mm/damon/core.c       | 6 ++++--
 mm/damon/lru_sort.c   | 3 ++-
 mm/damon/reclaim.c    | 3 ++-
 mm/damon/stat.c       | 3 ++-
 5 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/include/linux/damon.h b/include/linux/damon.h
index cae8c613c5fc..1ce75a20febf 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -947,7 +947,8 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control);
 int damos_walk(struct damon_ctx *ctx, struct damos_walk_control *control);
 
 int damon_set_region_biggest_system_ram_default(struct damon_target *t,
-				unsigned long *start, unsigned long *end);
+				unsigned long *start, unsigned long *end,
+				unsigned long min_sz_region);
 
 #endif	/* CONFIG_DAMON */
 
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 93848b4c6944..e29f08b8a114 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -2767,6 +2767,7 @@ static bool damon_find_biggest_system_ram(unsigned long *start,
  * @t:		The monitoring target to set the region.
  * @start:	The pointer to the start address of the region.
  * @end:	The pointer to the end address of the region.
+ * @min_sz_region:	Minimum region size.
  *
  * This function sets the region of @t as requested by @start and @end.  If the
  * values of @start and @end are zero, however, this function finds the biggest
@@ -2777,7 +2778,8 @@ static bool damon_find_biggest_system_ram(unsigned long *start,
  * Return: 0 on success, negative error code otherwise.
  */
 int damon_set_region_biggest_system_ram_default(struct damon_target *t,
-			unsigned long *start, unsigned long *end)
+			unsigned long *start, unsigned long *end,
+			unsigned long min_sz_region)
 {
 	struct damon_addr_range addr_range;
 
@@ -2790,7 +2792,7 @@ int damon_set_region_biggest_system_ram_default(struct damon_target *t,
 
 	addr_range.start = *start;
 	addr_range.end = *end;
-	return damon_set_regions(t, &addr_range, 1, DAMON_MIN_REGION);
+	return damon_set_regions(t, &addr_range, 1, min_sz_region);
 }
 
 /*
diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
index 42b9a656f9de..49b4bc294f4e 100644
--- a/mm/damon/lru_sort.c
+++ b/mm/damon/lru_sort.c
@@ -242,7 +242,8 @@ static int damon_lru_sort_apply_parameters(void)
 
 	err = damon_set_region_biggest_system_ram_default(param_target,
 					&monitor_region_start,
-					&monitor_region_end);
+					&monitor_region_end,
+					param_ctx->min_sz_region);
 	if (err)
 		goto out;
 	err = damon_commit_ctx(ctx, param_ctx);
diff --git a/mm/damon/reclaim.c b/mm/damon/reclaim.c
index 7ba3d0f9a19a..36a582e09eae 100644
--- a/mm/damon/reclaim.c
+++ b/mm/damon/reclaim.c
@@ -250,7 +250,8 @@ static int damon_reclaim_apply_parameters(void)
 
 	err = damon_set_region_biggest_system_ram_default(param_target,
 					&monitor_region_start,
-					&monitor_region_end);
+					&monitor_region_end,
+					param_ctx->min_sz_region);
 	if (err)
 		goto out;
 	err = damon_commit_ctx(ctx, param_ctx);
diff --git a/mm/damon/stat.c b/mm/damon/stat.c
index d8010968bbed..6c4503d2aee3 100644
--- a/mm/damon/stat.c
+++ b/mm/damon/stat.c
@@ -187,7 +187,8 @@ static struct damon_ctx *damon_stat_build_ctx(void)
 	if (!target)
 		goto free_out;
 	damon_add_target(ctx, target);
-	if (damon_set_region_biggest_system_ram_default(target, &start, &end))
+	if (damon_set_region_biggest_system_ram_default(target, &start, &end,
+							ctx->min_sz_region))
 		goto free_out;
 	return ctx;
 free_out:
-- 
2.43.0

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

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

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

> Signed-off-by: Quanmin Yan <yanquanmin1@huawei.com>
> ---
>  include/linux/damon.h | 3 ++-
>  mm/damon/core.c       | 6 ++++--
>  mm/damon/lru_sort.c   | 3 ++-
>  mm/damon/reclaim.c    | 3 ++-
>  mm/damon/stat.c       | 3 ++-
>  5 files changed, 12 insertions(+), 6 deletions(-)

The code change looks good to me.


Thanks,
SJ

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

[Quanmin Yan]

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

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

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


Thanks,
Quanmin Yan

>> Signed-off-by: Quanmin Yan <yanquanmin1@huawei.com>
>> ---
>>   include/linux/damon.h | 3 ++-
>>   mm/damon/core.c       | 6 ++++--
>>   mm/damon/lru_sort.c   | 3 ++-
>>   mm/damon/reclaim.c    | 3 ++-
>>   mm/damon/stat.c       | 3 ++-
>>   5 files changed, 12 insertions(+), 6 deletions(-)
> The code change looks good to me.
>
>
> Thanks,
> SJ

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

[SeongJae Park]

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

[...]

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

[Andrew Morton]

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.

Why?

Presumably the current code causes some sort of user-visible
misbehavior.  Please include a description of that misbehavior and of
how users might trigger it.

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