Re: [PATCH bpf-next] bpf: warn against BPF_RB_NO_WAKEUP in bpf_ringbuf_discard()

[Eyal Birger]

Hi,

On Mon, Mar 30, 2026 at 6:39 AM Puranjay Mohan <puranjay@xxxxxxxxxx> wrote:
>
> Eyal Birger <eyal.birger@xxxxxxxxx> writes:
>
> > Document that BPF_RB_NO_WAKEUP is not recommended for
> > bpf_ringbuf_discard().
> >
> > A discard done with BPF_RB_NO_WAKEUP can suppress a later adaptive
> > wakeup from a valid record, leaving an epoll-based userspace consumer
> > asleep even though data is available in the ring buffer.
> >
> > Scenario:
> >
> > epoll_wait(rb_fd);                     // blocks
> >
> > rec = bpf_ringbuf_reserve(&rb, ...);
> > bpf_ringbuf_discard(rec, BPF_RB_NO_WAKEUP);
> >
> > rec = bpf_ringbuf_reserve(&rb, ...);
> > bpf_ringbuf_submit(rec, 0);           // valid record, but no wakeup
> >
> > This behavior is surprising in the context of bpf_ringbuf_discard()
> > as it seems natural not to want to wake userspace.
>
> It appears that once you do a submit or discard with NO_WAKEUP, you can
> never go back to adaptive mode. You will need to do an explicit
> BPF_RB_FORCE_WAKEUP to do a wakeup.
>
> This looks like expected behaviour (by design) but for discard the
> programmer might think: "I'm discarding and there's no data, why would I
> wake anyone?  Let me pass BPF_RB_NO_WAKEUP to be a good citizen."

This is exactly what I've seen and it created real bugs.

>
> I am not sure if we just want to change the description of the helper or
> also fix the code to ignore the BPF_RB_NO_WAKEUP for discard?

I wanted to avoid breaking something that might be in use by advanced use-cases.

>
> Or a better approach would be to add a wakeup_needed flag to struct
> bpf_ringbuf. When NO_WAKEUP suppresses a wakeup that would have fired
> (cons_pos == rec_pos), it sets the flag. Any subsequent adaptive commit
> sees the flag and sends the wakeup, clearing it. FORCE_WAKEUP also
> clears it. But this could change how the ringbuf behaves for existing
> programs.

Considered this, but wanted to avoid a runtime penalty for this edge case.

>
> I will let others comment on this.

Thanks for your input!

Eyal.
>
> > Reported-by: Shmulik Ladkani <shmulik.ladkani@xxxxxxxxx>
> > Signed-off-by: Eyal Birger <eyal.birger@xxxxxxxxx>
> > ---
> >  include/uapi/linux/bpf.h       | 3 ++-
> >  tools/include/uapi/linux/bpf.h | 3 ++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index c8d400b7680a..c46b06d45904 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4645,7 +4645,8 @@ union bpf_attr {
> >   *   Description
> >   *           Discard reserved ring buffer sample, pointed to by *data*.
> >   *           If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification
> > - *           of new data availability is sent.
> > + *           of new data availability is sent, which is not recommended as
> > + *           it can suppress a later adaptive wakeup from a subsequent submit.
> >   *           If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
> >   *           of new data availability is sent unconditionally.
> >   *           If **0** is specified in *flags*, an adaptive notification
> > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > index 5e38b4887de6..96de37c3b896 100644
> > --- a/tools/include/uapi/linux/bpf.h
> > +++ b/tools/include/uapi/linux/bpf.h
> > @@ -4645,7 +4645,8 @@ union bpf_attr {
> >   *   Description
> >   *           Discard reserved ring buffer sample, pointed to by *data*.
> >   *           If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification
> > - *           of new data availability is sent.
> > + *           of new data availability is sent, which is not recommended as
> > + *           it can suppress a later adaptive wakeup from a subsequent submit.
> >   *           If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
> >   *           of new data availability is sent unconditionally.
> >   *           If **0** is specified in *flags*, an adaptive notification