Re: [PATCH bpf 5/6] libbpf: ringbuf: Prevent missed wakeups

[Tamir Duberstein <tamird@kernel.org>]

On Thu, Jun 18, 2026 at 02:41:54AM -0400, Emil Tsalapatis wrote:
[...]
> Sashiko is right on this, let's use regular kernel style for new
> comments.

Done in v2.

[...]
> The extra new variable feels overly complicated. I think this is
> becuase the new code is going off of the got_new_data pattern, which
> imo is also unnecessary. We should just remove got_new data and just
> do:
>
> while (true) {
> 	prod_pos = __atomic_load_n(r->producer_pos, __ATOMIC_ACQUIRE);
> 	if (cons_pos == prod_pos)
> 		break;
>
> 	while (cons_pos != prod_pos) {
> 		...
> 	}
> }

V2 removes `needs_wakeup` and leaves one fence after each batch that
advances the consumer position. I kept `got_new_data` because the
proposed unconditional outer loop would retry the same busy record
indefinitely. The revised loop retries a busy record once after
publishing progress and returns if the record is still busy.

[...]
> We just add an smp_mb() here. And since AFAICT smb_mb is available
> here, we can also avoid the move from smp_* to __atomic in the
> previous patch.

I kept the compiler atomics because `smp_mb()` is not a portable full
barrier in the tools headers. Only x86, arm64, and RISC-V define it
directly; the generic fallback can reduce `mb()` to a compiler barrier.
A single `__atomic_thread_fence()` after each batch orders the preceding
consumer-position stores without a full barrier after every record.

[...]
> Why __ATOMIC_RELAXED here? Maybe just declare the variable volatile
> and do regular reads?

I kept the stop flag as a relaxed atomic because `volatile` would leave
a C data race; the flag needs atomicity but no ordering.

[...]
> Label is unnecessary, just call break;

Done in v2.