[PATCH bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag

[PATCH bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag

[Pedro Tammela]

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;
 }
-- 
2.25.1

[PATCH bpf-next] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()'

[Pedro Tammela]

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
-- 
2.25.1

[PATCH bpf-next] libbpf: Add '_wait()' and '_nowait()' macros for 'bpf_ring_buffer__poll()'

[Pedro Tammela]

'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;
 
-- 
2.25.1

Re: [PATCH bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag

[Song Liu]

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

Re: [PATCH bpf-next] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()'

[Song Liu]

> 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(). 

Thanks,
Song

[...]

Re: [PATCH bpf-next] libbpf: Add '_wait()' and '_nowait()' macros for 'bpf_ring_buffer__poll()'

[Song Liu]

> 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

Re: [PATCH bpf-next] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()'

[Pedro Tammela]

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

Re: [PATCH bpf-next] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()'

[Song Liu]

> 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

Re: [PATCH bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag

[Andrii Nakryiko]

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(-)
>

[...]

Re: [PATCH bpf-next] bpf: check flags in 'bpf_ringbuf_discard()' and 'bpf_ringbuf_submit()'

[Andrii Nakryiko]

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)
>   *     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.

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.
>   *

[...]

Re: [PATCH bpf-next] libbpf: Add '_wait()' and '_nowait()' macros for 'bpf_ring_buffer__poll()'

[Andrii Nakryiko]

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
>

Re: [PATCH bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag

[Pedro Tammela]

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(-)
> >
>
> [...]

Re: [PATCH bpf-next] bpf: add 'BPF_RB_MAY_WAKEUP' flag

[Andrii Nakryiko]

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(-)
> > >
> >
> > [...]