[PATCH] mm/damon/tests/core-kunit: extend test scenarios and remove redundancy

[PATCH] mm/damon/tests/core-kunit: extend test scenarios and remove redundancy

[Shu Anzai]

Add some missing test scenarios to cover a wider range of cases. Also,
remove a redundant case in damos_test_commit_quota_goal().

Signed-off-by: Shu Anzai <shu17az@gmail.com>
---
 mm/damon/tests/core-kunit.h | 51 ++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 18 deletions(-)

diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
index f59ae7ee19a0..e9ccc3fb34f9 100644
--- a/mm/damon/tests/core-kunit.h
+++ b/mm/damon/tests/core-kunit.h
@@ -158,6 +158,7 @@ static void damon_test_split_at(struct kunit *test)
 	r->nr_accesses_bp = 420000;
 	r->nr_accesses = 42;
 	r->last_nr_accesses = 15;
+	r->age = 10;
 	damon_add_region(r, t);
 	damon_split_region_at(t, r, 25);
 	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
@@ -170,6 +171,7 @@ static void damon_test_split_at(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, r_new->nr_accesses_bp);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, r_new->nr_accesses);
 	KUNIT_EXPECT_EQ(test, r->last_nr_accesses, r_new->last_nr_accesses);
+	KUNIT_EXPECT_EQ(test, r->age, r_new->age);
 
 	damon_free_target(t);
 }
@@ -190,6 +192,7 @@ static void damon_test_merge_two(struct kunit *test)
 	}
 	r->nr_accesses = 10;
 	r->nr_accesses_bp = 100000;
+	r->age = 9;
 	damon_add_region(r, t);
 	r2 = damon_new_region(100, 300);
 	if (!r2) {
@@ -198,12 +201,15 @@ static void damon_test_merge_two(struct kunit *test)
 	}
 	r2->nr_accesses = 20;
 	r2->nr_accesses_bp = 200000;
+	r2->age = 21;
 	damon_add_region(r2, t);
 
 	damon_merge_two_regions(t, r, r2);
 	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
 	KUNIT_EXPECT_EQ(test, r->ar.end, 300ul);
 	KUNIT_EXPECT_EQ(test, r->nr_accesses, 16u);
+	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, 160000u);
+	KUNIT_EXPECT_EQ(test, r->age, 17u);
 
 	i = 0;
 	damon_for_each_region(r3, t) {
@@ -232,12 +238,12 @@ static void damon_test_merge_regions_of(struct kunit *test)
 {
 	struct damon_target *t;
 	struct damon_region *r;
-	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184};
-	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230};
-	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2};
+	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184, 235, 240};
+	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230, 240, 10235};
+	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2, 5, 5};
 
-	unsigned long saddrs[] = {0, 114, 130, 156, 170};
-	unsigned long eaddrs[] = {112, 130, 156, 170, 230};
+	unsigned long saddrs[] = {0, 114, 130, 156, 170, 235, 240};
+	unsigned long eaddrs[] = {112, 130, 156, 170, 230, 240, 10235};
 	int i;
 
 	t = damon_new_target();
@@ -255,9 +261,9 @@ static void damon_test_merge_regions_of(struct kunit *test)
 	}
 
 	damon_merge_regions_of(t, 9, 9999);
