Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers
@ 2021-04-06 18:58 Pedro Tammela
  2021-04-07 18:42 ` Joe Stringer
  0 siblings, 1 reply; 5+ messages in thread
From: Pedro Tammela @ 2021-04-06 18:58 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Joe Stringer, Quentin Monnet,
	open list:BPF (Safe dynamic programs and tools),
	open list:BPF (Safe dynamic programs and tools),
	open list
  Cc: 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


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers
  2021-04-06 18:58 [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers Pedro Tammela
@ 2021-04-07 18:42 ` Joe Stringer
  2021-04-07 19:58   ` Andrii Nakryiko
  0 siblings, 1 reply; 5+ messages in thread
From: Joe Stringer @ 2021-04-07 18:42 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
	Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
	KP Singh, Quentin Monnet,
	open list:BPF (Safe dynamic programs and tools),
	open list:BPF (Safe dynamic programs and tools),
	open list, Pedro Tammela

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers
  2021-04-07 18:42 ` Joe Stringer
@ 2021-04-07 19:58   ` Andrii Nakryiko
  2021-04-07 20:10     ` Pedro Tammela
  0 siblings, 1 reply; 5+ messages in thread
From: Andrii Nakryiko @ 2021-04-07 19:58 UTC (permalink / raw)
  To: Joe Stringer
  Cc: Pedro Tammela, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Quentin Monnet,
	open list:BPF (Safe dynamic programs and tools),
	open list:BPF (Safe dynamic programs and tools),
	open list, Pedro Tammela

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? ;)

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers
  2021-04-07 19:58   ` Andrii Nakryiko
@ 2021-04-07 20:10     ` Pedro Tammela
  2021-04-07 21:16       ` Andrii Nakryiko
  0 siblings, 1 reply; 5+ messages in thread
From: Pedro Tammela @ 2021-04-07 20:10 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Joe Stringer, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Quentin Monnet,
	open list:BPF (Safe dynamic programs and tools),
	open list:BPF (Safe dynamic programs and tools),
	open list, 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?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers
  2021-04-07 20:10     ` Pedro Tammela
@ 2021-04-07 21:16       ` Andrii Nakryiko
  0 siblings, 0 replies; 5+ messages in thread
From: Andrii Nakryiko @ 2021-04-07 21:16 UTC (permalink / raw)
  To: Pedro Tammela
  Cc: Joe Stringer, Alexei Starovoitov, Daniel Borkmann,
	Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
	John Fastabend, KP Singh, Quentin Monnet,
	open list:BPF (Safe dynamic programs and tools),
	open list:BPF (Safe dynamic programs and tools),
	open list, Pedro Tammela

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, back to index

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 18:58 [PATCH bpf-next] libbpf: clarify flags in ringbuf helpers Pedro Tammela
2021-04-07 18:42 ` Joe Stringer
2021-04-07 19:58   ` Andrii Nakryiko
2021-04-07 20:10     ` Pedro Tammela
2021-04-07 21:16       ` Andrii Nakryiko

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git