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

Shu Anzai
2025-12-22T11:33:54Z

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