-	/* 0-112, 114-130, 130-156, 156-170 */
-	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
-	for (i = 0; i < 5; i++) {
+	/* 0-112, 114-130, 130-156, 156-170, 170-230, 235-240, 240-10235 */
+	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 7u);
+	for (i = 0; i < 7; i++) {
 		r = __nth_region_of(t, i);
 		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
 		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
@@ -269,6 +275,9 @@ static void damon_test_split_regions_of(struct kunit *test)
 {
 	struct damon_target *t;
 	struct damon_region *r;
+	unsigned long sa[] = {0, 300, 500};
+	unsigned long ea[] = {220, 400, 700};
+	int i;
 
 	t = damon_new_target();
 	if (!t)
@@ -286,14 +295,19 @@ static void damon_test_split_regions_of(struct kunit *test)
 	t = damon_new_target();
 	if (!t)
 		kunit_skip(test, "second target alloc fail");
-	r = damon_new_region(0, 220);
-	if (!r) {
-		damon_free_target(t);
-		kunit_skip(test, "second region alloc fail");
+	for (i = 0; i < ARRAY_SIZE(sa); i++) {
+		r = damon_new_region(sa[i], ea[i]);
+		if (!r) {
+			damon_free_target(t);
+			kunit_skip(test, "region alloc fail");
+		}
+		damon_add_region(r, t);
+	}
+	damon_split_regions_of(t, 4, 5);
+	KUNIT_EXPECT_LE(test, damon_nr_regions(t), 12u);
+	damon_for_each_region(r, t) {
+		KUNIT_EXPECT_GE(test, damon_sz_region(r) % 5ul, 0ul);
 	}
-	damon_add_region(r, t);
-	damon_split_regions_of(t, 4, 1);
-	KUNIT_EXPECT_LE(test, damon_nr_regions(t), 4u);
 	damon_free_target(t);
 }
 
@@ -574,9 +588,10 @@ static void damos_test_commit_quota_goal(struct kunit *test)
 			});
 	damos_test_commit_quota_goal_for(test, &dst,
 			&(struct damos_quota_goal) {
-			.metric = DAMOS_QUOTA_USER_INPUT,
-			.target_value = 789,
-			.current_value = 12,
+			.metric = DAMOS_QUOTA_SOME_MEM_PSI_US,
+			.target_value = 234,
+			.current_value = 345,
+			.last_psi_total = 567,
 			});
 }
 
-- 
2.43.0

Re: [PATCH] mm/damon/tests/core-kunit: extend test scenarios and remove redundancy

[SeongJae Park]

Hello Shu,


Thank you for sending this patch :)

On Sun, 21 Dec 2025 13:01:14 +0000 Shu Anzai <shu17az@gmail.com> wrote:

> Add some missing test scenarios to cover a wider range of cases. Also,
> remove a redundant case in damos_test_commit_quota_goal().

Seems this patch is making more than one change to multiple test cases at once.
It makes following which change is for what purpose bit difficult for me.  I'd
suggest splitting this into smaller ones that making changes for each test
function, and adding more explanation of the changes.  E.g., a patch for
damon_test_split_at(), another one for damon_test_merge_two(), and so on.  Not
a strong request, though.

I have two questions below, though.

> 
> Signed-off-by: Shu Anzai <shu17az@gmail.com>
> ---
>  mm/damon/tests/core-kunit.h | 51 ++++++++++++++++++++++++-------------
>  1 file changed, 33 insertions(+), 18 deletions(-)
> 
> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index f59ae7ee19a0..e9ccc3fb34f9 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -158,6 +158,7 @@ static void damon_test_split_at(struct kunit *test)
>  	r->nr_accesses_bp = 420000;
>  	r->nr_accesses = 42;
>  	r->last_nr_accesses = 15;
> +	r->age = 10;
>  	damon_add_region(r, t);
>  	damon_split_region_at(t, r, 25);
>  	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
> @@ -170,6 +171,7 @@ static void damon_test_split_at(struct kunit *test)
>  	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, r_new->nr_accesses_bp);
>  	KUNIT_EXPECT_EQ(test, r->nr_accesses, r_new->nr_accesses);
>  	KUNIT_EXPECT_EQ(test, r->last_nr_accesses, r_new->last_nr_accesses);
> +	KUNIT_EXPECT_EQ(test, r->age, r_new->age);
>  
>  	damon_free_target(t);
>  }

I understand that the above change is increasing the coverage of this test to
also verify the age of splitted region.  Nice.

Correct me if I'm misunderstanding the intention of the above diff.

