Re: [RFC PATCH v1.2 1/7] mm/damon/core: remove comment and test for nr_to_bp() divide-by-zero

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit message claims to entirely remove the test code, but instead leaves a broken, orphaned test function causing build failures.
--

--- Patch [1]: [RFC PATCH v1.2 1/7] mm/damon/core: remove comment and test for nr_to_bp() divide-by-zero ---
commit d67544b646a2eb5eb3336e5b2555a05dd0f2f18c
Author: SJ Park <sj@kernel.org>

mm/damon/core: remove comment and test for nr_to_bp() divide-by-zero

This patch removes obsolete comments and test code related to division by zero
in DAMON's nr_accesses_to_accesses_bp() function, noting that earlier commits
prevent zero aggregation intervals from reaching this code.

> Commit 35d4a3cf70a8 ("mm/damon/ops-common: handle extreme
> intervals in damon_hot_score()") modified damon_nr_samples_per_aggr() to
> always return non-zero.  Hence no division-by-zero of the note can
> happen.  Remove the obsolete comment on the function.  The test code was
> written to test the division-by-zero case, which cannot happen anymore.
> Having it makes no sense.  Entirely remove the test code and its
> comment.

[Severity: Medium]
The commit message states that the test code was entirely removed, but the
diff shows the test being rewritten to damon_test_max_nr_accesses() instead.

Also, should the commit message reference damon_max_nr_accesses() here
instead of damon_nr_samples_per_aggr(), given the function that is actually
being tested in the diff?

> diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> index 0124f83b39b83..b2a280556b841 100644
> --- a/mm/damon/tests/core-kunit.h
> +++ b/mm/damon/tests/core-kunit.h
> @@ -575,26 +575,20 @@ static void damon_test_set_regions(struct kunit *test)
>  			}, 3);
>  }
>  
> -static void damon_test_nr_accesses_to_accesses_bp(struct kunit *test)
> +static void damon_test_max_nr_accesses(struct kunit *test)
>  {
>  	struct damon_attrs attrs = {
> -		.sample_interval = 10,
> -		.aggr_interval = ((unsigned long)UINT_MAX + 1) * 10
> +		.sample_interval = 0,
> +		.aggr_interval = 0,
>  	};

[ ... ]

> +	/* Zero aggregation interval doesn't cause division by zero */
> -	KUNIT_EXPECT_EQ(test, damon_nr_accesses_to_accesses_bp(123, &attrs), 0);
> +	KUNIT_EXPECT_EQ(test, damon_max_nr_accesses(attrs), 1);

[Severity: Medium]
Will this cause a build failure?

Looking at damon_max_nr_accesses(), it expects a pointer to struct damon_attrs
rather than a struct passed by value. Passing attrs here directly looks like it
will result in an incompatible type error during compilation.

[ ... ]

> @@ -1572,7 +1566,6 @@ static struct kunit_case damon_test_cases[] = {
>  	KUNIT_CASE(damon_test_split_above_half_progresses),
>  	KUNIT_CASE(damon_test_ops_registration),
>  	KUNIT_CASE(damon_test_set_regions),
> -	KUNIT_CASE(damon_test_nr_accesses_to_accesses_bp),
>  	KUNIT_CASE(damon_test_update_monitoring_result),
>  	KUNIT_CASE(damon_test_set_attrs),
>  	KUNIT_CASE(damon_test_mvsum),

[Severity: Medium]
Does removing this test case from the array orphan the newly rewritten test
function above?

Since damon_test_max_nr_accesses() is declared as static and is no longer
referenced by damon_test_cases[], it appears this will trigger a
-Wunused-function warning when compiling the tests.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708013252.95888-1-sj@kernel.org?part=1