Re: [PATCH 2/5] perf/core: add AUX ring accessors for kernel consumers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] perf: misplaced smp_rmb() in perf_event_aux_tail_set()
--

commit bf06e748dd797d73b38eceb210cb85ac496712d0
Author: Kunwu Chan <kunwu.chan@gmail.com>

perf/core: add AUX ring accessors for kernel consumers

This commit adds kernel-consumer accessors for the published AUX producer head,
the consumer tail, and copying a possibly wrapped AUX interval. It introduces
these helpers to allow kernel consumers to own and drain the AUX buffer
without relying on the overflow path.

> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index eae28e42346c7..2c6f260fcf91b 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
[ ... ]
> +int perf_event_aux_tail_set(struct perf_event *event, unsigned long tail)
> +{
> +	struct perf_buffer *rb = ring_buffer_get(event);
> +	unsigned long advance, aux_size, head, old_tail;
> +	int ret = -EINVAL;
> +
> +	if (!rb)
> +		return -ENOENT;
> +
> +	if (!rb_has_kernel_aux(rb)) {
> +		ret = -ENOENT;
> +		goto out;
> +	}
> +
> +	aux_size = (unsigned long)rb->aux_nr_pages << PAGE_SHIFT;
> +	old_tail = READ_ONCE(rb->user_page->aux_tail);
> +	/*
> +	 * Pairs with the producer's AUX-data write barrier in
> +	 * perf_aux_output_end() before it publishes aux_head.
> +	 */
> +	smp_rmb();
> +	head = READ_ONCE(rb->user_page->aux_head);

[Severity: Low]
Is this smp_rmb() necessary here? Memory barriers order operations, they do not
accelerate visibility. Since perf_event_aux_tail_set() performs no payload data
reads, there is nothing for the smp_rmb() to order against the read of aux_head,
rendering the barrier functionally inert.

The comment claims it pairs with the producer's AUX-data write barrier, but
without trailing data reads, this pairing is meaningless. Does this result in
unnecessary code complexity and a negligible performance overhead, while
providing no structural safety risk since the subsequent smp_mb() correctly
orders space release?

> +	advance = tail - old_tail;
> +
> +	/* Modular distances keep the check valid when a cursor wraps. */
> +	if (head - old_tail <= aux_size && advance <= head - old_tail) {
> +		/* Order all prior AUX data reads before releasing the space. */
> +		smp_mb();
> +		WRITE_ONCE(rb->user_page->aux_tail, tail);
> +		ret = 0;
> +	}
> +
> +out:
> +	ring_buffer_put(rb);
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004173458.837842-1-kunwu.chan@gmail.com?part=2