> @@ -190,6 +192,7 @@ static void damon_test_merge_two(struct kunit *test)
>  	}
>  	r->nr_accesses = 10;
>  	r->nr_accesses_bp = 100000;
> +	r->age = 9;
>  	damon_add_region(r, t);
>  	r2 = damon_new_region(100, 300);
>  	if (!r2) {
> @@ -198,12 +201,15 @@ static void damon_test_merge_two(struct kunit *test)
>  	}
>  	r2->nr_accesses = 20;
>  	r2->nr_accesses_bp = 200000;
> +	r2->age = 21;
>  	damon_add_region(r2, t);
>  
>  	damon_merge_two_regions(t, r, r2);
>  	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
>  	KUNIT_EXPECT_EQ(test, r->ar.end, 300ul);
>  	KUNIT_EXPECT_EQ(test, r->nr_accesses, 16u);
> +	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, 160000u);
> +	KUNIT_EXPECT_EQ(test, r->age, 17u);
>  
>  	i = 0;
>  	damon_for_each_region(r3, t) {

I understand that the above change is increasing the coverage of this test to
also verify the age handling of the merge logic.  Looks good!

> @@ -232,12 +238,12 @@ static void damon_test_merge_regions_of(struct kunit *test)
>  {
>  	struct damon_target *t;
>  	struct damon_region *r;
> -	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184};
> -	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230};
> -	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2};
> +	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184, 235, 240};
> +	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230, 240, 10235};
> +	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2, 5, 5};
>  
> -	unsigned long saddrs[] = {0, 114, 130, 156, 170};
> -	unsigned long eaddrs[] = {112, 130, 156, 170, 230};
> +	unsigned long saddrs[] = {0, 114, 130, 156, 170, 235, 240};
> +	unsigned long eaddrs[] = {112, 130, 156, 170, 230, 240, 10235};
>  	int i;
>  
>  	t = damon_new_target();
> @@ -255,9 +261,9 @@ static void damon_test_merge_regions_of(struct kunit *test)
>  	}
>  
>  	damon_merge_regions_of(t, 9, 9999);
> -	/* 0-112, 114-130, 130-156, 156-170 */
> -	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
> -	for (i = 0; i < 5; i++) {
> +	/* 0-112, 114-130, 130-156, 156-170, 170-230, 235-240, 240-10235 */
> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 7u);
> +	for (i = 0; i < 7; i++) {
>  		r = __nth_region_of(t, i);
>  		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
>  		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);

I understand the above change adds two regions on the test input, but I'm not
following what logic it intends to additionally test.  Could you please
clarify?

> @@ -269,6 +275,9 @@ static void damon_test_split_regions_of(struct kunit *test)
>  {
>  	struct damon_target *t;
>  	struct damon_region *r;
> +	unsigned long sa[] = {0, 300, 500};
> +	unsigned long ea[] = {220, 400, 700};
> +	int i;
>  
>  	t = damon_new_target();
>  	if (!t)
> @@ -286,14 +295,19 @@ static void damon_test_split_regions_of(struct kunit *test)
>  	t = damon_new_target();
>  	if (!t)
>  		kunit_skip(test, "second target alloc fail");
> -	r = damon_new_region(0, 220);
> -	if (!r) {
> -		damon_free_target(t);
> -		kunit_skip(test, "second region alloc fail");
> +	for (i = 0; i < ARRAY_SIZE(sa); i++) {
> +		r = damon_new_region(sa[i], ea[i]);
> +		if (!r) {
> +			damon_free_target(t);
> +			kunit_skip(test, "region alloc fail");
> +		}
> +		damon_add_region(r, t);
> +	}
> +	damon_split_regions_of(t, 4, 5);
> +	KUNIT_EXPECT_LE(test, damon_nr_regions(t), 12u);
> +	damon_for_each_region(r, t) {
> +		KUNIT_EXPECT_GE(test, damon_sz_region(r) % 5ul, 0ul);
>  	}
> -	damon_add_region(r, t);
> -	damon_split_regions_of(t, 4, 1);
> -	KUNIT_EXPECT_LE(test, damon_nr_regions(t), 4u);
>  	damon_free_target(t);
>  }

I understand that the above change make the existing test scenario bit more
complex, and cover the alignment.  Looks good.  But
damon_test_split_regions_of() aims to cover multiple scenarios.  Your change is
updating one existing test scenario, so I'm bit concerned at the fact that it
is removing one test case.  I understand the updated test scenario is including
the old one, but I think keeping the current test is also ok, as long as the
maintenace burden is not that high.  So, instead of modifying an existing test
scenario, how about adding the new test case?

>  
> @@ -574,9 +588,10 @@ static void damos_test_commit_quota_goal(struct kunit *test)
>  			});
>  	damos_test_commit_quota_goal_for(test, &dst,
>  			&(struct damos_quota_goal) {
> -			.metric = DAMOS_QUOTA_USER_INPUT,
> -			.target_value = 789,
> -			.current_value = 12,
> +			.metric = DAMOS_QUOTA_SOME_MEM_PSI_US,
> +			.target_value = 234,
> +			.current_value = 345,
> +			.last_psi_total = 567,
>  			});
>  }

