Re: [RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()

On Tue, 28 Jul 2026 05:33:44 +0000 [email protected] wrote:

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] The newly added `damon_test_nr_samples_per_aggr` test uses tautological assertions that will always pass, failing to detect the regressions it was intended to prevent. Additionally, the test setup overflows to zero on 32-bit architectures.
> --
> 
> --- Patch [7]: [RFC PATCH 07/10] mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr() ---
> commit 50a7320dda40ca169bc23924caa1421599f78314
> Author: SJ Park <[email protected]>
> 
> mm/damon/tests/core-kunit: test damon_nr_samples_per_aggr()
> 
> This commit adds a kunit test for damon_nr_samples_per_aggr() to ensure
> that extreme interval setups are handled properly without returning zero
> or random overflowed values.
> 
> > diff --git a/mm/damon/tests/core-kunit.h b/mm/damon/tests/core-kunit.h
> > index 3041b8288ec26..511f2856d343c 100644
> > --- a/mm/damon/tests/core-kunit.h
> > +++ b/mm/damon/tests/core-kunit.h
> 
> [ ... ]
> 
> > +static void damon_test_nr_samples_per_aggr(struct kunit *test)
> > +{
> > +	struct damon_attrs attrs = {
> > +		.sample_interval = 0,
> > +		.aggr_interval = 0,
> > +	};
> > +
> > +	/* Zero aggregation interval doesn't cause division by zero */
> > +	KUNIT_EXPECT_EQ(test, damon_nr_samples_per_aggr(&attrs), 1);
> > +
> > +	/* Too large aggregation interval doesn't cause overflow */
> > +	attrs.aggr_interval = (unsigned long)UINT_MAX + 1;
> 
> [Severity: Medium]
> On 32-bit systems where unsigned long is 32-bits wide, does this assignment
> overflow to 0?
> 
> If it overflows, the test would configure an interval of 0 instead of a value
> larger than UINT_MAX, failing to test the large-interval scenario.

Yes, this is for testing on 64bit system.  I will add this to the comment.

> 
> > +	KUNIT_EXPECT_GE(test, damon_nr_samples_per_aggr(&attrs), 0);
> > +	KUNIT_EXPECT_LE(test, damon_nr_samples_per_aggr(&attrs), UINT_MAX);
> > +}
> 
> [Severity: Medium]
> Since damon_nr_samples_per_aggr() returns an unsigned int, are these
> assertions tautological?
> 
> An unsigned int is inherently always >= 0 and <= UINT_MAX. This means the
> test will pass regardless of the function's return value, and it wouldn't
> catch if the function unexpectedly returned 0.

Good catch.  The test should ensure the return value on 64 bit system is
exactly UINT_MAX.  I will do so.

> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7


Thanks,
SJ