On 10/7/2025 6:10 AM, Andrii Nakryiko wrote:
[...]
+
+ over_pos = READ_ONCE(rb->overwrite_pos);
+ return min(prod_pos - max(cons_pos, over_pos), rb->mask + 1);
I'm trying to understand why you need to min with `rb->mask + 1`, can
you please elaborate?
We need the min because rb->producer_pos and rb->overwrite_pos are read
at different times. During this gap, a fast producer may wrap once or
more, making over_pos larger than prod_pos.
what if you read overwrite_pos before reading producer_pos? Then it
can't be larger than producer_pos and available data would be
producer_pos - max(consumer_pos, overwrite_pos)? would that work?
No, it won’t work. Between reading overwrite_pos and producer_pos, producer
on a different CPU may have already moved producer_pos forward by more than
one ring buffer size, causing prod_pos - max(cons_pos, over_pos) to exceed
the ring buffer size.
And also, at least for consistency, use smp_load_acquire() for overwrite_pos?
Using READ_ONCE here is to stay symmetric with __bpf_ringbuf_reserve(),
where overwrite_pos is WRITE_ONCE first, followed by
smp_store_release(producer_pos).
So here we do smp_load_acquire(producer_pos) first, then
READ_ONCE(overwrite_pos)
to ensure a consistent view of the ring buffer.
For consistency when reading consumer_pos and producer_pos, I’m fine with
switching READ_ONCE to smp_load_acquire for overwrite_pos.
I'm not sure it matters much, but this function is called outside of
rb->spinlock, while __bpf_ringbuf_reserve() does hold a lock while
doing that WRITE_ONCE(). So it might not make any difference, but I
have mild preference for smp_load_acquire() here.
OK, I'll switch to smp_load_acquire.
}
static u32 ringbuf_total_data_sz(const struct bpf_ringbuf *rb)
@@ -402,11 +419,43 @@ bpf_ringbuf_restore_from_rec(struct bpf_ringbuf_hdr *hdr)
return (void*)((addr & PAGE_MASK) - off);
}
+
[...]
+ /* In overwrite mode, move overwrite_pos to the next record to be
+ * overwritten if the ring buffer is full
+ */
hm... here I think the important point is that we search for the next
record boundary until which we need to overwrite data such that it
fits newly reserved record. "next record to be overwritten" isn't that
important (we might never need to overwrite it). Important are those
aspects of a) staying on record boundary and b) consuming enough
records to reserve the new one.
Can you please update the comment to mention the above points?
Sure, I'll update the comment to:
In overwrite mode, advance overwrite_pos when the ring buffer is full.
The key points are to stay on record boundaries and consume enough
records to fit the new one.
ok
[...]
+ unsigned long rec_pos,
+ unsigned long cons_pos,
+ u32 len, u64 flags)
+{
+ unsigned long rec_end;
+
+ if (flags & BPF_RB_FORCE_WAKEUP)
+ return true;
+
+ if (flags & BPF_RB_NO_WAKEUP)
+ return false;
+
+ /* for non-overwrite mode, if consumer caught up and is waiting for
+ * our record, notify about new data availability
+ */
+ if (likely(!rb->overwrite_mode))
+ return cons_pos == rec_pos;
+
+ /* for overwrite mode, to give the consumer a chance to catch up
+ * before being overwritten, wake up consumer every half a round
+ * ahead.
+ */
+ rec_end = rec_pos + ringbuf_round_up_hdr_len(len);
+
+ cons_pos &= (rb->mask >> 1);
+ rec_pos &= (rb->mask >> 1);
+ rec_end &= (rb->mask >> 1);
+
+ if (cons_pos == rec_pos)
+ return true;
+
+ if (rec_pos < cons_pos && cons_pos < rec_end)
+ return true;
+
+ if (rec_end < rec_pos && (cons_pos > rec_pos || cons_pos < rec_end))
+ return true;
+
hm... ok, let's discuss this. Why do we need to do some half-round
heuristic for overwrite mode? If a consumer is falling behind it
should be actively trying to catch up and they don't need notification
(that's the non-overwrite mode logic already).
So there is more to this than a brief comment you left, can you please
elaborate?
The half-round wakeup was originally intended to work with libbpf in the
v1 version. In that version, libbpf used a retry loop to safely copy data
from the ring buffer that hadn’t been overwritten. By waking the consumer
once every half round, there was always a period where the consumer and
producer did not overlap, which helped reduce the number of retries.
I can't say I completely grok the logic here, but do you think we
should still keep this half-round wakeup? It looks like an arbitrary
heuristic, so I'd rather not have it.
Sure, since the related libbpf code is no longer present, I’ll remove this
logic in the next version.
pw-bot: cr
+ return false;
+}
+
+static __always_inline
we didn't have always_inline before, any strong reason to add it now?
I just wanted to avoid introducing any performance regression. Before this
patch, bpf_ringbuf_commit() was automatically inlined by the compiler, but
after the patch it wasn’t, so I added always_inline explicitly to keep it
inlined.
how big of a difference was it in benchmarks? It's generally frowned
upon using __always_inline without a good reason.
The difference is not noticeable on my arm64 test machine, but it is on my
amd machine.
Below is the benchmark data on AMD EPYC 9654, with and without always_inline
attribute.
- With always_inline
Ringbuf, multi-producer contention
==================================
rb-libbpf nr_prod 1 13.070 ± 0.158M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 2 15.440 ± 0.017M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 3 7.860 ± 0.003M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 4 6.444 ± 0.003M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 8 3.788 ± 0.005M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 12 2.802 ± 0.007M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 16 2.560 ± 0.003M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 20 2.227 ± 0.006M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 24 2.141 ± 0.007M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 28 1.960 ± 0.003M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 32 1.913 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 36 1.854 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 40 1.818 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 44 1.779 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 48 1.758 ± 0.003M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 52 1.812 ± 0.003M/s (drops 0.000 ± 0.000M/s)
- Without always_inline
Ringbuf, multi-producer contention
==================================
rb-libbpf nr_prod 1 10.550 ± 0.032M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 2 14.661 ± 0.024M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 3 7.616 ± 0.002M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 4 6.476 ± 0.002M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 8 3.806 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 12 2.814 ± 0.001M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 16 2.608 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 20 2.337 ± 0.005M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 24 2.270 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 28 1.977 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 32 1.921 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 36 1.862 ± 0.002M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 40 1.827 ± 0.004M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 44 1.912 ± 0.002M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 48 1.860 ± 0.002M/s (drops 0.000 ± 0.000M/s)
rb-libbpf nr_prod 52 1.824 ± 0.001M/s (drops 0.000 ± 0.000M/s)
When nr_prod=1, the performance regression is significant, dropping from
13.070 ± 0.158 M/s with always_inline to 10.550 ± 0.032 M/s without it.
However, since the half-round wakeup logic will be removed in the next
version, the changes to bpf_ringbuf_commit, including always_inline, will
also be removed.
[...]