Thank you for correcting this!


Thanks,
SJ

[...]

Re: [PATCH] mm/damon/tests/core-kunit: extend test scenarios and remove redundancy

[Shu Anzai]

Hello SJ,

Thank you for reviewing my patch! My responses are below.

On 2025/12/21 12:13, SeongJae Park wrote:
> Hello Shu,
>
>
> Thank you for sending this patch :)
>
> On Sun, 21 Dec 2025 13:01:14 +0000 Shu Anzai <shu17az@gmail.com> wrote:
>
>> Add some missing test scenarios to cover a wider range of cases. Also,
>> remove a redundant case in damos_test_commit_quota_goal().
> Seems this patch is making more than one change to multiple test cases at once.
> It makes following which change is for what purpose bit difficult for me.  I'd
> suggest splitting this into smaller ones that making changes for each test
> function, and adding more explanation of the changes.  E.g., a patch for
> damon_test_split_at(), another one for damon_test_merge_two(), and so on.  Not
> a strong request, though.
>
> I have two questions below, though.

I see. I will split this and send v2 later. Let me first explain the 
changes in detail.

>
>> Signed-off-by: Shu Anzai <shu17az@gmail.com>
>> ---
>>   mm/damon/tests/core-kunit.h | 51 ++++++++++++++++++++++++-------------
>>   1 file changed, 33 insertions(+), 18 deletions(-)
>>
>> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
>> index f59ae7ee19a0..e9ccc3fb34f9 100644
>> --- a/mm/damon/tests/core-kunit.h
>> +++ b/mm/damon/tests/core-kunit.h
>> @@ -158,6 +158,7 @@ static void damon_test_split_at(struct kunit *test)
>>   	r->nr_accesses_bp = 420000;
>>   	r->nr_accesses = 42;
>>   	r->last_nr_accesses = 15;
>> +	r->age = 10;
>>   	damon_add_region(r, t);
>>   	damon_split_region_at(t, r, 25);
>>   	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
>> @@ -170,6 +171,7 @@ static void damon_test_split_at(struct kunit *test)
>>   	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, r_new->nr_accesses_bp);
>>   	KUNIT_EXPECT_EQ(test, r->nr_accesses, r_new->nr_accesses);
>>   	KUNIT_EXPECT_EQ(test, r->last_nr_accesses, r_new->last_nr_accesses);
>> +	KUNIT_EXPECT_EQ(test, r->age, r_new->age);
>>   
>>   	damon_free_target(t);
>>   }
> I understand that the above change is increasing the coverage of this test to
> also verify the age of splitted region.  Nice.
>
> Correct me if I'm misunderstanding the intention of the above diff.

Yes, that is correct.

