| Message ID | 20210328161055.257504-1-pctammela@mojatatu.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag | expand |
> On Mar 28, 2021, at 9:10 AM, Pedro Tammela <pctammela@gmail.com> wrote: > > The current way to provide a no-op flag to 'bpf_ringbuf_submit()', > 'bpf_ringbuf_discard()' and 'bpf_ringbuf_output()' is to provide a '0' > value. > > A '0' value might notify the consumer if it already caught up in processing, > so let's provide a more descriptive notation for this value. > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> Acked-by: Song Liu <songliubraving@fb.com>
Em seg., 29 de mar. de 2021 às 13:10, Song Liu <songliubraving@fb.com> escreveu: > > > > > On Mar 28, 2021, at 9:10 AM, Pedro Tammela <pctammela@gmail.com> wrote: > > > > The current code only checks flags in 'bpf_ringbuf_output()'. > > > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > > --- > > include/uapi/linux/bpf.h | 8 ++++---- > > kernel/bpf/ringbuf.c | 13 +++++++++++-- > > tools/include/uapi/linux/bpf.h | 8 ++++---- > > 3 files changed, 19 insertions(+), 10 deletions(-) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 100cb2e4c104..232b5e5dd045 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -4073,7 +4073,7 @@ union bpf_attr { > > * Valid pointer with *size* bytes of memory available; NULL, > > * otherwise. > > * > > - * void bpf_ringbuf_submit(void *data, u64 flags) > > + * int bpf_ringbuf_submit(void *data, u64 flags) > > This should be "long" instead of "int". > > > * Description > > * Submit reserved ring buffer sample, pointed to by *data*. > > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > > @@ -4083,9 +4083,9 @@ union bpf_attr { > > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > > * of new data availability is sent unconditionally. > > * Return > > - * Nothing. Always succeeds. > > + * 0 on success, or a negative error in case of failure. > > * > > - * void bpf_ringbuf_discard(void *data, u64 flags) > > + * int bpf_ringbuf_discard(void *data, u64 flags) > > Ditto. And same for tools/include/uapi/linux/bpf.h > > > * Description > > * Discard reserved ring buffer sample, pointed to by *data*. > > * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification > > @@ -4095,7 +4095,7 @@ union bpf_attr { > > * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification > > * of new data availability is sent unconditionally. > > * Return > > - * Nothing. Always succeeds. > > + * 0 on success, or a negative error in case of failure. > > * > > * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) > > * Description > > diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c > > index f25b719ac786..f76dafe2427e 100644 > > --- a/kernel/bpf/ringbuf.c > > +++ b/kernel/bpf/ringbuf.c > > @@ -397,26 +397,35 @@ static void bpf_ringbuf_commit(void *sample, u64 flags, bool discard) > > > > BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) > > { > > + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) > > + return -EINVAL; > > We can move this check to bpf_ringbuf_commit(). I don't believe we can because in 'bpf_ringbuf_output()' the flag checking in 'bpf_ringbuf_commit()' is already too late. > > Thanks, > Song > > [...] Pedro
> On Mar 30, 2021, at 7:22 AM, Pedro Tammela <pctammela@gmail.com> wrote: > > Em seg., 29 de mar. de 2021 às 13:10, Song Liu <songliubraving@fb.com> escreveu: >> >> >> >>> On Mar 28, 2021, at 9:10 AM, Pedro Tammela <pctammela@gmail.com> wrote: >>> >>> The current code only checks flags in 'bpf_ringbuf_output()'. >>> >>> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> >>> --- >>> include/uapi/linux/bpf.h | 8 ++++---- >>> kernel/bpf/ringbuf.c | 13 +++++++++++-- >>> tools/include/uapi/linux/bpf.h | 8 ++++---- >>> 3 files changed, 19 insertions(+), 10 deletions(-) >>> >>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h >>> index 100cb2e4c104..232b5e5dd045 100644 >>> --- a/include/uapi/linux/bpf.h >>> +++ b/include/uapi/linux/bpf.h >>> @@ -4073,7 +4073,7 @@ union bpf_attr { >>> * Valid pointer with *size* bytes of memory available; NULL, >>> * otherwise. >>> * >>> - * void bpf_ringbuf_submit(void *data, u64 flags) >>> + * int bpf_ringbuf_submit(void *data, u64 flags) >> >> This should be "long" instead of "int". >> >>> * Description >>> * Submit reserved ring buffer sample, pointed to by *data*. >>> * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification >>> @@ -4083,9 +4083,9 @@ union bpf_attr { >>> * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification >>> * of new data availability is sent unconditionally. >>> * Return >>> - * Nothing. Always succeeds. >>> + * 0 on success, or a negative error in case of failure. >>> * >>> - * void bpf_ringbuf_discard(void *data, u64 flags) >>> + * int bpf_ringbuf_discard(void *data, u64 flags) >> >> Ditto. And same for tools/include/uapi/linux/bpf.h >> >>> * Description >>> * Discard reserved ring buffer sample, pointed to by *data*. >>> * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification >>> @@ -4095,7 +4095,7 @@ union bpf_attr { >>> * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification >>> * of new data availability is sent unconditionally. >>> * Return >>> - * Nothing. Always succeeds. >>> + * 0 on success, or a negative error in case of failure. >>> * >>> * u64 bpf_ringbuf_query(void *ringbuf, u64 flags) >>> * Description >>> diff --git a/kernel/bpf/ringbuf.c b/kernel/bpf/ringbuf.c >>> index f25b719ac786..f76dafe2427e 100644 >>> --- a/kernel/bpf/ringbuf.c >>> +++ b/kernel/bpf/ringbuf.c >>> @@ -397,26 +397,35 @@ static void bpf_ringbuf_commit(void *sample, u64 flags, bool discard) >>> >>> BPF_CALL_2(bpf_ringbuf_submit, void *, sample, u64, flags) >>> { >>> + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) >>> + return -EINVAL; >> >> We can move this check to bpf_ringbuf_commit(). > > I don't believe we can because in 'bpf_ringbuf_output()' the flag > checking in 'bpf_ringbuf_commit()' is already > too late. I see. Let's keep it in current functions then. Thanks, Song
On Sun, Mar 28, 2021 at 9:11 AM Pedro Tammela <pctammela@gmail.com> wrote: > > The current way to provide a no-op flag to 'bpf_ringbuf_submit()', > 'bpf_ringbuf_discard()' and 'bpf_ringbuf_output()' is to provide a '0' > value. > > A '0' value might notify the consumer if it already caught up in processing, > so let's provide a more descriptive notation for this value. > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > --- flags == 0 means "no extra modifiers of behavior". That's default adaptive notification. If you want to adjust default behavior, only then you specify non-zero flags. I don't think anyone will bother typing BPF_RB_MAY_WAKEUP for this, nor I think it's really needed. The documentation update is nice (if no flags are specified notification will be sent if needed), but the new "pseudo-flag" seems like an overkill to me. > include/uapi/linux/bpf.h | 8 ++++++++ > tools/include/uapi/linux/bpf.h | 8 ++++++++ > tools/testing/selftests/bpf/progs/ima.c | 2 +- > tools/testing/selftests/bpf/progs/ringbuf_bench.c | 2 +- > tools/testing/selftests/bpf/progs/test_ringbuf.c | 2 +- > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 2 +- > 6 files changed, 20 insertions(+), 4 deletions(-) > [...]
On Sat, Apr 3, 2021 at 6:34 AM Pedro Tammela <pctammela@gmail.com> wrote: > > Em qua., 31 de mar. de 2021 às 03:54, Andrii Nakryiko > <andrii.nakryiko@gmail.com> escreveu: > > > > On Sun, Mar 28, 2021 at 9:11 AM Pedro Tammela <pctammela@gmail.com> wrote: > > > > > > The current way to provide a no-op flag to 'bpf_ringbuf_submit()', > > > 'bpf_ringbuf_discard()' and 'bpf_ringbuf_output()' is to provide a '0' > > > value. > > > > > > A '0' value might notify the consumer if it already caught up in processing, > > > so let's provide a more descriptive notation for this value. > > > > > > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> > > > --- > > > > flags == 0 means "no extra modifiers of behavior". That's default > > adaptive notification. If you want to adjust default behavior, only > > then you specify non-zero flags. I don't think anyone will bother > > typing BPF_RB_MAY_WAKEUP for this, nor I think it's really needed. The > > documentation update is nice (if no flags are specified notification > > will be sent if needed), but the new "pseudo-flag" seems like an > > overkill to me. > > My intention here is to make '0' more descriptive. > But if you think just the documentation update is enough, then I will > remove the flag. flags == 0 means "default behavior", I don't think you have to remember which verbose flag you need to specify for that, so I think just expanding documentation is sufficient and better. Thanks! > > > > > > include/uapi/linux/bpf.h | 8 ++++++++ > > > tools/include/uapi/linux/bpf.h | 8 ++++++++ > > > tools/testing/selftests/bpf/progs/ima.c | 2 +- > > > tools/testing/selftests/bpf/progs/ringbuf_bench.c | 2 +- > > > tools/testing/selftests/bpf/progs/test_ringbuf.c | 2 +- > > > tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 2 +- > > > 6 files changed, 20 insertions(+), 4 deletions(-) > > > > > > > [...]
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 598716742593..100cb2e4c104 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -4058,6 +4058,8 @@ union bpf_attr { * Copy *size* bytes from *data* into a ring buffer *ringbuf*. * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification * of new data availability is sent. + * If **BPF_RB_MAY_WAKEUP** is specified in *flags*, notification + * of new data availability is sent if needed. * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return @@ -4066,6 +4068,7 @@ union bpf_attr { * 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. @@ -4075,6 +4078,8 @@ union bpf_attr { * Submit 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. + * If **BPF_RB_MAY_WAKEUP** is specified in *flags*, notification + * of new data availability is sent if needed. * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return @@ -4085,6 +4090,8 @@ union bpf_attr { * 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. + * If **BPF_RB_MAY_WAKEUP** is specified in *flags*, notification + * of new data availability is sent if needed. * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return @@ -4965,6 +4972,7 @@ enum { * BPF_FUNC_bpf_ringbuf_output flags. */ enum { + BPF_RB_MAY_WAKEUP = 0, BPF_RB_NO_WAKEUP = (1ULL << 0), BPF_RB_FORCE_WAKEUP = (1ULL << 1), }; diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index ab9f2233607c..3d6d324184c0 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -4058,6 +4058,8 @@ union bpf_attr { * Copy *size* bytes from *data* into a ring buffer *ringbuf*. * If **BPF_RB_NO_WAKEUP** is specified in *flags*, no notification * of new data availability is sent. + * If **BPF_RB_MAY_WAKEUP** is specified in *flags*, notification + * of new data availability is sent if needed. * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return @@ -4066,6 +4068,7 @@ union bpf_attr { * 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. @@ -4075,6 +4078,8 @@ union bpf_attr { * Submit 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. + * If **BPF_RB_MAY_WAKEUP** is specified in *flags*, notification + * of new data availability is sent if needed. * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return @@ -4085,6 +4090,8 @@ union bpf_attr { * 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. + * If **BPF_RB_MAY_WAKEUP** is specified in *flags*, notification + * of new data availability is sent if needed. * If **BPF_RB_FORCE_WAKEUP** is specified in *flags*, notification * of new data availability is sent unconditionally. * Return @@ -4959,6 +4966,7 @@ enum { * BPF_FUNC_bpf_ringbuf_output flags. */ enum { + BPF_RB_MAY_WAKEUP = 0, BPF_RB_NO_WAKEUP = (1ULL << 0), BPF_RB_FORCE_WAKEUP = (1ULL << 1), }; diff --git a/tools/testing/selftests/bpf/progs/ima.c b/tools/testing/selftests/bpf/progs/ima.c index 96060ff4ffc6..0f4daced6aad 100644 --- a/tools/testing/selftests/bpf/progs/ima.c +++ b/tools/testing/selftests/bpf/progs/ima.c @@ -38,7 +38,7 @@ void BPF_PROG(ima, struct linux_binprm *bprm) return; *sample = ima_hash; - bpf_ringbuf_submit(sample, 0); + bpf_ringbuf_submit(sample, BPF_RB_MAY_WAKEUP); } return; diff --git a/tools/testing/selftests/bpf/progs/ringbuf_bench.c b/tools/testing/selftests/bpf/progs/ringbuf_bench.c index 123607d314d6..808e2e0e3d64 100644 --- a/tools/testing/selftests/bpf/progs/ringbuf_bench.c +++ b/tools/testing/selftests/bpf/progs/ringbuf_bench.c @@ -24,7 +24,7 @@ static __always_inline long get_flags() long sz; if (!wakeup_data_size) - return 0; + return BPF_RB_MAY_WAKEUP; sz = bpf_ringbuf_query(&ringbuf, BPF_RB_AVAIL_DATA); return sz >= wakeup_data_size ? BPF_RB_FORCE_WAKEUP : BPF_RB_NO_WAKEUP; diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c index 8ba9959b036b..03a5cbd21356 100644 --- a/tools/testing/selftests/bpf/progs/test_ringbuf.c +++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c @@ -21,7 +21,7 @@ struct { /* inputs */ int pid = 0; long value = 0; -long flags = 0; +long flags = BPF_RB_MAY_WAKEUP; /* outputs */ long total = 0; diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c index edf3b6953533..f33c3fdfb1d6 100644 --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c @@ -71,7 +71,7 @@ int test_ringbuf(void *ctx) sample->seq = total; total += 1; - bpf_ringbuf_submit(sample, 0); + bpf_ringbuf_submit(sample, BPF_RB_MAY_WAKEUP); return 0; }
The current way to provide a no-op flag to 'bpf_ringbuf_submit()', 'bpf_ringbuf_discard()' and 'bpf_ringbuf_output()' is to provide a '0' value. A '0' value might notify the consumer if it already caught up in processing, so let's provide a more descriptive notation for this value. Signed-off-by: Pedro Tammela <pctammela@mojatatu.com> --- include/uapi/linux/bpf.h | 8 ++++++++ tools/include/uapi/linux/bpf.h | 8 ++++++++ tools/testing/selftests/bpf/progs/ima.c | 2 +- tools/testing/selftests/bpf/progs/ringbuf_bench.c | 2 +- tools/testing/selftests/bpf/progs/test_ringbuf.c | 2 +- tools/testing/selftests/bpf/progs/test_ringbuf_multi.c | 2 +- 6 files changed, 20 insertions(+), 4 deletions(-)