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(-)
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 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) * 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) * 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; + bpf_ringbuf_commit(sample, flags, false /* discard */); + return 0; }
const struct bpf_func_proto bpf_ringbuf_submit_proto = { .func = bpf_ringbuf_submit, - .ret_type = RET_VOID, + .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_ALLOC_MEM, .arg2_type = ARG_ANYTHING, };
BPF_CALL_2(bpf_ringbuf_discard, void *, sample, u64, flags) { + + if (unlikely(flags & ~(BPF_RB_NO_WAKEUP | BPF_RB_FORCE_WAKEUP))) + return -EINVAL; + bpf_ringbuf_commit(sample, flags, true /* discard */); + return 0; }
const struct bpf_func_proto bpf_ringbuf_discard_proto = { .func = bpf_ringbuf_discard, - .ret_type = RET_VOID, + .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_ALLOC_MEM, .arg2_type = ARG_ANYTHING, }; diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index 3d6d324184c0..d19c8c2688a2 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/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) * 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) * 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
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*, notificationof 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*, notificationof 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().
Thanks, Song
[...]
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*, notificationof 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*, notificationof 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*, notificationof 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*, notificationof 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:12 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)
DescriptionSubmit 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*, notificationof new data availability is sent unconditionally.Return
Nothing. Always succeeds.
bpf_ringbuf_submit/bpf_ringbuf_commit has to alway succeed. That's an explicit and strict rule, which BPF verifier relies on. We cannot bail out due to unknown flags, because then ringbuf sample won't ever be submitted and will block all the subsequent samples.
0 on success, or a negative error in case of failure.
[...]
'bpf_ring_buffer__poll()' abstracts the polling method, so abstract the constants that make the implementation don't wait or wait indefinetly for data.
Signed-off-by: Pedro Tammela pctammela@mojatatu.com --- tools/lib/bpf/libbpf.h | 3 +++ tools/testing/selftests/bpf/benchs/bench_ringbufs.c | 2 +- tools/testing/selftests/bpf/prog_tests/ringbuf.c | 6 +++--- tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 4 ++-- 4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index f500621d28e5..3817d84f91c6 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -540,6 +540,9 @@ LIBBPF_API int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms); LIBBPF_API int ring_buffer__consume(struct ring_buffer *rb); LIBBPF_API int ring_buffer__epoll_fd(const struct ring_buffer *rb);
+#define ring_buffer__poll_wait(rb) ring_buffer__poll(rb, -1) +#define ring_buffer__poll_nowait(rb) ring_buffer__poll(rb, 0) + /* Perf buffer APIs */ struct perf_buffer;
diff --git a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c index bde6c9d4cbd4..82db2cc9bab3 100644 --- a/tools/testing/selftests/bpf/benchs/bench_ringbufs.c +++ b/tools/testing/selftests/bpf/benchs/bench_ringbufs.c @@ -191,7 +191,7 @@ static void *ringbuf_libbpf_consumer(void *input) { struct ringbuf_libbpf_ctx *ctx = &ringbuf_libbpf_ctx;
- while (ring_buffer__poll(ctx->ringbuf, -1) >= 0) { + while (ring_buffer__poll_wait(ctx->ringbuf) >= 0) { if (args.back2back) bufs_trigger_batch(); } diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c index fddbc5db5d6a..321c646a0685 100644 --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c @@ -121,7 +121,7 @@ void test_ringbuf(void) 3L * rec_sz, skel->bss->prod_pos);
/* poll for samples */ - err = ring_buffer__poll(ringbuf, -1); + err = ring_buffer__poll_wait(ringbuf);
/* -EDONE is used as an indicator that we are done */ if (CHECK(err != -EDONE, "err_done", "done err: %d\n", err)) @@ -130,7 +130,7 @@ void test_ringbuf(void) CHECK(cnt != 2, "cnt", "exp %d samples, got %d\n", 2, cnt);
/* we expect extra polling to return nothing */ - err = ring_buffer__poll(ringbuf, 0); + err = ring_buffer__poll_nowait(ringbuf); if (CHECK(err != 0, "extra_samples", "poll result: %d\n", err)) goto cleanup; cnt = atomic_xchg(&sample_cnt, 0); @@ -148,7 +148,7 @@ void test_ringbuf(void) CHECK(skel->bss->cons_pos != 3 * rec_sz, "err_cons_pos", "exp %ld, got %ld\n", 3L * rec_sz, skel->bss->cons_pos); - err = ring_buffer__poll(ringbuf, -1); + err = ring_buffer__poll_wait(ringbuf); CHECK(err <= 0, "poll_err", "err %d\n", err); cnt = atomic_xchg(&sample_cnt, 0); CHECK(cnt != 2, "cnt", "exp %d samples, got %d\n", 2, cnt); diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c index d37161e59bb2..65ba0a3472f1 100644 --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c @@ -80,12 +80,12 @@ void test_ringbuf_multi(void) syscall(__NR_getpgid);
/* poll for samples, should get 2 ringbufs back */ - err = ring_buffer__poll(ringbuf, -1); + err = ring_buffer__poll_wait(ringbuf); if (CHECK(err != 2, "poll_res", "expected 2 records, got %d\n", err)) goto cleanup;
/* expect extra polling to return nothing */ - err = ring_buffer__poll(ringbuf, 0); + err = ring_buffer__poll_nowait(ringbuf); if (CHECK(err < 0, "extra_samples", "poll result: %d\n", err)) goto cleanup;
On Mar 28, 2021, at 9:10 AM, Pedro Tammela pctammela@gmail.com wrote:
'bpf_ring_buffer__poll()' abstracts the polling method, so abstract the constants that make the implementation don't wait or wait indefinetly for data.
Signed-off-by: Pedro Tammela pctammela@mojatatu.com
tools/lib/bpf/libbpf.h | 3 +++ tools/testing/selftests/bpf/benchs/bench_ringbufs.c | 2 +- tools/testing/selftests/bpf/prog_tests/ringbuf.c | 6 +++--- tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 4 ++-- 4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index f500621d28e5..3817d84f91c6 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -540,6 +540,9 @@ LIBBPF_API int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms); LIBBPF_API int ring_buffer__consume(struct ring_buffer *rb); LIBBPF_API int ring_buffer__epoll_fd(const struct ring_buffer *rb);
+#define ring_buffer__poll_wait(rb) ring_buffer__poll(rb, -1) +#define ring_buffer__poll_nowait(rb) ring_buffer__poll(rb, 0)
I think we don't need ring_buffer__poll_wait() as ring_buffer__poll() already means "wait for timeout_ms".
Actually, I think ring_buffer__poll() is enough. ring_buffer__poll_nowait() is not that useful either.
Thanks, Song
On Mon, Mar 29, 2021 at 9:28 AM Song Liu songliubraving@fb.com wrote:
On Mar 28, 2021, at 9:10 AM, Pedro Tammela pctammela@gmail.com wrote:
'bpf_ring_buffer__poll()' abstracts the polling method, so abstract the constants that make the implementation don't wait or wait indefinetly for data.
Signed-off-by: Pedro Tammela pctammela@mojatatu.com
tools/lib/bpf/libbpf.h | 3 +++ tools/testing/selftests/bpf/benchs/bench_ringbufs.c | 2 +- tools/testing/selftests/bpf/prog_tests/ringbuf.c | 6 +++--- tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c | 4 ++-- 4 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index f500621d28e5..3817d84f91c6 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -540,6 +540,9 @@ LIBBPF_API int ring_buffer__poll(struct ring_buffer *rb, int timeout_ms); LIBBPF_API int ring_buffer__consume(struct ring_buffer *rb); LIBBPF_API int ring_buffer__epoll_fd(const struct ring_buffer *rb);
+#define ring_buffer__poll_wait(rb) ring_buffer__poll(rb, -1) +#define ring_buffer__poll_nowait(rb) ring_buffer__poll(rb, 0)
I think we don't need ring_buffer__poll_wait() as ring_buffer__poll() already means "wait for timeout_ms".
Actually, I think ring_buffer__poll() is enough. ring_buffer__poll_nowait() is not that useful either.
I agree. I think adding a comment to the API itself might be useful specifying 0 and -1 as somewhat special cases.
Thanks, Song
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
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(-)
[...]
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.
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(-)
[...]
linux-kselftest-mirror@lists.linaro.org
-
Andrii Nakryiko -
Pedro Tammela -
Song Liu