[PATCH bpf-next] libbpf: clarify flags in ringbuf helpers

[PATCH bpf-next] libbpf: clarify flags in ringbuf helpers

[Pedro Tammela]

In 'bpf_ringbuf_reserve()' we require the flag to '0' at the moment.

For 'bpf_ringbuf_{discard,submit,output}' a flag of '0' might send a
notification to the process if needed.

Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
---
 include/uapi/linux/bpf.h       | 7 +++++++
 tools/include/uapi/linux/bpf.h | 7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 49371eba98ba..8c5c7a893b87 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -4061,12 +4061,15 @@ union bpf_attr {
  * 		of new data availability is sent.
  * 		If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
  * 		of new data availability is sent unconditionally.
+ * 		If **0** is specified in *flags*, notification
+ * 		of new data availability is sent if needed.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
  * void *bpf_ringbuf_reserve(void *ringbuf, u64 size, u64 flags)
  * 	Description
  * 		Reserve *size* bytes of payload in a ring buffer *ringbuf*.
+ * 		*flags* must be 0.
  * 	Return
  * 		Valid pointer with *size* bytes of memory available; NULL,
  * 		otherwise.
@@ -4078,6 +4081,8 @@ union bpf_attr {
  * 		of new data availability is sent.
  * 		If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
  * 		of new data availability is sent unconditionally.
+ * 		If **0** is specified in *flags*, notification
+ * 		of new data availability is sent if needed.
  * 	Return
  * 		Nothing. Always succeeds.
  *
@@ -4088,6 +4093,8 @@ union bpf_attr {
  * 		of new data availability is sent.
  * 		If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
  * 		of new data availability is sent unconditionally.
+ * 		If **0** is specified in *flags*, notification
+ * 		of new data availability is sent if needed.
  * 	Return
  * 		Nothing. Always succeeds.
  *
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 69902603012c..51df1bd45cef 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -4061,12 +4061,15 @@ union bpf_attr {
  * 		of new data availability is sent.
  * 		If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
  * 		of new data availability is sent unconditionally.
+ * 		If **0** is specified in *flags*, notification
+ * 		of new data availability is sent if needed.
  * 	Return
  * 		0 on success, or a negative error in case of failure.
  *
  * void *bpf_ringbuf_reserve(void *ringbuf, u64 size, u64 flags)
  * 	Description
  * 		Reserve *size* bytes of payload in a ring buffer *ringbuf*.
+ * 		*flags* must be 0.
  * 	Return
  * 		Valid pointer with *size* bytes of memory available; NULL,
  * 		otherwise.
@@ -4078,6 +4081,8 @@ union bpf_attr {
  * 		of new data availability is sent.
  * 		If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
  * 		of new data availability is sent unconditionally.
+ * 		If **0** is specified in *flags*, notification
+ * 		of new data availability is sent if needed.
  * 	Return
  * 		Nothing. Always succeeds.
  *
@@ -4088,6 +4093,8 @@ union bpf_attr {
  * 		of new data availability is sent.
  * 		If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
  * 		of new data availability is sent unconditionally.
+ * 		If **0** is specified in *flags*, notification
+ * 		of new data availability is sent if needed.
  * 	Return
  * 		Nothing. Always succeeds.
  *
-- 
2.25.1

Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers

[Joe Stringer]

Hi Pedro,

On Tue, Apr 6, 2021 at 11:58 AM Pedro Tammela <pctammela@gmail.com> wrote:
>
> In 'bpf_ringbuf_reserve()' we require the flag to '0' at the moment.
>
> For 'bpf_ringbuf_{discard,submit,output}' a flag of '0' might send a
> notification to the process if needed.
>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> ---
>  include/uapi/linux/bpf.h       | 7 +++++++
>  tools/include/uapi/linux/bpf.h | 7 +++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 49371eba98ba..8c5c7a893b87 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -4061,12 +4061,15 @@ union bpf_attr {
>   *             of new data availability is sent.
>   *             If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
>   *             of new data availability is sent unconditionally.
> + *             If **0** is specified in *flags*, notification
> + *             of new data availability is sent if needed.

Maybe a trivial question, but what does "if needed" mean? Does that
mean "when the buffer is full"?

Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers

[Andrii Nakryiko]

On Wed, Apr 7, 2021 at 11:43 AM Joe Stringer <joe@cilium.io> wrote:
>
> Hi Pedro,
>
> On Tue, Apr 6, 2021 at 11:58 AM Pedro Tammela <pctammela@gmail.com> wrote:
> >
> > In 'bpf_ringbuf_reserve()' we require the flag to '0' at the moment.
> >
> > For 'bpf_ringbuf_{discard,submit,output}' a flag of '0' might send a
> > notification to the process if needed.
> >
> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> > ---
> >  include/uapi/linux/bpf.h       | 7 +++++++
> >  tools/include/uapi/linux/bpf.h | 7 +++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 49371eba98ba..8c5c7a893b87 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -4061,12 +4061,15 @@ union bpf_attr {
> >   *             of new data availability is sent.
> >   *             If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
> >   *             of new data availability is sent unconditionally.
> > + *             If **0** is specified in *flags*, notification
> > + *             of new data availability is sent if needed.
>
> Maybe a trivial question, but what does "if needed" mean? Does that
> mean "when the buffer is full"?

I used to call it ns "adaptive notification", so maybe let's use that
term instead of "if needed"? It means that in kernel BPF ringbuf code
will check if the user-space consumer has caught up and consumed all
the available data. In that case user-space might be waiting
(sleeping) in epoll_wait() already and not processing samples
actively. That means that we have to send notification, otherwise
user-space might never wake up. But if the kernel sees that user-space
is still processing previous record (consumer position < producer
position), then we can bypass sending another notification, because
user-space consumer protocol dictates that it needs to consume all the
record until consumer position == producer position. So no
notification is necessary for the newly submitted sample, as
user-space will eventually see it without notification.

Of course there is careful writes and memory ordering involved to make
sure that we never miss notification.

Does someone want to try to condense it into a succinct description? ;)

Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers

[Pedro Tammela]

Em qua., 7 de abr. de 2021 às 16:58, Andrii Nakryiko
<andrii.nakryiko@gmail.com> escreveu:
>
> On Wed, Apr 7, 2021 at 11:43 AM Joe Stringer <joe@cilium.io> wrote:
> >
> > Hi Pedro,
> >
> > On Tue, Apr 6, 2021 at 11:58 AM Pedro Tammela <pctammela@gmail.com> wrote:
> > >
> > > In 'bpf_ringbuf_reserve()' we require the flag to '0' at the moment.
> > >
> > > For 'bpf_ringbuf_{discard,submit,output}' a flag of '0' might send a
> > > notification to the process if needed.
> > >
> > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> > > ---
> > >  include/uapi/linux/bpf.h       | 7 +++++++
> > >  tools/include/uapi/linux/bpf.h | 7 +++++++
> > >  2 files changed, 14 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 49371eba98ba..8c5c7a893b87 100644
> > > --- a/include/uapi/linux/bpf.h
> > > +++ b/include/uapi/linux/bpf.h
> > > @@ -4061,12 +4061,15 @@ union bpf_attr {
> > >   *             of new data availability is sent.
> > >   *             If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
> > >   *             of new data availability is sent unconditionally.
> > > + *             If **0** is specified in *flags*, notification
> > > + *             of new data availability is sent if needed.
> >
> > Maybe a trivial question, but what does "if needed" mean? Does that
> > mean "when the buffer is full"?
>
> I used to call it ns "adaptive notification", so maybe let's use that
> term instead of "if needed"? It means that in kernel BPF ringbuf code
> will check if the user-space consumer has caught up and consumed all
> the available data. In that case user-space might be waiting
> (sleeping) in epoll_wait() already and not processing samples
> actively. That means that we have to send notification, otherwise
> user-space might never wake up. But if the kernel sees that user-space
> is still processing previous record (consumer position < producer
> position), then we can bypass sending another notification, because
> user-space consumer protocol dictates that it needs to consume all the
> record until consumer position == producer position. So no
> notification is necessary for the newly submitted sample, as
> user-space will eventually see it without notification.
>
> Of course there is careful writes and memory ordering involved to make
> sure that we never miss notification.
>
> Does someone want to try to condense it into a succinct description? ;)

OK.

I can try to condense this and perhaps add it as code in the comment?

Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers

[Andrii Nakryiko]

On Wed, Apr 7, 2021 at 1:10 PM Pedro Tammela <pctammela@gmail.com> wrote:
>
> Em qua., 7 de abr. de 2021 às 16:58, Andrii Nakryiko
> <andrii.nakryiko@gmail.com> escreveu:
> >
> > On Wed, Apr 7, 2021 at 11:43 AM Joe Stringer <joe@cilium.io> wrote:
> > >
> > > Hi Pedro,
> > >
> > > On Tue, Apr 6, 2021 at 11:58 AM Pedro Tammela <pctammela@gmail.com> wrote:
> > > >
> > > > In 'bpf_ringbuf_reserve()' we require the flag to '0' at the moment.
> > > >
> > > > For 'bpf_ringbuf_{discard,submit,output}' a flag of '0' might send a
> > > > notification to the process if needed.
> > > >
> > > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> > > > ---
> > > >  include/uapi/linux/bpf.h       | 7 +++++++
> > > >  tools/include/uapi/linux/bpf.h | 7 +++++++
> > > >  2 files changed, 14 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > > index 49371eba98ba..8c5c7a893b87 100644
> > > > --- a/include/uapi/linux/bpf.h
> > > > +++ b/include/uapi/linux/bpf.h
> > > > @@ -4061,12 +4061,15 @@ union bpf_attr {
> > > >   *             of new data availability is sent.
> > > >   *             If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification
> > > >   *             of new data availability is sent unconditionally.
> > > > + *             If **0** is specified in *flags*, notification
> > > > + *             of new data availability is sent if needed.
> > >
> > > Maybe a trivial question, but what does "if needed" mean? Does that
> > > mean "when the buffer is full"?
> >
> > I used to call it ns "adaptive notification", so maybe let's use that
> > term instead of "if needed"? It means that in kernel BPF ringbuf code
> > will check if the user-space consumer has caught up and consumed all
> > the available data. In that case user-space might be waiting
> > (sleeping) in epoll_wait() already and not processing samples
> > actively. That means that we have to send notification, otherwise
> > user-space might never wake up. But if the kernel sees that user-space
> > is still processing previous record (consumer position < producer
> > position), then we can bypass sending another notification, because
> > user-space consumer protocol dictates that it needs to consume all the
> > record until consumer position == producer position. So no
> > notification is necessary for the newly submitted sample, as
> > user-space will eventually see it without notification.
> >
> > Of course there is careful writes and memory ordering involved to make
> > sure that we never miss notification.
> >
> > Does someone want to try to condense it into a succinct description? ;)
>
> OK.
>
> I can try to condense this and perhaps add it as code in the comment?

Sure, though there is already a brief comment to that effect. But
having high-level explanation in uapi/linux/bpf.h would be great for
users, though.