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

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

The current code only checks flags in 'bpf_ringbuf_output()'.

Signed-off-by: Pedro Tammela <redacted>
---
 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

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

On Mar 28, 2021, at 9:10 AM, Pedro Tammela [off-list ref] 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 <redacted>

Acked-by: Song Liu <redacted>

This should be "long" instead of "int". 

Ditto. And same for tools/include/uapi/linux/bpf.h

We can move this check to bpf_ringbuf_commit(). 

Thanks,
Song

[...]

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

Em seg., 29 de mar. de 2021 às 13:10, Song Liu [off-list ref] escreveu:

quoted

On Mar 28, 2021, at 9:10 AM, Pedro Tammela [off-list ref] wrote:

The current code only checks flags in 'bpf_ringbuf_output()'.

Signed-off-by: Pedro Tammela <redacted>
---
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".

quoted

 *    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

quoted

 *    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 [off-list ref] wrote:

Em seg., 29 de mar. de 2021 às 13:10, Song Liu [off-list ref] escreveu:

quoted

quoted

On Mar 28, 2021, at 9:10 AM, Pedro Tammela [off-list ref] wrote:

The current code only checks flags in 'bpf_ringbuf_output()'.

Signed-off-by: Pedro Tammela <redacted>
---
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".

quoted

*    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

quoted

*    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 [off-list ref] 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 <redacted>
---

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 Sun, Mar 28, 2021 at 9:12 AM Pedro Tammela [off-list ref] wrote:

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

[...]

On Mon, Mar 29, 2021 at 9:28 AM Song Liu [off-list ref] wrote:

quoted

On Mar 28, 2021, at 9:10 AM, Pedro Tammela [off-list ref] 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 <redacted>
---
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

Em qua., 31 de mar. de 2021 às 03:54, Andrii Nakryiko
[off-list ref] escreveu:

On Sun, Mar 28, 2021 at 9:11 AM Pedro Tammela [off-list ref] wrote:

quoted

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

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.

quoted

 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 [off-list ref] wrote:

Em qua., 31 de mar. de 2021 às 03:54, Andrii Nakryiko
[off-list ref] escreveu:

quoted

On Sun, Mar 28, 2021 at 9:11 AM Pedro Tammela [off-list ref] wrote:

quoted

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

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!

quoted

quoted

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

[...]