Re: [PATCH] mm/damon/tests/core-kunit: extend test scenarios and remove redundancy
From: Shu Anzai
Date: Mon Dec 22 2025 - 06:33:54 EST
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@xxxxxxxxx> wrote:
Add some missing test scenarios to cover a wider range of cases. Also,Seems this patch is making more than one change to multiple test cases at once.
remove a redundant case in damos_test_commit_quota_goal().
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@xxxxxxxxx>I understand that the above change is increasing the coverage of this test to
---
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);
}
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)I understand that the above change is increasing the coverage of this test to
}
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) {
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)I understand the above change adds two regions on the test input, but I'm not
{
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]);
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)I understand that the above change make the existing test scenario bit more
{
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);
}
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)Thank you for correcting this!
});
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,
});
}
Thanks,
SJ
[...]
Best,
Shu