>
>> @@ -190,6 +192,7 @@ static void damon_test_merge_two(struct kunit *test)
>>   	}
>>   	r->nr_accesses = 10;
>>   	r->nr_accesses_bp = 100000;
>> +	r->age = 9;
>>   	damon_add_region(r, t);
>>   	r2 = damon_new_region(100, 300);
>>   	if (!r2) {
>> @@ -198,12 +201,15 @@ static void damon_test_merge_two(struct kunit *test)
>>   	}
>>   	r2->nr_accesses = 20;
>>   	r2->nr_accesses_bp = 200000;
>> +	r2->age = 21;
>>   	damon_add_region(r2, t);
>>   
>>   	damon_merge_two_regions(t, r, r2);
>>   	KUNIT_EXPECT_EQ(test, r->ar.start, 0ul);
>>   	KUNIT_EXPECT_EQ(test, r->ar.end, 300ul);
>>   	KUNIT_EXPECT_EQ(test, r->nr_accesses, 16u);
>> +	KUNIT_EXPECT_EQ(test, r->nr_accesses_bp, 160000u);
>> +	KUNIT_EXPECT_EQ(test, r->age, 17u);
>>   
>>   	i = 0;
>>   	damon_for_each_region(r3, t) {
> I understand that the above change is increasing the coverage of this test to
> also verify the age handling of the merge logic.  Looks good!

Exactly.

>
>> @@ -232,12 +238,12 @@ static void damon_test_merge_regions_of(struct kunit *test)
>>   {
>>   	struct damon_target *t;
>>   	struct damon_region *r;
>> -	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184};
>> -	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230};
>> -	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2};
>> +	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184, 235, 240};
>> +	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230, 240, 10235};
>> +	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2, 5, 5};
>>   
>> -	unsigned long saddrs[] = {0, 114, 130, 156, 170};
>> -	unsigned long eaddrs[] = {112, 130, 156, 170, 230};
>> +	unsigned long saddrs[] = {0, 114, 130, 156, 170, 235, 240};
>> +	unsigned long eaddrs[] = {112, 130, 156, 170, 230, 240, 10235};
>>   	int i;
>>   
>>   	t = damon_new_target();
>> @@ -255,9 +261,9 @@ static void damon_test_merge_regions_of(struct kunit *test)
>>   	}
>>   
>>   	damon_merge_regions_of(t, 9, 9999);
>> -	/* 0-112, 114-130, 130-156, 156-170 */
>> -	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
>> -	for (i = 0; i < 5; i++) {
>> +	/* 0-112, 114-130, 130-156, 156-170, 170-230, 235-240, 240-10235 */
>> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 7u);
>> +	for (i = 0; i < 7; i++) {
>>   		r = __nth_region_of(t, i);
>>   		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
>>   		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
> I understand the above change adds two regions on the test input, but I'm not
> following what logic it intends to additionally test.  Could you please
> clarify?

Both cases were intended to verify that damon_merge_two_regions() is 
called properly in damon_merge_regions_of().
The first one was intended to ensure that non-adjacent regions (170-230, 
235-240) are not merged even if their nr_accesses difference is within 
the threshold. However, on second thought, I realized this is redundant 
since it is natural for non-adjacent regions not to be merged.
The second one is to verify that two adjacent regions (235-240, 
240-10235) are not merged if the resulting region would exceed the size 
limit.

>
>> @@ -269,6 +275,9 @@ static void damon_test_split_regions_of(struct kunit *test)
>>   {
>>   	struct damon_target *t;
>>   	struct damon_region *r;
>> +	unsigned long sa[] = {0, 300, 500};
>> +	unsigned long ea[] = {220, 400, 700};
>> +	int i;
>>   
>>   	t = damon_new_target();
>>   	if (!t)
>> @@ -286,14 +295,19 @@ static void damon_test_split_regions_of(struct kunit *test)
>>   	t = damon_new_target();
>>   	if (!t)
>>   		kunit_skip(test, "second target alloc fail");
>> -	r = damon_new_region(0, 220);
>> -	if (!r) {
>> -		damon_free_target(t);
>> -		kunit_skip(test, "second region alloc fail");
>> +	for (i = 0; i < ARRAY_SIZE(sa); i++) {
>> +		r = damon_new_region(sa[i], ea[i]);
>> +		if (!r) {
>> +			damon_free_target(t);
>> +			kunit_skip(test, "region alloc fail");
>> +		}
>> +		damon_add_region(r, t);
>> +	}
>> +	damon_split_regions_of(t, 4, 5);
>> +	KUNIT_EXPECT_LE(test, damon_nr_regions(t), 12u);
>> +	damon_for_each_region(r, t) {
>> +		KUNIT_EXPECT_GE(test, damon_sz_region(r) % 5ul, 0ul);
>>   	}
>> -	damon_add_region(r, t);
>> -	damon_split_regions_of(t, 4, 1);
>> -	KUNIT_EXPECT_LE(test, damon_nr_regions(t), 4u);
>>   	damon_free_target(t);
>>   }
> I understand that the above change make the existing test scenario bit more
> complex, and cover the alignment.  Looks good.  But
> damon_test_split_regions_of() aims to cover multiple scenarios.  Your change is
> updating one existing test scenario, so I'm bit concerned at the fact that it
> is removing one test case.  I understand the updated test scenario is including
> the old one, but I think keeping the current test is also ok, as long as the
> maintenace burden is not that high.  So, instead of modifying an existing test
> scenario, how about adding the new test case?

I agree. I will restore the test case I removed and add the new one 
instead.

>
>>   
>> @@ -574,9 +588,10 @@ static void damos_test_commit_quota_goal(struct kunit *test)
>>   			});
>>   	damos_test_commit_quota_goal_for(test, &dst,
>>   			&(struct damos_quota_goal) {
>> -			.metric = DAMOS_QUOTA_USER_INPUT,
>> -			.target_value = 789,
>> -			.current_value = 12,
>> +			.metric = DAMOS_QUOTA_SOME_MEM_PSI_US,
>> +			.target_value = 234,
>> +			.current_value = 345,
>> +			.last_psi_total = 567,
>>   			});
>>   }
> Thank you for correcting this!
>
>
> Thanks,
> SJ
>
> [...]

