Re: [RFC PATCH v3 2/9] mm/damon/core: replace the access report buffer with per-context rings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mm: Off-by-one error drops valid reports for the maximum probe index
- [High] mm: Preempt-unsafe this_cpu_inc() usage in preemptible kthread context
- [Low] mm: Spurious target-lookup allocation failure warning when targets are zero
--

commit 8fff2ca2d88fc496c6ec48a611f9bfdad8cf1ae4
Author: Ravi Jonnalagadda <ravis.opensrc@gmail.com>

mm/damon/core: replace the access report buffer with per-context rings

This commit replaces the global access report buffer with per-context, per-CPU
SPSC rings for NMI-safe access reporting. It updates the kdamond drain logic
to process these rings using a binary search over a per-target region snapshot.

> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 886e068e78447..4fd1db12bc49b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c

[ ... ]

> +static void __kdamond_drain_ring(struct damon_ctx *ctx,
> +		struct damon_target_lookup *tbl,
> +		struct damon_report_ring __percpu *ring_pcpu,
> +		cpumask_t *pending)
> +{

[ ... ]

> +		while (tail != head) {
> +			unsigned long stale_before;
> +			int pidx;
> +
> +			entry = &ring->entries[tail];
> +			/*
> +			 * Use sample_interval (not aggr_interval) as the
> +			 * staleness window: entries older than one sample
> +			 * interval are from a previous monitoring tick and
> +			 * should not inflate the current aggregation window.
> +			 */
> +			stale_before = jiffies -
> +				usecs_to_jiffies(ctx->attrs.sample_interval);
> +			if (time_before(entry->report_jiffies, stale_before)) {
> +				this_cpu_inc(damon_samples_stale_drained);

[Severity: High]
Does this code trigger a BUG: using this_cpu_inc() in preemptible context
warning? __kdamond_drain_ring() runs in the kdamond kernel thread, which is a
fully preemptible context, but this_cpu_inc() is preempt-unsafe without
explicitly disabling preemption.

> +				goto next;
> +			}
> +			pidx = entry->probe_idx;
> +			/*
> +			 * Every entry in this ring is a perf-event report
> +			 * (probe_idx >= 1); damon_report_access() drops any
> +			 * DAMON_PROBE_IDX_NONE report before it reaches a ring.
> +			 * Reject only out-of-range indices (>= DAMON_MAX_PROBES)
> +			 * and, defensively, any non-positive value.
> +			 */
> +			if (pidx <= 0 || pidx >= DAMON_MAX_PROBES)

[Severity: High]
Does this bounds check inadvertently discard reports for the 4th configured
probe? The probe_idx is 1-based, meaning valid indices range from 1 to
DAMON_MAX_PROBES (4). However, checking pidx >= DAMON_MAX_PROBES drops the
report if pidx == 4.

> +				goto next;
> +

[ ... ]

> +static void kdamond_check_reported_accesses(struct damon_ctx *ctx)
> +{
> +	struct damon_target_lookup *tbl;
> +	unsigned int nr_targets = 0;
> +
> +	/*
> +	 * Build the sorted region snapshot once for this drain.  If the alloc
> +	 * fails, skip the drain this tick rather than falling back to a linear
> +	 * scan (a missed tick self-heals; a linear scan does not).
> +	 */
> +	tbl = damon_build_target_lookup(ctx, &nr_targets);
> +	if (!tbl) {
> +		pr_warn_ratelimited(
> +			"damon: target-lookup alloc failed; ring drain skipped this tick\n");

[Severity: Low]
Can this spam the kernel log with spurious warnings when 0 targets are
configured?

If a DAMON context is started with 0 targets, damon_build_target_lookup() sets
nr_targets to 0 and correctly returns NULL if the lookup buffer was never
previously allocated. This code misinterprets the NULL as an allocation failure.

> +		return;
> +	}
> +
> +	if (damon_drains_ring_perf(ctx))
> +		__kdamond_drain_ring(ctx, tbl, ctx->perf_rings,
> +				&ctx->perf_pending);
> +}

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