Re: [PATCH 2/2] mm/damon/tests/core-kunit: test split above max_nr_regions/2

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 5/21/26 1:41 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The new unit test `damon_test_split_above_half_progresses` fails to effectively test its target logic and introduces architecture-dependent flakiness due to relying on default page sizes.
> --
>
> commit 41d857e275ff50ce39c3b59f85362709927d922a
> Author: Jiayuan Chen <jiayuan.chen@shopee.com>
>
> mm/damon/tests/core-kunit: test split above max_nr_regions/2
>
> This commit adds a kunit test to exercise kdamond_split_regions() when the
> total region count is already above max_nr_regions / 2. It asserts that the
> function can still produce new regions without overshooting the maximum
> region limit.
>
>> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
>> index 866f716e5760d..c87933c58785b 100644
>> --- a/mm/damon/tests/core-kunit.h
>> +++ b/mm/damon/tests/core-kunit.h
> [ ... ]
>> +static void damon_test_split_above_half_progresses(struct kunit *test)
>> +{
>> +	struct damon_ctx *c;
>> +	struct damon_target *t;
>> +	struct damon_region *r, *big;
>> +	unsigned long start;
>> +	unsigned int nr_before, nr_after, i;
>> +	const unsigned int nr_small = 799;
>> +	const unsigned long small_sz = 10;
>> +	const unsigned long big_sz = 1000000;
>> +
>> +	c = damon_new_ctx();
> [Severity: Medium]
> Does this introduce a test regression on architectures with large page sizes?
>
> Because damon_new_ctx() sets c->min_region_sz to the default PAGE_SIZE,
> this test might become flaky or fail on architectures with larger pages.
>
> For example, on architectures with a 256KB page size, the random split math
> could calculate a sub-region size that aligns down to 0, which would skip
> the split. On architectures with 1MB pages, the test would deterministically
> fail because the big region size (1,000,000) is smaller than
> 2 * min_region_sz.
>
> Will this cause the KUNIT_EXPECT_GT(test, nr_after, nr_before) assertion
> below to randomly or deterministically fail since no regions will be split?
>
> [ ... ]
>> +	nr_after = damon_nr_regions(t);
>> +	KUNIT_EXPECT_GT(test, nr_after, nr_before);
>> +	KUNIT_EXPECT_LE(test, (unsigned long)nr_after,
>> +			c->attrs.max_nr_regions);
> [Severity: Medium]
> Does this test actually exercise the budget constraint logic to ensure the
> region limit isn't overshot?
>
> Since the setup only provides a single splittable "big" region, and
> damon_split_zero_age_regions() skips newly inserted regions during its
> traversal, wouldn't this only ever produce at most 1 new region before
> terminating?
>
> If it can only split once, it seems structurally impossible for the test to
> reach or exceed the remaining budget of 700 regions, which might defeat the
> stated purpose of checking if the function overshoots the limit.
>
>> +
>> +	damon_destroy_ctx(c);
>> +}


Both are valid, thanks.  I'll fold them into the respin once the series 
has been reviewed by the maintainer.