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

[Puranjay Mohan]

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." 

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?

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.

I will let others comment on this.

> 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