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

SeongJae Park
2025-12-22T15:24:34Z

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 …