| Message ID | 20210406185806.377576-1-pctammela@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] libbpf: clarify flags in ringbuf helpers | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 12 of 12 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 11983 this patch: 11983 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: From:/Signed-off-by: email address mismatch: 'From: Pedro Tammela <pctammela@gmail.com>' != 'Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>' WARNING: please, no space before tabs |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 12470 this patch: 12470 |
| netdev/header_inline | success | Link |
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"?
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? ;)
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?
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.
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. *
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(+)