Re: [RFC PATCH v4 7/9] mm/damon/tests/drain-kunit: kunit for report rings and ring drain

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] mm: kunit test fails on architectures with PAGE_SIZE > 4KB
- [Medium] mm: kunit test flaky due to short staleness timeout
- [Low] mm: kunit test flaky due to hardcoded magic TGID 9999
- [Low] mm: kunit test flaky due to assertion on global state
--

commit 00b2c09e452d8dccc585f63512e7c9dee88a28fd
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

    mm/damon/tests/drain-kunit: kunit for report rings and ring drain
    
    Add kunit coverage for the per-context perf report rings and the drain
    that credits regions from them. Wire the suites into core.c so
    CONFIG_DAMON_KUNIT_TEST=y builds them.

> diff --git a/mm/damon/tests/drain-kunit.h b/mm/damon/tests/drain-kunit.h
> new file mode 100644
> --- /dev/null
> +++ b/mm/damon/tests/drain-kunit.h

[ ... ]

> +static void damon_test_unified_vaddr_match(struct kunit *test)
> +{
> +	struct damon_ctx *ctx;
> +	struct damon_target *t;
> +	struct damon_region *r;
> +	struct damon_access_report rep = {
> +		.paddr     = 0,
> +		.vaddr     = 0x1500,
> +		.probe_idx = 1,
> +		.size      = PAGE_SIZE,
> +	};
> +	unsigned long before, after;
> +	int hits;
> +
> +	ctx = damon_new_ctx();

[ ... ]

> +	/*
> +	 * Region must fully contain the report [vaddr, vaddr + size): a report
> +	 * straddling the region end is rejected by the drain.  With
> +	 * vaddr=0x1500 and size=PAGE_SIZE the region must reach >= 0x2500.
> +	 */
> +	r = damon_new_region(0x1000, 0x3000);

[Severity: Medium]
Will this test fail deterministically on architectures where PAGE_SIZE exceeds
4KB (such as ARM64 with 64KB pages)?

The injected report size is PAGE_SIZE starting at 0x1500. If PAGE_SIZE is
larger than 6.75KB, the report will straddle the hardcoded 8KB region boundary
ending at 0x3000, causing the drain logic to reject it and fail the test.

> +	if (!r) {
> +		put_pid(t->pid);
> +		damon_free_target(t);
> +		damon_destroy_ctx(ctx);
> +		kunit_skip(test, "region alloc failed");
> +	}
> +	damon_add_region(r, t);
> +	damon_add_target(ctx, t);
> +
> +	rep.report_jiffies = jiffies;
> +	before = damon_get_samples_drained();
> +	damon_report_access(&rep);
> +	kdamond_check_reported_accesses(ctx);

[Severity: Medium]
Could this test be flaky under CPU load or preemption?

If execution is delayed by just a few ticks between capturing jiffies and
evaluating the drain:

	rep.report_jiffies = jiffies;
	before = damon_get_samples_drained();
	damon_report_access(&rep);
	<-- execution delayed by preemption here
	kdamond_check_reported_accesses(ctx);

Couldn't the drain logic in __kdamond_drain_ring() drop the report as stale
since the default sample_interval is 5ms?

[ ... ]

> +static void damon_test_unified_vaddr_tgid_mismatch(struct kunit *test)
> +{
> +	struct damon_ctx *ctx;
> +	struct damon_target *t;
> +	struct damon_region *r;
> +	struct damon_access_report rep = {
> +		.paddr     = 0,
> +		.vaddr     = 0x1500,
> +		.tgid      = 9999,	/* matches no target */
> +		.probe_idx = 1,
> +		.size      = PAGE_SIZE,
> +	};

[Severity: Low]
Could this intermittently fail if the test runner thread happens to be
assigned PID 9999 by the OS?

If that occurs, the TGIDs would match, erroneously crediting the report
and causing the test assertion to fail.

[ ... ]

> +static void damon_test_report_return_value(struct kunit *test)
> +{
> +	struct damon_ctx *ctx;
> +	struct damon_access_report rep = {
> +		.paddr = 0x15000, .probe_idx = 1, .size = PAGE_SIZE,
> +	};
> +	unsigned long full_before, busy_before;
> +	unsigned int queued = 0, dropped = 0;
> +	int i;
> +
> +	ctx = damon_new_ctx();

[ ... ]

> +	busy_before = damon_get_report_busy_drop();
> +
> +	/* One past capacity, so the last iteration must be a drop. */
> +	for (i = 0; i < DAMON_REPORT_RING_SIZE; i++) {
> +		if (damon_report_access(&rep))
> +			queued++;
> +		else
> +			dropped++;
> +	}
> +	preempt_enable();
> +
> +	KUNIT_EXPECT_EQ(test, queued, (unsigned int)DAMON_REPORT_RING_SIZE - 1);
> +	KUNIT_EXPECT_EQ(test, dropped, 1u);
> +	/* No NMI nests here, so the drop must be the full ring. */
> +	KUNIT_EXPECT_GT(test, damon_get_report_ring_full(), full_before);
> +	KUNIT_EXPECT_EQ(test, damon_get_report_busy_drop(), busy_before);

[Severity: Low]
Is it safe to assert that this global counter remains completely unchanged
during the test's execution?

Since damon_get_report_busy_drop() aggregates the damon_report_busy_drop_perf
counter across all CPUs, wouldn't concurrent perf NMI activity in other DAMON
contexts on different CPUs increment this counter and cause the test assertion
to fail?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-damon-perf-rfc-v3-send-2026-10-03-v4-0-b03452e137f3@gmail.com?part=7