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

[SeongJae Park <sj@kernel.org>]

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

[...]