Message-ID: <a98e23a1-5be3-4c2a-a65a-c33853683674@gmail.com>
Date: Mon, 22 Dec 2025 03:33:52 -0800
From: Shu Anzai <shu17az@...il.com>
To: SeongJae Park <sj@...nel.org>
Cc: akpm@...ux-foundation.org, yanquanmin1@...wei.com, damon@...ts.linux.dev,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/damon/tests/core-kunit: extend test scenarios and
 remove redundancy

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

Powered by blists - more mailing lists