Message-ID: <CAC1LvL1=61DYMAG=c57LRns++9rHF_thD3Kn=nopUoi8CkPshA@mail.gmail.com>
Date: Thu, 14 Aug 2025 14:20:20 -0700
From: Zvi Effron <zeffron@...tgames.com>
To: Eduard Zingerman <eddyz87@...il.com>
Cc: Xu Kuohai <xukuohai@...weicloud.com>, bpf@...r.kernel.org, 
	linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>, 
	Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau <martin.lau@...ux.dev>, Yonghong Song <yhs@...com>, 
	Song Liu <song@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>, 
	Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>, 
	Stanislav Fomichev <sdf@...ichev.me>, Willem de Bruijn <willemb@...gle.com>, 
	Jason Xing <kerneljasonxing@...il.com>, Paul Chaignon <paul.chaignon@...il.com>, 
	Tao Chen <chen.dylane@...ux.dev>, Kumar Kartikeya Dwivedi <memxor@...il.com>, 
	Martin Kelly <martin.kelly@...wdstrike.com>
Subject: Re: [PATCH bpf-next 2/4] libbpf: ringbuf: Add overwrite ring buffer process

On Thu, Aug 14, 2025 at 12:34 PM Eduard Zingerman <eddyz87@...il.com> wrote:
>
> On Mon, 2025-08-04 at 10:20 +0800, Xu Kuohai wrote:
>
> [...]
>
> > @@ -278,6 +293,92 @@ static int64_t ringbuf_process_ring(struct ring *r, size_t n)
> >       return cnt;
> >  }
> >
> > +static int64_t ringbuf_process_overwrite_ring(struct ring *r, size_t n)
> > +{
> > +
> > +     int err;
> > +     uint32_t *len_ptr, len;
> > +     /* 64-bit to avoid overflow in case of extreme application behavior */
> > +     int64_t cnt = 0;
> > +     size_t size, offset;
> > +     unsigned long cons_pos, prod_pos, over_pos, tmp_pos;
> > +     bool got_new_data;
> > +     void *sample;
> > +     bool copied;
> > +
> > +     size = r->mask + 1;
> > +
> > +     cons_pos = smp_load_acquire(r->consumer_pos);
> > +     do {
> > +             got_new_data = false;
> > +
> > +             /* grab a copy of data */
> > +             prod_pos = smp_load_acquire(r->producer_pos);
> > +             do {
> > +                     over_pos = READ_ONCE(*r->overwrite_pos);
> > +                     /* prod_pos may be outdated now */
> > +                     if (over_pos < prod_pos) {
> > +                             tmp_pos = max(cons_pos, over_pos);
> > +                             /* smp_load_acquire(r->producer_pos) before
> > +                              * READ_ONCE(*r->overwrite_pos) ensures that
> > +                              * over_pos + r->mask < prod_pos never occurs,
> > +                              * so size is never larger than r->mask
> > +                              */
> > +                             size = prod_pos - tmp_pos;
> > +                             if (!size)
> > +                                     goto done;
> > +                             memcpy(r->read_buffer,
> > +                                    r->data + (tmp_pos & r->mask), size);
> > +                             copied = true;
> > +                     } else {
> > +                             copied = false;
> > +                     }
> > +                     prod_pos = smp_load_acquire(r->producer_pos);
> > +             /* retry if data is overwritten by producer */
> > +             } while (!copied || prod_pos - tmp_pos > r->mask);
>
> Could you please elaborate a bit, why this condition is sufficient to
> guarantee that r->overwrite_pos had not changed while memcpy() was
> executing?
>

It isn't sufficient to guarantee that, but does it need tobe ? The concern is
that the data being memcpy-ed might have been overwritten, right? This
condition is sufficient to guarantee that can't happen without forcing another
loop iteration.

For the producer to overwrite a memcpy-ed byte, it must have looped around the
entire buffer, so r->producer_pos will be at least r->mask + 1 more than
tmp_pos. The +1 is because r->producer_pos first had to produce the byte
that got overwritten for it to be included in the memcpy, then produce it a
second time to overwrite it.

Since the code rereads r->producer_pos before making the check, if any bytes
have been overwritten, prod_pos - tmp_pos will be at least r->mask + 1, so the
check will return true and the loop will iterate again, and a new memcpy will
be performed.

> > +
> > +             cons_pos = tmp_pos;
> > +
> > +             for (offset = 0; offset < size; offset += roundup_len(len)) {
> > +                     len_ptr = r->read_buffer + (offset & r->mask);
> > +                     len = *len_ptr;
> > +
> > +                     if (len & BPF_RINGBUF_BUSY_BIT)
> > +                             goto done;
> > +
> > +                     got_new_data = true;
> > +                     cons_pos += roundup_len(len);
> > +
> > +                     if ((len & BPF_RINGBUF_DISCARD_BIT) == 0) {
> > +                             sample = (void *)len_ptr + BPF_RINGBUF_HDR_SZ;
> > +                             err = r->sample_cb(r->ctx, sample, len);
> > +                             if (err < 0) {
> > +                                     /* update consumer pos and bail out */
> > +                                     smp_store_release(r->consumer_pos,
> > +                                                       cons_pos);
> > +                                     return err;
> > +                             }
> > +                             cnt++;
> > +                     }
> > +
> > +                     if (cnt >= n)
> > +                             goto done;
> > +             }
> > +     } while (got_new_data);
> > +
> > +done:
> > +     smp_store_release(r->consumer_pos, cons_pos);
> > +     return cnt;
> > +}
>
> [...]
>

Powered by blists - more mailing lists