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

[Kunwu Chan <kunwu.chan@gmail.com>]

On Sat, 03 Oct 2026 14:07:55 -0700 Ravi Jonnalagadda <ravis.opensrc@gmail.com> wrote:

[...]
>  
> @@ -2519,30 +2662,107 @@ int damos_walk(struct damon_ctx *ctx, struct damos_walk_control *control)
>   * damon_report_access() - Report identified access events to DAMON.
>   * @report:	The reporting access information.
>   *
> - * Report access events to DAMON.
> + * Report access events to DAMON via a per-context per-CPU SPSC lockless ring
> + * (ctx->perf_rings).  Producer is the local CPU (typically NMI from a
> + * hardware-sampling backend); consumer is the kdamond drain in
> + * kdamond_check_reported_accesses().
> + *
> + * The destination ring is selected by this_cpu_ptr(), i.e. by the CPU calling
> + * this function, not by @report->cpu, which is sample metadata used by the
> + * drain-side filter.  The two coincide for a sample delivered by an interrupt
> + * on the CPU that produced it.
> + *
> + * A backend whose PMU writes a record stream into a memory buffer instead of
> + * raising a per-sample interrupt, or one reading a device counter table, must
> + * therefore decode CPU N's buffer on CPU N -- for example by queueing per-CPU
> + * work with queue_work_on() -- rather than calling this function in a loop
> + * from one thread.  A single-thread loop puts every report in that thread's
> + * ring, which caps machine-wide capacity at DAMON_REPORT_RING_SIZE - 1
> + * reports per drain regardless of the number of producing CPUs, and does not
> + * satisfy the single-producer invariant if the thread can migrate.
> + *
> + * Context: any (NMI-safe).  An NMI nesting on top of a process-context
> + * producer on the same CPU would otherwise stomp the same entries[head]
> + * slot; the busy guard detects and drops in that case.
>   *
> - * Context: May sleep.
> + * If the ring is full, the sample is dropped and the per-CPU ring-full
> + * counter incremented; a busy-guard drop increments the busy-drop counter.
>   *
> - * NOTE: we may be able to implement this as a lockless queue, and allow any
> - * context.  As the overhead is unknown, and region-based DAMON logics would
> - * guarantee the reports would be not made that frequently, let's start with
> - * this simple implementation.
> + * Return: true if the report was queued, false if it was dropped.  A producer
> + * holding a single report may ignore this.  A producer decoding a batch out
> + * of a hardware buffer should stop on false and leave the remainder in that
> + * buffer for the next round, since a report released from the buffer but not
> + * queued here is not delivered.
>   */
> -void damon_report_access(struct damon_access_report *report)
> +bool damon_report_access(struct damon_access_report *report)
>  {
> -	struct damon_access_report *dst;
> +	/*
> +	 * Only perf-event reports (probe_idx >= 1) have a ring to feed: the
> +	 * global page_fault ring this dispatch also fed has been removed.
> +	 * A probe_idx == DAMON_PROBE_IDX_NONE report has nowhere to go and is
> +	 * dropped here rather than at each caller.
> +	 */
> +	struct damon_report_ring *ring;
> +	cpumask_t *pending;
> +	int __percpu *busy_pcpu;
> +	unsigned int head, next;
> +	int busy;
> +	bool queued = false;
> +	struct damon_ctx *pctx = report->ctx;
> +
> +	if (report->probe_idx == DAMON_PROBE_IDX_NONE)
> +		return false;
>  
> -	/* silently fail for races */
> -	if (!mutex_trylock(&damon_access_reports_lock))
> -		return;
> -	dst = &damon_access_reports[damon_access_reports_len++];
> -	/* just drop all existing reports in favor of simplicity. */
> -	if (damon_access_reports_len == DAMON_ACCESS_REPORTS_CAP)
> -		damon_access_reports_len = 0;
> -	*dst = *report;
> -	dst->report_jiffies = jiffies;
> -	mutex_unlock(&damon_access_reports_lock);
> +	/*
> +	 * A perf report must carry its owning ctx (set by the overflow handler)
> +	 * and that ctx must have an allocated per-ctx perf ring.  If either is
> +	 * missing (e.g. an overflow racing teardown after the ring was freed, or
> +	 * a report raised before the ring was allocated), drop the sample rather
> +	 * than touch NULL/freed storage.
> +	 */
> +	if (!pctx || !pctx->perf_rings || !pctx->perf_ring_busy)
> +		return false;
> +
> +	/* Pin to a CPU so the SPSC invariant holds for preemptible callers. */
> +	preempt_disable();
> +	busy_pcpu = pctx->perf_ring_busy;
> +	busy = this_cpu_inc_return(*busy_pcpu);
> +	if (busy != 1) {
> +		/* NMI nested on a process-context producer; drop. */
> +		this_cpu_inc(damon_report_busy_drop_perf);
> +		goto out;
> +	}
> +
> +	ring = this_cpu_ptr(pctx->perf_rings);
> +	pending = &pctx->perf_pending;
> +	head = ring->head;
> +	next = (head + 1) & DAMON_REPORT_RING_MASK;
> +
> +	if (next == READ_ONCE(ring->tail)) {
> +		this_cpu_inc(damon_report_ring_full_perf);
> +		goto out;
> +	}
> +

Hi Ravi,
 
I noticed that ring overflow drops reports and updates an
internal counter.

Since hardware sampling is used as an access observation source,
could userspace get any indication that reports were lost during
an aggregation window?

Without such visibility, users cannot distinguish an aggregation
result affected by report loss from one collected without loss.
This may make it difficult to evaluate the reliability of the
observed access information.

Thanks,
Kunwu

[...]

Sent using hkml (https://github.com/sjp38/hackermail)