Best,
Shu

Re: [PATCH] mm/damon/tests/core-kunit: extend test scenarios and remove redundancy

[SeongJae Park]

On Mon, 22 Dec 2025 03:33:52 -0800 Shu Anzai <shu17az@gmail.com> wrote:

> Hello SJ,
> 
> Thank you for reviewing my patch! My responses are below.
> 
> On 2025/12/21 12:13, SeongJae Park wrote:
> > Hello Shu,
> >
> >
> > Thank you for sending this patch :)
> >
> > On Sun, 21 Dec 2025 13:01:14 +0000 Shu Anzai <shu17az@gmail.com> wrote:
> >
> >> Add some missing test scenarios to cover a wider range of cases. Also,
> >> remove a redundant case in damos_test_commit_quota_goal().
> > Seems this patch is making more than one change to multiple test cases at once.
> > It makes following which change is for what purpose bit difficult for me.  I'd
> > suggest splitting this into smaller ones that making changes for each test
> > function, and adding more explanation of the changes.  E.g., a patch for
> > damon_test_split_at(), another one for damon_test_merge_two(), and so on.  Not
> > a strong request, though.
> >
> > I have two questions below, though.
> 
> I see. I will split this and send v2 later. Let me first explain the 
> changes in detail.

Looking forward to the v2! :)

[...]
> >> @@ -232,12 +238,12 @@ static void damon_test_merge_regions_of(struct kunit *test)
> >>   {
> >>   	struct damon_target *t;
> >>   	struct damon_region *r;
> >> -	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184};
> >> -	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230};
> >> -	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2};
> >> +	unsigned long sa[] = {0, 100, 114, 122, 130, 156, 170, 184, 235, 240};
> >> +	unsigned long ea[] = {100, 112, 122, 130, 156, 170, 184, 230, 240, 10235};
> >> +	unsigned int nrs[] = {0, 0, 10, 10, 20, 30, 1, 2, 5, 5};
> >>   
> >> -	unsigned long saddrs[] = {0, 114, 130, 156, 170};
> >> -	unsigned long eaddrs[] = {112, 130, 156, 170, 230};
> >> +	unsigned long saddrs[] = {0, 114, 130, 156, 170, 235, 240};
> >> +	unsigned long eaddrs[] = {112, 130, 156, 170, 230, 240, 10235};
> >>   	int i;
> >>   
> >>   	t = damon_new_target();
> >> @@ -255,9 +261,9 @@ static void damon_test_merge_regions_of(struct kunit *test)
> >>   	}
> >>   
> >>   	damon_merge_regions_of(t, 9, 9999);
> >> -	/* 0-112, 114-130, 130-156, 156-170 */
> >> -	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 5u);
> >> -	for (i = 0; i < 5; i++) {
> >> +	/* 0-112, 114-130, 130-156, 156-170, 170-230, 235-240, 240-10235 */
> >> +	KUNIT_EXPECT_EQ(test, damon_nr_regions(t), 7u);
> >> +	for (i = 0; i < 7; i++) {
> >>   		r = __nth_region_of(t, i);
> >>   		KUNIT_EXPECT_EQ(test, r->ar.start, saddrs[i]);
> >>   		KUNIT_EXPECT_EQ(test, r->ar.end, eaddrs[i]);
> > I understand the above change adds two regions on the test input, but I'm not
> > following what logic it intends to additionally test.  Could you please
> > clarify?
> 
> Both cases were intended to verify that damon_merge_two_regions() is 
> called properly in damon_merge_regions_of().
> The first one was intended to ensure that non-adjacent regions (170-230, 
> 235-240) are not merged even if their nr_accesses difference is within 
> the threshold. However, on second thought, I realized this is redundant 
> since it is natural for non-adjacent regions not to be merged.
> The second one is to verify that two adjacent regions (235-240, 
> 240-10235) are not merged if the resulting region would exceed the size 
> limit.

Makes sense, please add the details to the second version.


Thanks,
SJ

[...]