[PATCH 0/3] bpf/selftests: page size fixes

[PATCH 0/3] bpf/selftests: page size fixes

[Yauheni Kaliuta]

A set of fixes for selftests to make them working on systems with PAGE_SIZE > 4K

2 questions left:

- about `nit: if (!ASSERT_OK(err, "setsockopt_attach"))`. I left
  CHECK() for now since otherwise it has too many negations. But
  should I anyway use ASSERT?

- https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/prog_tests/mmap.c#L41
  and below -- it works now as is, but should be switched also to page_size?

Yauheni Kaliuta (3):
  selftests/bpf: test_progs/sockopt_sk: pass page size from userspace
  bpf: selftests: test_progs/sockopt_sk: remove version
  selftests/bpf: ringbuf, mmap: bump up page size to 64K

 tools/testing/selftests/bpf/prog_tests/ringbuf.c      |  9 +++++++--
 tools/testing/selftests/bpf/prog_tests/sockopt_sk.c   |  2 ++
 tools/testing/selftests/bpf/progs/map_ptr_kern.c      |  9 +++++++--
 tools/testing/selftests/bpf/progs/sockopt_sk.c        | 11 ++++-------
 tools/testing/selftests/bpf/progs/test_mmap.c         | 10 ++++++++--
 tools/testing/selftests/bpf/progs/test_ringbuf.c      |  8 +++++++-
 .../testing/selftests/bpf/progs/test_ringbuf_multi.c  |  7 ++++++-
 7 files changed, 41 insertions(+), 15 deletions(-)

-- 
2.29.2

[PATCH 1/3] selftests/bpf: test_progs/sockopt_sk: pass page size from userspace

[Yauheni Kaliuta]

Since there is no convenient way for bpf program to get PAGE_SIZE
from inside of the kernel, pass the value from userspace.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/sockopt_sk.c |  2 ++
 tools/testing/selftests/bpf/progs/sockopt_sk.c      | 10 ++++------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
index 114c1a622ffa..6a7cb5f23db2 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
@@ -201,6 +201,8 @@ static void run_test(int cgroup_fd)
 	if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
 		goto cleanup;
 
+	skel->bss->page_size = getpagesize();
+
 	skel->links._setsockopt =
 		bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);
 	if (CHECK(IS_ERR(skel->links._setsockopt),
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index d3597f81e6e9..55dfbe53c24e 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -8,9 +8,7 @@
 char _license[] SEC("license") = "GPL";
 __u32 _version SEC("version") = 1;
 
-#ifndef PAGE_SIZE
-#define PAGE_SIZE 4096
-#endif
+int page_size; /* userspace should set it */
 
 #ifndef SOL_TCP
 #define SOL_TCP IPPROTO_TCP
@@ -90,7 +88,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
 		 * program can only see the first PAGE_SIZE
 		 * bytes of data.
 		 */
-		if (optval_end - optval != PAGE_SIZE)
+		if (optval_end - optval != page_size)
 			return 0; /* EPERM, unexpected data size */
 
 		return 1;
@@ -161,7 +159,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
 
 	if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
 		/* Original optlen is larger than PAGE_SIZE. */
-		if (ctx->optlen != PAGE_SIZE * 2)
+		if (ctx->optlen != page_size * 2)
 			return 0; /* EPERM, unexpected data size */
 
 		if (optval + 1 > optval_end)
@@ -175,7 +173,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
 		 * program can only see the first PAGE_SIZE
 		 * bytes of data.
 		 */
-		if (optval_end - optval != PAGE_SIZE)
+		if (optval_end - optval != page_size)
 			return 0; /* EPERM, unexpected data size */
 
 		return 1;
-- 
2.29.2

[PATCH 2/3] bpf: selftests: test_progs/sockopt_sk: remove version

[Yauheni Kaliuta]

As pointed by Andrii Nakryiko, _version is useless now, remove it.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/progs/sockopt_sk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index 55dfbe53c24e..d6d03f64e2e4 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -6,7 +6,6 @@
 #include <bpf/bpf_helpers.h>
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1;
 
 int page_size; /* userspace should set it */
 
-- 
2.29.2

[PATCH 3/3] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Yauheni Kaliuta]

Both ringbuf and mmap need PAGE_SIZE, but it's not available during
bpf program compile time. 4K size was hardcoded (page shift 12 bits)
which makes the tests fail on systems, configured for larger pages.

Bump it up to 64K which at the first glance look reasonable at the
moment for most of the systems.

Use define to make it clear.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/ringbuf.c       |  9 +++++++--
 tools/testing/selftests/bpf/progs/map_ptr_kern.c       |  9 +++++++--
 tools/testing/selftests/bpf/progs/test_mmap.c          | 10 ++++++++--
 tools/testing/selftests/bpf/progs/test_ringbuf.c       |  8 +++++++-
 tools/testing/selftests/bpf/progs/test_ringbuf_multi.c |  7 ++++++-
 5 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index fddbc5db5d6a..9057654da957 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -15,6 +15,11 @@
 #include "test_ringbuf.skel.h"
 
 #define EDONE 7777
+#ifdef PAGE_SIZE
+#undef PAGE_SIZE
+#endif
+/* this is not actual page size, but the value used for ringbuf */
+#define PAGE_SIZE 65536
 
 static int duration = 0;
 
@@ -110,9 +115,9 @@ void test_ringbuf(void)
 	CHECK(skel->bss->avail_data != 3 * rec_sz,
 	      "err_avail_size", "exp %ld, got %ld\n",
 	      3L * rec_sz, skel->bss->avail_data);
-	CHECK(skel->bss->ring_size != 4096,
+	CHECK(skel->bss->ring_size != PAGE_SIZE,
 	      "err_ring_size", "exp %ld, got %ld\n",
-	      4096L, skel->bss->ring_size);
+	      (long)PAGE_SIZE, skel->bss->ring_size);
 	CHECK(skel->bss->cons_pos != 0,
 	      "err_cons_pos", "exp %ld, got %ld\n",
 	      0L, skel->bss->cons_pos);
diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
index d8850bc6a9f1..c1460f27af78 100644
--- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
+++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
@@ -8,6 +8,11 @@
 #define MAX_ENTRIES 8
 #define HALF_ENTRIES (MAX_ENTRIES >> 1)
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
 _Static_assert(MAX_ENTRIES < LOOP_BOUND, "MAX_ENTRIES must be < LOOP_BOUND");
 
 enum bpf_map_type g_map_type = BPF_MAP_TYPE_UNSPEC;
@@ -635,7 +640,7 @@ struct bpf_ringbuf_map {
 
 struct {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
-	__uint(max_entries, 1 << 12);
+	__uint(max_entries, PAGE_SIZE);
 } m_ringbuf SEC(".maps");
 
 static inline int check_ringbuf(void)
@@ -643,7 +648,7 @@ static inline int check_ringbuf(void)
 	struct bpf_ringbuf_map *ringbuf = (struct bpf_ringbuf_map *)&m_ringbuf;
 	struct bpf_map *map = (struct bpf_map *)&m_ringbuf;
 
-	VERIFY(check(&ringbuf->map, map, 0, 0, 1 << 12));
+	VERIFY(check(&ringbuf->map, map, 0, 0, PAGE_SIZE));
 
 	return 1;
 }
diff --git a/tools/testing/selftests/bpf/progs/test_mmap.c b/tools/testing/selftests/bpf/progs/test_mmap.c
index 4eb42cff5fe9..c22fcfea0767 100644
--- a/tools/testing/selftests/bpf/progs/test_mmap.c
+++ b/tools/testing/selftests/bpf/progs/test_mmap.c
@@ -5,11 +5,16 @@
 #include <stdint.h>
 #include <bpf/bpf_helpers.h>
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
 char _license[] SEC("license") = "GPL";
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 4096);
+	__uint(max_entries, PAGE_SIZE);
 	__uint(map_flags, BPF_F_MMAPABLE | BPF_F_RDONLY_PROG);
 	__type(key, __u32);
 	__type(value, char);
@@ -17,7 +22,8 @@ struct {
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 512 * 4); /* at least 4 pages of data */
+	/* at least 4 pages of data */
+	__uint(max_entries, 4 * (PAGE_SIZE / sizeof (__u64)));
 	__uint(map_flags, BPF_F_MMAPABLE);
 	__type(key, __u32);
 	__type(value, __u64);
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c
index 8ba9959b036b..6e645babdc18 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c
@@ -4,6 +4,12 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
+
 char _license[] SEC("license") = "GPL";
 
 struct sample {
@@ -15,7 +21,7 @@ struct sample {
 
 struct {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
-	__uint(max_entries, 1 << 12);
+	__uint(max_entries, PAGE_SIZE);
 } ringbuf SEC(".maps");
 
 /* inputs */
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
index edf3b6953533..13bcf095e06c 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
@@ -4,6 +4,11 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
 char _license[] SEC("license") = "GPL";
 
 struct sample {
@@ -15,7 +20,7 @@ struct sample {
 
 struct ringbuf_map {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
-	__uint(max_entries, 1 << 12);
+	__uint(max_entries, PAGE_SIZE);
 } ringbuf1 SEC(".maps"),
   ringbuf2 SEC(".maps");
 
-- 
2.29.2

Re: [PATCH 0/3] bpf/selftests: page size fixes

[Yauheni Kaliuta]

Oops, one patch is missing.

Will recend

On Fri, Mar 26, 2021 at 1:47 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> A set of fixes for selftests to make them working on systems with PAGE_SIZE > 4K
>
> 2 questions left:
>
> - about `nit: if (!ASSERT_OK(err, "setsockopt_attach"))`. I left
>   CHECK() for now since otherwise it has too many negations. But
>   should I anyway use ASSERT?
>
> - https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/prog_tests/mmap.c#L41
>   and below -- it works now as is, but should be switched also to page_size?
>
> Yauheni Kaliuta (3):
>   selftests/bpf: test_progs/sockopt_sk: pass page size from userspace
>   bpf: selftests: test_progs/sockopt_sk: remove version
>   selftests/bpf: ringbuf, mmap: bump up page size to 64K
>
>  tools/testing/selftests/bpf/prog_tests/ringbuf.c      |  9 +++++++--
>  tools/testing/selftests/bpf/prog_tests/sockopt_sk.c   |  2 ++
>  tools/testing/selftests/bpf/progs/map_ptr_kern.c      |  9 +++++++--
>  tools/testing/selftests/bpf/progs/sockopt_sk.c        | 11 ++++-------
>  tools/testing/selftests/bpf/progs/test_mmap.c         | 10 ++++++++--
>  tools/testing/selftests/bpf/progs/test_ringbuf.c      |  8 +++++++-
>  .../testing/selftests/bpf/progs/test_ringbuf_multi.c  |  7 ++++++-
>  7 files changed, 41 insertions(+), 15 deletions(-)
>
> --
> 2.29.2
>


-- 
WBR, Yauheni

[PATCH v2 0/4] bpf/selftests: page size fixes

[Yauheni Kaliuta]

A set of fixes for selftests to make them working on systems with PAGE_SIZE > 4K

2 questions left:

- about `nit: if (!ASSERT_OK(err, "setsockopt_attach"))`. I left
  CHECK() for now since otherwise it has too many negations. But
  should I anyway use ASSERT?

- https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/prog_tests/mmap.c#L41
  and below -- it works now as is, but should be switched also to page_size?

--
v1->v2:

- add missed 'selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton'

Yauheni Kaliuta (4):
  
  selftests/bpf: test_progs/sockopt_sk: pass page size from userspace
  bpf: selftests: test_progs/sockopt_sk: remove version
  selftests/bpf: ringbuf, mmap: bump up page size to 64K

 .../selftests/bpf/prog_tests/ringbuf.c        |  9 ++-
 .../selftests/bpf/prog_tests/sockopt_sk.c     | 68 ++++++-------------
 .../selftests/bpf/progs/map_ptr_kern.c        |  9 ++-
 .../testing/selftests/bpf/progs/sockopt_sk.c  | 11 ++-
 tools/testing/selftests/bpf/progs/test_mmap.c | 10 ++-
 .../selftests/bpf/progs/test_ringbuf.c        |  8 ++-
 .../selftests/bpf/progs/test_ringbuf_multi.c  |  7 +-
 7 files changed, 61 insertions(+), 61 deletions(-)

-- 
2.29.2

[PATCH v2 1/4] selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton

[Yauheni Kaliuta]

Switch the test to use BPF skeleton to save some boilerplate and
make it easy to access bpf program bss segment.

The latter will be used to pass PAGE_SIZE from userspace since there
is no convenient way for bpf program to get it from inside of the
kernel.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 .../selftests/bpf/prog_tests/sockopt_sk.c     | 72 ++++++-------------
 1 file changed, 23 insertions(+), 49 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
index d5b44b135c00..114c1a622ffa 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
@@ -3,6 +3,7 @@
 #include "cgroup_helpers.h"
 
 #include <linux/tcp.h>
+#include "sockopt_sk.skel.h"
 
 #ifndef SOL_TCP
 #define SOL_TCP IPPROTO_TCP
@@ -191,60 +192,33 @@ static int getsetsockopt(void)
 	return -1;
 }
 
-static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title)
-{
-	enum bpf_attach_type attach_type;
-	enum bpf_prog_type prog_type;
-	struct bpf_program *prog;
-	int err;
-
-	err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
-	if (err) {
-		log_err("Failed to deduct types for %s BPF program", title);
-		return -1;
-	}
-
-	prog = bpf_object__find_program_by_title(obj, title);
-	if (!prog) {
-		log_err("Failed to find %s BPF program", title);
-		return -1;
-	}
-
-	err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
-			      attach_type, 0);
-	if (err) {
-		log_err("Failed to attach %s BPF program", title);
-		return -1;
-	}
-
-	return 0;
-}
-
 static void run_test(int cgroup_fd)
 {
-	struct bpf_prog_load_attr attr = {
-		.file = "./sockopt_sk.o",
-	};
-	struct bpf_object *obj;
-	int ignored;
-	int err;
-
-	err = bpf_prog_load_xattr(&attr, &obj, &ignored);
-	if (CHECK_FAIL(err))
-		return;
-
-	err = prog_attach(obj, cgroup_fd, "cgroup/getsockopt");
-	if (CHECK_FAIL(err))
-		goto close_bpf_object;
-
-	err = prog_attach(obj, cgroup_fd, "cgroup/setsockopt");
-	if (CHECK_FAIL(err))
-		goto close_bpf_object;
+	struct sockopt_sk *skel;
+	int duration = 0;
+
+	skel = sockopt_sk__open_and_load();
+	if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
+		goto cleanup;
+
+	skel->links._setsockopt =
+		bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);
+	if (CHECK(IS_ERR(skel->links._setsockopt),
+			 "setsockopt_attach", "err: %ld\n",
+			 PTR_ERR(skel->links._setsockopt)))
+		goto cleanup;
+
+	skel->links._getsockopt =
+		bpf_program__attach_cgroup(skel->progs._getsockopt, cgroup_fd);
+	if (CHECK(IS_ERR(skel->links._getsockopt),
+			 "getsockopt_attach", "err: %ld\n",
+			 PTR_ERR(skel->links._getsockopt)))
+		goto cleanup;
 
 	CHECK_FAIL(getsetsockopt());
 
-close_bpf_object:
-	bpf_object__close(obj);
+cleanup:
+	sockopt_sk__destroy(skel);
 }
 
 void test_sockopt_sk(void)
-- 
2.29.2

[PATCH v2 2/4] selftests/bpf: test_progs/sockopt_sk: pass page size from userspace

[Yauheni Kaliuta]

Since there is no convenient way for bpf program to get PAGE_SIZE
from inside of the kernel, pass the value from userspace.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/sockopt_sk.c |  2 ++
 tools/testing/selftests/bpf/progs/sockopt_sk.c      | 10 ++++------
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
index 114c1a622ffa..6a7cb5f23db2 100644
--- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
@@ -201,6 +201,8 @@ static void run_test(int cgroup_fd)
 	if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
 		goto cleanup;
 
+	skel->bss->page_size = getpagesize();
+
 	skel->links._setsockopt =
 		bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);
 	if (CHECK(IS_ERR(skel->links._setsockopt),
diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index d3597f81e6e9..55dfbe53c24e 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -8,9 +8,7 @@
 char _license[] SEC("license") = "GPL";
 __u32 _version SEC("version") = 1;
 
-#ifndef PAGE_SIZE
-#define PAGE_SIZE 4096
-#endif
+int page_size; /* userspace should set it */
 
 #ifndef SOL_TCP
 #define SOL_TCP IPPROTO_TCP
@@ -90,7 +88,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
 		 * program can only see the first PAGE_SIZE
 		 * bytes of data.
 		 */
-		if (optval_end - optval != PAGE_SIZE)
+		if (optval_end - optval != page_size)
 			return 0; /* EPERM, unexpected data size */
 
 		return 1;
@@ -161,7 +159,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
 
 	if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
 		/* Original optlen is larger than PAGE_SIZE. */
-		if (ctx->optlen != PAGE_SIZE * 2)
+		if (ctx->optlen != page_size * 2)
 			return 0; /* EPERM, unexpected data size */
 
 		if (optval + 1 > optval_end)
@@ -175,7 +173,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
 		 * program can only see the first PAGE_SIZE
 		 * bytes of data.
 		 */
-		if (optval_end - optval != PAGE_SIZE)
+		if (optval_end - optval != page_size)
 			return 0; /* EPERM, unexpected data size */
 
 		return 1;
-- 
2.29.2

[PATCH v2 3/4] bpf: selftests: test_progs/sockopt_sk: remove version

[Yauheni Kaliuta]

As pointed by Andrii Nakryiko, _version is useless now, remove it.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/progs/sockopt_sk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
index 55dfbe53c24e..d6d03f64e2e4 100644
--- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
+++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
@@ -6,7 +6,6 @@
 #include <bpf/bpf_helpers.h>
 
 char _license[] SEC("license") = "GPL";
-__u32 _version SEC("version") = 1;
 
 int page_size; /* userspace should set it */
 
-- 
2.29.2

[PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Yauheni Kaliuta]

Both ringbuf and mmap need PAGE_SIZE, but it's not available during
bpf program compile time. 4K size was hardcoded (page shift 12 bits)
which makes the tests fail on systems, configured for larger pages.

Bump it up to 64K which at the first glance look reasonable at the
moment for most of the systems.

Use define to make it clear.

Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
---
 tools/testing/selftests/bpf/prog_tests/ringbuf.c       |  9 +++++++--
 tools/testing/selftests/bpf/progs/map_ptr_kern.c       |  9 +++++++--
 tools/testing/selftests/bpf/progs/test_mmap.c          | 10 ++++++++--
 tools/testing/selftests/bpf/progs/test_ringbuf.c       |  8 +++++++-
 tools/testing/selftests/bpf/progs/test_ringbuf_multi.c |  7 ++++++-
 5 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
index fddbc5db5d6a..9057654da957 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
@@ -15,6 +15,11 @@
 #include "test_ringbuf.skel.h"
 
 #define EDONE 7777
+#ifdef PAGE_SIZE
+#undef PAGE_SIZE
+#endif
+/* this is not actual page size, but the value used for ringbuf */
+#define PAGE_SIZE 65536
 
 static int duration = 0;
 
@@ -110,9 +115,9 @@ void test_ringbuf(void)
 	CHECK(skel->bss->avail_data != 3 * rec_sz,
 	      "err_avail_size", "exp %ld, got %ld\n",
 	      3L * rec_sz, skel->bss->avail_data);
-	CHECK(skel->bss->ring_size != 4096,
+	CHECK(skel->bss->ring_size != PAGE_SIZE,
 	      "err_ring_size", "exp %ld, got %ld\n",
-	      4096L, skel->bss->ring_size);
+	      (long)PAGE_SIZE, skel->bss->ring_size);
 	CHECK(skel->bss->cons_pos != 0,
 	      "err_cons_pos", "exp %ld, got %ld\n",
 	      0L, skel->bss->cons_pos);
diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
index d8850bc6a9f1..c1460f27af78 100644
--- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
+++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
@@ -8,6 +8,11 @@
 #define MAX_ENTRIES 8
 #define HALF_ENTRIES (MAX_ENTRIES >> 1)
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
 _Static_assert(MAX_ENTRIES < LOOP_BOUND, "MAX_ENTRIES must be < LOOP_BOUND");
 
 enum bpf_map_type g_map_type = BPF_MAP_TYPE_UNSPEC;
@@ -635,7 +640,7 @@ struct bpf_ringbuf_map {
 
 struct {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
-	__uint(max_entries, 1 << 12);
+	__uint(max_entries, PAGE_SIZE);
 } m_ringbuf SEC(".maps");
 
 static inline int check_ringbuf(void)
@@ -643,7 +648,7 @@ static inline int check_ringbuf(void)
 	struct bpf_ringbuf_map *ringbuf = (struct bpf_ringbuf_map *)&m_ringbuf;
 	struct bpf_map *map = (struct bpf_map *)&m_ringbuf;
 
-	VERIFY(check(&ringbuf->map, map, 0, 0, 1 << 12));
+	VERIFY(check(&ringbuf->map, map, 0, 0, PAGE_SIZE));
 
 	return 1;
 }
diff --git a/tools/testing/selftests/bpf/progs/test_mmap.c b/tools/testing/selftests/bpf/progs/test_mmap.c
index 4eb42cff5fe9..c22fcfea0767 100644
--- a/tools/testing/selftests/bpf/progs/test_mmap.c
+++ b/tools/testing/selftests/bpf/progs/test_mmap.c
@@ -5,11 +5,16 @@
 #include <stdint.h>
 #include <bpf/bpf_helpers.h>
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
 char _license[] SEC("license") = "GPL";
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 4096);
+	__uint(max_entries, PAGE_SIZE);
 	__uint(map_flags, BPF_F_MMAPABLE | BPF_F_RDONLY_PROG);
 	__type(key, __u32);
 	__type(value, char);
@@ -17,7 +22,8 @@ struct {
 
 struct {
 	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 512 * 4); /* at least 4 pages of data */
+	/* at least 4 pages of data */
+	__uint(max_entries, 4 * (PAGE_SIZE / sizeof (__u64)));
 	__uint(map_flags, BPF_F_MMAPABLE);
 	__type(key, __u32);
 	__type(value, __u64);
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c
index 8ba9959b036b..6e645babdc18 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c
@@ -4,6 +4,12 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
+
 char _license[] SEC("license") = "GPL";
 
 struct sample {
@@ -15,7 +21,7 @@ struct sample {
 
 struct {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
-	__uint(max_entries, 1 << 12);
+	__uint(max_entries, PAGE_SIZE);
 } ringbuf SEC(".maps");
 
 /* inputs */
diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
index edf3b6953533..13bcf095e06c 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
@@ -4,6 +4,11 @@
 #include <linux/bpf.h>
 #include <bpf/bpf_helpers.h>
 
+#ifndef PAGE_SIZE
+/* use reasonable value for various configurations */
+#define PAGE_SIZE 65536
+#endif
+
 char _license[] SEC("license") = "GPL";
 
 struct sample {
@@ -15,7 +20,7 @@ struct sample {
 
 struct ringbuf_map {
 	__uint(type, BPF_MAP_TYPE_RINGBUF);
-	__uint(max_entries, 1 << 12);
+	__uint(max_entries, PAGE_SIZE);
 } ringbuf1 SEC(".maps"),
   ringbuf2 SEC(".maps");
 
-- 
2.29.2

Re: [PATCH v2 1/4] selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton

[Andrii Nakryiko]

On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Switch the test to use BPF skeleton to save some boilerplate and
> make it easy to access bpf program bss segment.
>
> The latter will be used to pass PAGE_SIZE from userspace since there
> is no convenient way for bpf program to get it from inside of the
> kernel.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---

It's a nice cleanup. See some suggestion to further clean it up. But
even with this:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>  .../selftests/bpf/prog_tests/sockopt_sk.c     | 72 ++++++-------------
>  1 file changed, 23 insertions(+), 49 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> index d5b44b135c00..114c1a622ffa 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> @@ -3,6 +3,7 @@
>  #include "cgroup_helpers.h"
>
>  #include <linux/tcp.h>
> +#include "sockopt_sk.skel.h"
>
>  #ifndef SOL_TCP
>  #define SOL_TCP IPPROTO_TCP
> @@ -191,60 +192,33 @@ static int getsetsockopt(void)
>         return -1;
>  }
>
> -static int prog_attach(struct bpf_object *obj, int cgroup_fd, const char *title)
> -{
> -       enum bpf_attach_type attach_type;
> -       enum bpf_prog_type prog_type;
> -       struct bpf_program *prog;
> -       int err;
> -
> -       err = libbpf_prog_type_by_name(title, &prog_type, &attach_type);
> -       if (err) {
> -               log_err("Failed to deduct types for %s BPF program", title);
> -               return -1;
> -       }
> -
> -       prog = bpf_object__find_program_by_title(obj, title);
> -       if (!prog) {
> -               log_err("Failed to find %s BPF program", title);
> -               return -1;
> -       }
> -
> -       err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
> -                             attach_type, 0);
> -       if (err) {
> -               log_err("Failed to attach %s BPF program", title);
> -               return -1;
> -       }
> -
> -       return 0;
> -}
> -
>  static void run_test(int cgroup_fd)
>  {
> -       struct bpf_prog_load_attr attr = {
> -               .file = "./sockopt_sk.o",
> -       };
> -       struct bpf_object *obj;
> -       int ignored;
> -       int err;
> -
> -       err = bpf_prog_load_xattr(&attr, &obj, &ignored);
> -       if (CHECK_FAIL(err))
> -               return;
> -
> -       err = prog_attach(obj, cgroup_fd, "cgroup/getsockopt");
> -       if (CHECK_FAIL(err))
> -               goto close_bpf_object;
> -
> -       err = prog_attach(obj, cgroup_fd, "cgroup/setsockopt");
> -       if (CHECK_FAIL(err))
> -               goto close_bpf_object;
> +       struct sockopt_sk *skel;
> +       int duration = 0;
> +
> +       skel = sockopt_sk__open_and_load();
> +       if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
> +               goto cleanup;

if (!ASSERT_OK_PTR(skel, "skel_load))

is still less boilerplate :)

> +
> +       skel->links._setsockopt =
> +               bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);
> +       if (CHECK(IS_ERR(skel->links._setsockopt),
> +                        "setsockopt_attach", "err: %ld\n",
> +                        PTR_ERR(skel->links._setsockopt)))

you could save some more boilerplate if you did:

if (!ASSERT_OK_PTR(skel->links._getsockopt), "getsockopt_link")
    goto cleanup;

> +               goto cleanup;
> +
> +       skel->links._getsockopt =
> +               bpf_program__attach_cgroup(skel->progs._getsockopt, cgroup_fd);
> +       if (CHECK(IS_ERR(skel->links._getsockopt),
> +                        "getsockopt_attach", "err: %ld\n",
> +                        PTR_ERR(skel->links._getsockopt)))
> +               goto cleanup;
>
>         CHECK_FAIL(getsetsockopt());

please switch this to ASSERT_OK() as well.

Once you don't use CHECK/CHECK_FAIL, you also won't need `int duration`, btw.

>
> -close_bpf_object:
> -       bpf_object__close(obj);
> +cleanup:
> +       sockopt_sk__destroy(skel);
>  }
>
>  void test_sockopt_sk(void)
> --
> 2.29.2
>

Re: [PATCH v2 2/4] selftests/bpf: test_progs/sockopt_sk: pass page size from userspace

[Andrii Nakryiko]

On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Since there is no convenient way for bpf program to get PAGE_SIZE
> from inside of the kernel, pass the value from userspace.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---

Acked-by: Andrii Nakryiko <andrii@kernel.org>

But I'd also shorten the subject to:

selftests/bpf: pass page size from userspace in sockopt_sk

It's just as clear and doesn't include unnecessary prog_tests/ path.


>  tools/testing/selftests/bpf/prog_tests/sockopt_sk.c |  2 ++
>  tools/testing/selftests/bpf/progs/sockopt_sk.c      | 10 ++++------
>  2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> index 114c1a622ffa..6a7cb5f23db2 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sockopt_sk.c
> @@ -201,6 +201,8 @@ static void run_test(int cgroup_fd)
>         if (CHECK(!skel, "skel_load", "sockopt_sk skeleton failed\n"))
>                 goto cleanup;
>
> +       skel->bss->page_size = getpagesize();
> +
>         skel->links._setsockopt =
>                 bpf_program__attach_cgroup(skel->progs._setsockopt, cgroup_fd);
>         if (CHECK(IS_ERR(skel->links._setsockopt),
> diff --git a/tools/testing/selftests/bpf/progs/sockopt_sk.c b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> index d3597f81e6e9..55dfbe53c24e 100644
> --- a/tools/testing/selftests/bpf/progs/sockopt_sk.c
> +++ b/tools/testing/selftests/bpf/progs/sockopt_sk.c
> @@ -8,9 +8,7 @@
>  char _license[] SEC("license") = "GPL";
>  __u32 _version SEC("version") = 1;
>
> -#ifndef PAGE_SIZE
> -#define PAGE_SIZE 4096
> -#endif
> +int page_size; /* userspace should set it */
>
>  #ifndef SOL_TCP
>  #define SOL_TCP IPPROTO_TCP
> @@ -90,7 +88,7 @@ int _getsockopt(struct bpf_sockopt *ctx)
>                  * program can only see the first PAGE_SIZE
>                  * bytes of data.
>                  */
> -               if (optval_end - optval != PAGE_SIZE)
> +               if (optval_end - optval != page_size)
>                         return 0; /* EPERM, unexpected data size */
>
>                 return 1;
> @@ -161,7 +159,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
>
>         if (ctx->level == SOL_IP && ctx->optname == IP_FREEBIND) {
>                 /* Original optlen is larger than PAGE_SIZE. */
> -               if (ctx->optlen != PAGE_SIZE * 2)
> +               if (ctx->optlen != page_size * 2)
>                         return 0; /* EPERM, unexpected data size */
>
>                 if (optval + 1 > optval_end)
> @@ -175,7 +173,7 @@ int _setsockopt(struct bpf_sockopt *ctx)
>                  * program can only see the first PAGE_SIZE
>                  * bytes of data.
>                  */
> -               if (optval_end - optval != PAGE_SIZE)
> +               if (optval_end - optval != page_size)
>                         return 0; /* EPERM, unexpected data size */
>
>                 return 1;
> --
> 2.29.2
>

Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Andrii Nakryiko]

On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Both ringbuf and mmap need PAGE_SIZE, but it's not available during
> bpf program compile time. 4K size was hardcoded (page shift 12 bits)
> which makes the tests fail on systems, configured for larger pages.
>
> Bump it up to 64K which at the first glance look reasonable at the
> moment for most of the systems.
>
> Use define to make it clear.
>
> Signed-off-by: Yauheni Kaliuta <yauheni.kaliuta@redhat.com>
> ---
>  tools/testing/selftests/bpf/prog_tests/ringbuf.c       |  9 +++++++--
>  tools/testing/selftests/bpf/progs/map_ptr_kern.c       |  9 +++++++--
>  tools/testing/selftests/bpf/progs/test_mmap.c          | 10 ++++++++--
>  tools/testing/selftests/bpf/progs/test_ringbuf.c       |  8 +++++++-
>  tools/testing/selftests/bpf/progs/test_ringbuf_multi.c |  7 ++++++-
>  5 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf.c b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> index fddbc5db5d6a..9057654da957 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf.c
> @@ -15,6 +15,11 @@
>  #include "test_ringbuf.skel.h"
>
>  #define EDONE 7777
> +#ifdef PAGE_SIZE
> +#undef PAGE_SIZE
> +#endif
> +/* this is not actual page size, but the value used for ringbuf */
> +#define PAGE_SIZE 65536
>
>  static int duration = 0;
>
> @@ -110,9 +115,9 @@ void test_ringbuf(void)
>         CHECK(skel->bss->avail_data != 3 * rec_sz,
>               "err_avail_size", "exp %ld, got %ld\n",
>               3L * rec_sz, skel->bss->avail_data);
> -       CHECK(skel->bss->ring_size != 4096,
> +       CHECK(skel->bss->ring_size != PAGE_SIZE,
>               "err_ring_size", "exp %ld, got %ld\n",
> -             4096L, skel->bss->ring_size);
> +             (long)PAGE_SIZE, skel->bss->ring_size);
>         CHECK(skel->bss->cons_pos != 0,
>               "err_cons_pos", "exp %ld, got %ld\n",
>               0L, skel->bss->cons_pos);
> diff --git a/tools/testing/selftests/bpf/progs/map_ptr_kern.c b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
> index d8850bc6a9f1..c1460f27af78 100644
> --- a/tools/testing/selftests/bpf/progs/map_ptr_kern.c
> +++ b/tools/testing/selftests/bpf/progs/map_ptr_kern.c
> @@ -8,6 +8,11 @@
>  #define MAX_ENTRIES 8
>  #define HALF_ENTRIES (MAX_ENTRIES >> 1)
>
> +#ifndef PAGE_SIZE
> +/* use reasonable value for various configurations */
> +#define PAGE_SIZE 65536
> +#endif
> +
>  _Static_assert(MAX_ENTRIES < LOOP_BOUND, "MAX_ENTRIES must be < LOOP_BOUND");
>
>  enum bpf_map_type g_map_type = BPF_MAP_TYPE_UNSPEC;
> @@ -635,7 +640,7 @@ struct bpf_ringbuf_map {
>
>  struct {
>         __uint(type, BPF_MAP_TYPE_RINGBUF);
> -       __uint(max_entries, 1 << 12);
> +       __uint(max_entries, PAGE_SIZE);
>  } m_ringbuf SEC(".maps");
>
>  static inline int check_ringbuf(void)
> @@ -643,7 +648,7 @@ static inline int check_ringbuf(void)
>         struct bpf_ringbuf_map *ringbuf = (struct bpf_ringbuf_map *)&m_ringbuf;
>         struct bpf_map *map = (struct bpf_map *)&m_ringbuf;
>
> -       VERIFY(check(&ringbuf->map, map, 0, 0, 1 << 12));
> +       VERIFY(check(&ringbuf->map, map, 0, 0, PAGE_SIZE));
>
>         return 1;
>  }
> diff --git a/tools/testing/selftests/bpf/progs/test_mmap.c b/tools/testing/selftests/bpf/progs/test_mmap.c
> index 4eb42cff5fe9..c22fcfea0767 100644
> --- a/tools/testing/selftests/bpf/progs/test_mmap.c
> +++ b/tools/testing/selftests/bpf/progs/test_mmap.c
> @@ -5,11 +5,16 @@
>  #include <stdint.h>
>  #include <bpf/bpf_helpers.h>
>
> +#ifndef PAGE_SIZE
> +/* use reasonable value for various configurations */
> +#define PAGE_SIZE 65536
> +#endif
> +
>  char _license[] SEC("license") = "GPL";
>
>  struct {
>         __uint(type, BPF_MAP_TYPE_ARRAY);
> -       __uint(max_entries, 4096);
> +       __uint(max_entries, PAGE_SIZE);


so you can set map size at runtime before bpf_object__load (or
skeleton's load) with bpf_map__set_max_entries. That way you don't
have to do any assumptions. Just omit max_entries in BPF source code,
and always set it in userspace.

>         __uint(map_flags, BPF_F_MMAPABLE | BPF_F_RDONLY_PROG);
>         __type(key, __u32);
>         __type(value, char);
> @@ -17,7 +22,8 @@ struct {
>
>  struct {
>         __uint(type, BPF_MAP_TYPE_ARRAY);
> -       __uint(max_entries, 512 * 4); /* at least 4 pages of data */
> +       /* at least 4 pages of data */
> +       __uint(max_entries, 4 * (PAGE_SIZE / sizeof (__u64)));
>         __uint(map_flags, BPF_F_MMAPABLE);
>         __type(key, __u32);
>         __type(value, __u64);
> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf.c b/tools/testing/selftests/bpf/progs/test_ringbuf.c
> index 8ba9959b036b..6e645babdc18 100644
> --- a/tools/testing/selftests/bpf/progs/test_ringbuf.c
> +++ b/tools/testing/selftests/bpf/progs/test_ringbuf.c
> @@ -4,6 +4,12 @@
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
>
> +#ifndef PAGE_SIZE
> +/* use reasonable value for various configurations */
> +#define PAGE_SIZE 65536
> +#endif
> +
> +
>  char _license[] SEC("license") = "GPL";
>
>  struct sample {
> @@ -15,7 +21,7 @@ struct sample {
>
>  struct {
>         __uint(type, BPF_MAP_TYPE_RINGBUF);
> -       __uint(max_entries, 1 << 12);
> +       __uint(max_entries, PAGE_SIZE);
>  } ringbuf SEC(".maps");
>
>  /* inputs */
> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> index edf3b6953533..13bcf095e06c 100644
> --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> @@ -4,6 +4,11 @@
>  #include <linux/bpf.h>
>  #include <bpf/bpf_helpers.h>
>
> +#ifndef PAGE_SIZE
> +/* use reasonable value for various configurations */
> +#define PAGE_SIZE 65536
> +#endif
> +
>  char _license[] SEC("license") = "GPL";
>
>  struct sample {
> @@ -15,7 +20,7 @@ struct sample {
>
>  struct ringbuf_map {
>         __uint(type, BPF_MAP_TYPE_RINGBUF);
> -       __uint(max_entries, 1 << 12);
> +       __uint(max_entries, PAGE_SIZE);
>  } ringbuf1 SEC(".maps"),
>    ringbuf2 SEC(".maps");
>
> --
> 2.29.2
>

Re: [PATCH v2 0/4] bpf/selftests: page size fixes

[Andrii Nakryiko]

On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> A set of fixes for selftests to make them working on systems with PAGE_SIZE > 4K
>
> 2 questions left:
>
> - about `nit: if (!ASSERT_OK(err, "setsockopt_attach"))`. I left
>   CHECK() for now since otherwise it has too many negations. But
>   should I anyway use ASSERT?

CHECK itself is a negation as much more confusing, IMO. if
(!ASSERT_OK(err)) is pretty clear, as for me.

>
> - https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/prog_tests/mmap.c#L41
>   and below -- it works now as is, but should be switched also to page_size?

replied on another patch, it is possible to set all that at runtime
with bpf_map__set_max_entries().


Overall, please specify the [PATCH bpf-next] prefix to denote that it
targets bpf-next.


>
> --
> v1->v2:
>
> - add missed 'selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton'
>
> Yauheni Kaliuta (4):
>
>   selftests/bpf: test_progs/sockopt_sk: pass page size from userspace
>   bpf: selftests: test_progs/sockopt_sk: remove version
>   selftests/bpf: ringbuf, mmap: bump up page size to 64K
>
>  .../selftests/bpf/prog_tests/ringbuf.c        |  9 ++-
>  .../selftests/bpf/prog_tests/sockopt_sk.c     | 68 ++++++-------------
>  .../selftests/bpf/progs/map_ptr_kern.c        |  9 ++-
>  .../testing/selftests/bpf/progs/sockopt_sk.c  | 11 ++-
>  tools/testing/selftests/bpf/progs/test_mmap.c | 10 ++-
>  .../selftests/bpf/progs/test_ringbuf.c        |  8 ++-
>  .../selftests/bpf/progs/test_ringbuf_multi.c  |  7 +-
>  7 files changed, 61 insertions(+), 61 deletions(-)
>
> --
> 2.29.2
>

Re: [PATCH v2 0/4] bpf/selftests: page size fixes

[Yauheni Kaliuta]

Hi, Andrii,

On Sun, Mar 28, 2021 at 8:05 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > A set of fixes for selftests to make them working on systems with PAGE_SIZE > 4K
> >
> > 2 questions left:
> >
> > - about `nit: if (!ASSERT_OK(err, "setsockopt_attach"))`. I left
> >   CHECK() for now since otherwise it has too many negations. But
> >   should I anyway use ASSERT?
>
> CHECK itself is a negation as much more confusing, IMO. if
> (!ASSERT_OK(err)) is pretty clear, as for me.
>
> >
> > - https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/prog_tests/mmap.c#L41
> >   and below -- it works now as is, but should be switched also to page_size?
>
> replied on another patch, it is possible to set all that at runtime
> with bpf_map__set_max_entries().

For both mmap and ringbuf or only for mmap?

But the question is about the mmap userspace part. In the test for
some reason both hardcoded 4096 and runtime page_size are used. I'm a
bit confused, should I replace that 4096 with page size.

>
>
> Overall, please specify the [PATCH bpf-next] prefix to denote that it
> targets bpf-next.

thanks for the review, I'll prepare v3 then.

>
>
> >
> > --
> > v1->v2:
> >
> > - add missed 'selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton'
> >
> > Yauheni Kaliuta (4):
> >
> >   selftests/bpf: test_progs/sockopt_sk: pass page size from userspace
> >   bpf: selftests: test_progs/sockopt_sk: remove version
> >   selftests/bpf: ringbuf, mmap: bump up page size to 64K
> >
> >  .../selftests/bpf/prog_tests/ringbuf.c        |  9 ++-
> >  .../selftests/bpf/prog_tests/sockopt_sk.c     | 68 ++++++-------------
> >  .../selftests/bpf/progs/map_ptr_kern.c        |  9 ++-
> >  .../testing/selftests/bpf/progs/sockopt_sk.c  | 11 ++-
> >  tools/testing/selftests/bpf/progs/test_mmap.c | 10 ++-
> >  .../selftests/bpf/progs/test_ringbuf.c        |  8 ++-
> >  .../selftests/bpf/progs/test_ringbuf_multi.c  |  7 +-
> >  7 files changed, 61 insertions(+), 61 deletions(-)
> >
> > --
> > 2.29.2
> >
>


-- 
WBR, Yauheni

Re: [PATCH v2 0/4] bpf/selftests: page size fixes

[Andrii Nakryiko]

On Sun, Mar 28, 2021 at 10:06 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Andrii,
>
> On Sun, Mar 28, 2021 at 8:05 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Fri, Mar 26, 2021 at 5:24 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> > >
> > > A set of fixes for selftests to make them working on systems with PAGE_SIZE > 4K
> > >
> > > 2 questions left:
> > >
> > > - about `nit: if (!ASSERT_OK(err, "setsockopt_attach"))`. I left
> > >   CHECK() for now since otherwise it has too many negations. But
> > >   should I anyway use ASSERT?
> >
> > CHECK itself is a negation as much more confusing, IMO. if
> > (!ASSERT_OK(err)) is pretty clear, as for me.
> >
> > >
> > > - https://github.com/torvalds/linux/blob/master/tools/testing/selftests/bpf/prog_tests/mmap.c#L41
> > >   and below -- it works now as is, but should be switched also to page_size?
> >
> > replied on another patch, it is possible to set all that at runtime
> > with bpf_map__set_max_entries().
>
> For both mmap and ringbuf or only for mmap?
>
> But the question is about the mmap userspace part. In the test for
> some reason both hardcoded 4096 and runtime page_size are used. I'm a
> bit confused, should I replace that 4096 with page size.

everywhere where 4096 is hard-coded, it was supposed to match page
size, so switching to page size would be best. for test_mmap, in
particular, I was trying to validate mmap refcounting, so each
separate 4096 bytes page was supposed to trigger as separate mmap
operation (with corresponding refcnt bump).


>
> >
> >
> > Overall, please specify the [PATCH bpf-next] prefix to denote that it
> > targets bpf-next.
>
> thanks for the review, I'll prepare v3 then.
>
> >
> >
> > >
> > > --
> > > v1->v2:
> > >
> > > - add missed 'selftests/bpf: test_progs/sockopt_sk: Convert to use BPF skeleton'
> > >
> > > Yauheni Kaliuta (4):
> > >
> > >   selftests/bpf: test_progs/sockopt_sk: pass page size from userspace
> > >   bpf: selftests: test_progs/sockopt_sk: remove version
> > >   selftests/bpf: ringbuf, mmap: bump up page size to 64K
> > >
> > >  .../selftests/bpf/prog_tests/ringbuf.c        |  9 ++-
> > >  .../selftests/bpf/prog_tests/sockopt_sk.c     | 68 ++++++-------------
> > >  .../selftests/bpf/progs/map_ptr_kern.c        |  9 ++-
> > >  .../testing/selftests/bpf/progs/sockopt_sk.c  | 11 ++-
> > >  tools/testing/selftests/bpf/progs/test_mmap.c | 10 ++-
> > >  .../selftests/bpf/progs/test_ringbuf.c        |  8 ++-
> > >  .../selftests/bpf/progs/test_ringbuf_multi.c  |  7 +-
> > >  7 files changed, 61 insertions(+), 61 deletions(-)
> > >
> > > --
> > > 2.29.2
> > >
> >
>
>
> --
> WBR, Yauheni
>

Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Yauheni Kaliuta]

On Sun, Mar 28, 2021 at 8:03 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:

[...]

> >
> >  struct {
> >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > -       __uint(max_entries, 4096);
> > +       __uint(max_entries, PAGE_SIZE);
>
>
> so you can set map size at runtime before bpf_object__load (or
> skeleton's load) with bpf_map__set_max_entries. That way you don't
> have to do any assumptions. Just omit max_entries in BPF source code,
> and always set it in userspace.

Will it work for ringbuf_multi? If I just set max_entries for ringbuf1
and ringbuf2 that way, it gives me

libbpf: map 'ringbuf_arr': failed to create inner map: -22
libbpf: map 'ringbuf_arr': failed to create: Invalid argument(-22)
libbpf: failed to load object 'test_ringbuf_multi'
libbpf: failed to load BPF skeleton 'test_ringbuf_multi': -22
test_ringbuf_multi:FAIL:skel_load skeleton load failed

Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Andrii Nakryiko]

On Mon, Mar 29, 2021 at 8:20 AM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> On Sun, Mar 28, 2021 at 8:03 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
>
> [...]
>
> > >
> > >  struct {
> > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > -       __uint(max_entries, 4096);
> > > +       __uint(max_entries, PAGE_SIZE);
> >
> >
> > so you can set map size at runtime before bpf_object__load (or
> > skeleton's load) with bpf_map__set_max_entries. That way you don't
> > have to do any assumptions. Just omit max_entries in BPF source code,
> > and always set it in userspace.
>
> Will it work for ringbuf_multi? If I just set max_entries for ringbuf1
> and ringbuf2 that way, it gives me
>
> libbpf: map 'ringbuf_arr': failed to create inner map: -22
> libbpf: map 'ringbuf_arr': failed to create: Invalid argument(-22)
> libbpf: failed to load object 'test_ringbuf_multi'
> libbpf: failed to load BPF skeleton 'test_ringbuf_multi': -22
> test_ringbuf_multi:FAIL:skel_load skeleton load failed
>

You are right, it won't work. We'd need to add something like
bpf_map__inner_map() accessor to allow to adjust the inner map
definition:

bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), page_size);

And some more fixes. Here's minimal diff that made it work, but
probably needs a bit more testing:

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7aad78dbb4b4..ed5586cce227 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2194,6 +2194,7 @@ static int parse_btf_map_def(struct bpf_object *obj,
             map->inner_map = calloc(1, sizeof(*map->inner_map));
             if (!map->inner_map)
                 return -ENOMEM;
+            map->inner_map->fd = -1;
             map->inner_map->sec_idx = obj->efile.btf_maps_shndx;
             map->inner_map->name = malloc(strlen(map->name) +
                               sizeof(".inner") + 1);
@@ -3845,6 +3846,14 @@ __u32 bpf_map__max_entries(const struct bpf_map *map)
     return map->def.max_entries;
 }

+struct bpf_map *bpf_map__inner_map(struct bpf_map *map)
+{
+    if (!bpf_map_type__is_map_in_map(map->def.type))
+        return NULL;
+
+    return map->inner_map;
+}
+
 int bpf_map__set_max_entries(struct bpf_map *map, __u32 max_entries)
 {
     if (map->fd >= 0)
@@ -9476,6 +9485,7 @@ int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
         pr_warn("error: inner_map_fd already specified\n");
         return -EINVAL;
     }
+    zfree(&map->inner_map);
     map->inner_map_fd = fd;
     return 0;
 }
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index f500621d28e5..bec4e6a6e31d 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -480,6 +480,7 @@ LIBBPF_API int bpf_map__pin(struct bpf_map *map,
const char *path);
 LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);

 LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd);
+LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map);

 LIBBPF_API long libbpf_get_error(const void *ptr);

diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
index f5990f7208ce..eeb6d5ebd1cc 100644
--- a/tools/lib/bpf/libbpf.map
+++ b/tools/lib/bpf/libbpf.map
@@ -360,4 +360,5 @@ LIBBPF_0.4.0 {
         bpf_linker__free;
         bpf_linker__new;
         bpf_object__set_kversion;
+        bpf_map__inner_map;
 } LIBBPF_0.3.0;
diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
index d37161e59bb2..cdc9c9b1d0e1 100644
--- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
+++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
@@ -41,13 +41,23 @@ static int process_sample(void *ctx, void *data, size_t len)
 void test_ringbuf_multi(void)
 {
     struct test_ringbuf_multi *skel;
-    struct ring_buffer *ringbuf;
+    struct ring_buffer *ringbuf = NULL;
     int err;

-    skel = test_ringbuf_multi__open_and_load();
+    skel = test_ringbuf_multi__open();
     if (CHECK(!skel, "skel_open_load", "skeleton open&load failed\n"))
         return;

+    bpf_map__set_max_entries(skel->maps.ringbuf1, 4096);
+    bpf_map__set_max_entries(skel->maps.ringbuf2, 4096);
+    bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), 4096);
+
+    err = test_ringbuf_multi__load(skel);
+    if (!ASSERT_OK(err, "skel_load"))
+        goto cleanup;
+
     /* only trigger BPF program for current process */
     skel->bss->pid = getpid();

diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
index edf3b6953533..055c10b2ff80 100644
--- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
+++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
@@ -15,7 +15,6 @@ struct sample {

 struct ringbuf_map {
     __uint(type, BPF_MAP_TYPE_RINGBUF);
-    __uint(max_entries, 1 << 12);
 } ringbuf1 SEC(".maps"),
   ringbuf2 SEC(".maps");

Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Yauheni Kaliuta]

Hi, Andrii,

On Wed, Mar 31, 2021 at 8:49 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Mar 29, 2021 at 8:20 AM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > On Sun, Mar 28, 2021 at 8:03 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> >
> > [...]
> >
> > > >
> > > >  struct {
> > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > -       __uint(max_entries, 4096);
> > > > +       __uint(max_entries, PAGE_SIZE);
> > >
> > >
> > > so you can set map size at runtime before bpf_object__load (or
> > > skeleton's load) with bpf_map__set_max_entries. That way you don't
> > > have to do any assumptions. Just omit max_entries in BPF source code,
> > > and always set it in userspace.
> >
> > Will it work for ringbuf_multi? If I just set max_entries for ringbuf1
> > and ringbuf2 that way, it gives me
> >
> > libbpf: map 'ringbuf_arr': failed to create inner map: -22
> > libbpf: map 'ringbuf_arr': failed to create: Invalid argument(-22)
> > libbpf: failed to load object 'test_ringbuf_multi'
> > libbpf: failed to load BPF skeleton 'test_ringbuf_multi': -22
> > test_ringbuf_multi:FAIL:skel_load skeleton load failed
> >
>
> You are right, it won't work. We'd need to add something like
> bpf_map__inner_map() accessor to allow to adjust the inner map
> definition:
>
> bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), page_size);

Thanks!

On top on that, for some reason simple ringbuf_multi (converted to use
dynamic size) does not work on my 64K page configuration too, haven't
investigated why. Works on x86 4K page.

>
> And some more fixes. Here's minimal diff that made it work, but
> probably needs a bit more testing:

Thanks again.
I could send the patchset with mmap only converted and just increase
ringbuf size since it's not selftests only change, but requires libbpf
improvements.

Or you would prefer to change them all together?



> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 7aad78dbb4b4..ed5586cce227 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2194,6 +2194,7 @@ static int parse_btf_map_def(struct bpf_object *obj,
>              map->inner_map = calloc(1, sizeof(*map->inner_map));
>              if (!map->inner_map)
>                  return -ENOMEM;
> +            map->inner_map->fd = -1;
>              map->inner_map->sec_idx = obj->efile.btf_maps_shndx;
>              map->inner_map->name = malloc(strlen(map->name) +
>                                sizeof(".inner") + 1);
> @@ -3845,6 +3846,14 @@ __u32 bpf_map__max_entries(const struct bpf_map *map)
>      return map->def.max_entries;
>  }
>
> +struct bpf_map *bpf_map__inner_map(struct bpf_map *map)
> +{
> +    if (!bpf_map_type__is_map_in_map(map->def.type))
> +        return NULL;
> +
> +    return map->inner_map;
> +}
> +
>  int bpf_map__set_max_entries(struct bpf_map *map, __u32 max_entries)
>  {
>      if (map->fd >= 0)
> @@ -9476,6 +9485,7 @@ int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
>          pr_warn("error: inner_map_fd already specified\n");
>          return -EINVAL;
>      }
> +    zfree(&map->inner_map);
>      map->inner_map_fd = fd;
>      return 0;
>  }
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index f500621d28e5..bec4e6a6e31d 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -480,6 +480,7 @@ LIBBPF_API int bpf_map__pin(struct bpf_map *map,
> const char *path);
>  LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
>
>  LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd);
> +LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map);
>
>  LIBBPF_API long libbpf_get_error(const void *ptr);
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index f5990f7208ce..eeb6d5ebd1cc 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -360,4 +360,5 @@ LIBBPF_0.4.0 {
>          bpf_linker__free;
>          bpf_linker__new;
>          bpf_object__set_kversion;
> +        bpf_map__inner_map;
>  } LIBBPF_0.3.0;
> diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> index d37161e59bb2..cdc9c9b1d0e1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> @@ -41,13 +41,23 @@ static int process_sample(void *ctx, void *data, size_t len)
>  void test_ringbuf_multi(void)
>  {
>      struct test_ringbuf_multi *skel;
> -    struct ring_buffer *ringbuf;
> +    struct ring_buffer *ringbuf = NULL;
>      int err;
>
> -    skel = test_ringbuf_multi__open_and_load();
> +    skel = test_ringbuf_multi__open();
>      if (CHECK(!skel, "skel_open_load", "skeleton open&load failed\n"))
>          return;
>
> +    bpf_map__set_max_entries(skel->maps.ringbuf1, 4096);
> +    bpf_map__set_max_entries(skel->maps.ringbuf2, 4096);
> +    bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), 4096);
> +
> +    err = test_ringbuf_multi__load(skel);
> +    if (!ASSERT_OK(err, "skel_load"))
> +        goto cleanup;
> +
>      /* only trigger BPF program for current process */
>      skel->bss->pid = getpid();
>
> diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> index edf3b6953533..055c10b2ff80 100644
> --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> @@ -15,7 +15,6 @@ struct sample {
>
>  struct ringbuf_map {
>      __uint(type, BPF_MAP_TYPE_RINGBUF);
> -    __uint(max_entries, 1 << 12);
>  } ringbuf1 SEC(".maps"),
>    ringbuf2 SEC(".maps");
>


-- 
WBR, Yauheni

Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Andrii Nakryiko]

On Tue, Mar 30, 2021 at 11:11 PM Yauheni Kaliuta
<yauheni.kaliuta@redhat.com> wrote:
>
> Hi, Andrii,
>
> On Wed, Mar 31, 2021 at 8:49 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Mar 29, 2021 at 8:20 AM Yauheni Kaliuta
> > <yauheni.kaliuta@redhat.com> wrote:
> > >
> > > On Sun, Mar 28, 2021 at 8:03 AM Andrii Nakryiko
> > > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > [...]
> > >
> > > > >
> > > > >  struct {
> > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > -       __uint(max_entries, 4096);
> > > > > +       __uint(max_entries, PAGE_SIZE);
> > > >
> > > >
> > > > so you can set map size at runtime before bpf_object__load (or
> > > > skeleton's load) with bpf_map__set_max_entries. That way you don't
> > > > have to do any assumptions. Just omit max_entries in BPF source code,
> > > > and always set it in userspace.
> > >
> > > Will it work for ringbuf_multi? If I just set max_entries for ringbuf1
> > > and ringbuf2 that way, it gives me
> > >
> > > libbpf: map 'ringbuf_arr': failed to create inner map: -22
> > > libbpf: map 'ringbuf_arr': failed to create: Invalid argument(-22)
> > > libbpf: failed to load object 'test_ringbuf_multi'
> > > libbpf: failed to load BPF skeleton 'test_ringbuf_multi': -22
> > > test_ringbuf_multi:FAIL:skel_load skeleton load failed
> > >
> >
> > You are right, it won't work. We'd need to add something like
> > bpf_map__inner_map() accessor to allow to adjust the inner map
> > definition:
> >
> > bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), page_size);
>
> Thanks!
>
> On top on that, for some reason simple ringbuf_multi (converted to use
> dynamic size) does not work on my 64K page configuration too, haven't
> investigated why. Works on x86 4K page.
>
> >
> > And some more fixes. Here's minimal diff that made it work, but
> > probably needs a bit more testing:
>
> Thanks again.
> I could send the patchset with mmap only converted and just increase
> ringbuf size since it's not selftests only change, but requires libbpf
> improvements.
>
> Or you would prefer to change them all together?

Try to figure out why the 64K case doesn't work. And then just post
all the patches together, it doesn't have to be strictly only
selftests. bpf_map__inner_map() and bpf_map__set_inner_map_fd()
resetting inner map are both useful for some cases that need more
manual control.

>
>
>
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 7aad78dbb4b4..ed5586cce227 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -2194,6 +2194,7 @@ static int parse_btf_map_def(struct bpf_object *obj,
> >              map->inner_map = calloc(1, sizeof(*map->inner_map));
> >              if (!map->inner_map)
> >                  return -ENOMEM;
> > +            map->inner_map->fd = -1;
> >              map->inner_map->sec_idx = obj->efile.btf_maps_shndx;
> >              map->inner_map->name = malloc(strlen(map->name) +
> >                                sizeof(".inner") + 1);
> > @@ -3845,6 +3846,14 @@ __u32 bpf_map__max_entries(const struct bpf_map *map)
> >      return map->def.max_entries;
> >  }
> >
> > +struct bpf_map *bpf_map__inner_map(struct bpf_map *map)
> > +{
> > +    if (!bpf_map_type__is_map_in_map(map->def.type))
> > +        return NULL;
> > +
> > +    return map->inner_map;
> > +}
> > +
> >  int bpf_map__set_max_entries(struct bpf_map *map, __u32 max_entries)
> >  {
> >      if (map->fd >= 0)
> > @@ -9476,6 +9485,7 @@ int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
> >          pr_warn("error: inner_map_fd already specified\n");
> >          return -EINVAL;
> >      }
> > +    zfree(&map->inner_map);
> >      map->inner_map_fd = fd;
> >      return 0;
> >  }
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index f500621d28e5..bec4e6a6e31d 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -480,6 +480,7 @@ LIBBPF_API int bpf_map__pin(struct bpf_map *map,
> > const char *path);
> >  LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
> >
> >  LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd);
> > +LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map);
> >
> >  LIBBPF_API long libbpf_get_error(const void *ptr);
> >
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index f5990f7208ce..eeb6d5ebd1cc 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -360,4 +360,5 @@ LIBBPF_0.4.0 {
> >          bpf_linker__free;
> >          bpf_linker__new;
> >          bpf_object__set_kversion;
> > +        bpf_map__inner_map;
> >  } LIBBPF_0.3.0;
> > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > index d37161e59bb2..cdc9c9b1d0e1 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > @@ -41,13 +41,23 @@ static int process_sample(void *ctx, void *data, size_t len)
> >  void test_ringbuf_multi(void)
> >  {
> >      struct test_ringbuf_multi *skel;
> > -    struct ring_buffer *ringbuf;
> > +    struct ring_buffer *ringbuf = NULL;
> >      int err;
> >
> > -    skel = test_ringbuf_multi__open_and_load();
> > +    skel = test_ringbuf_multi__open();
> >      if (CHECK(!skel, "skel_open_load", "skeleton open&load failed\n"))
> >          return;
> >
> > +    bpf_map__set_max_entries(skel->maps.ringbuf1, 4096);
> > +    bpf_map__set_max_entries(skel->maps.ringbuf2, 4096);
> > +    bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), 4096);
> > +
> > +    err = test_ringbuf_multi__load(skel);
> > +    if (!ASSERT_OK(err, "skel_load"))
> > +        goto cleanup;
> > +
> >      /* only trigger BPF program for current process */
> >      skel->bss->pid = getpid();
> >
> > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > index edf3b6953533..055c10b2ff80 100644
> > --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > @@ -15,7 +15,6 @@ struct sample {
> >
> >  struct ringbuf_map {
> >      __uint(type, BPF_MAP_TYPE_RINGBUF);
> > -    __uint(max_entries, 1 << 12);
> >  } ringbuf1 SEC(".maps"),
> >    ringbuf2 SEC(".maps");
> >
>
>
> --
> WBR, Yauheni
>

Re: [PATCH v2 4/4] selftests/bpf: ringbuf, mmap: bump up page size to 64K

[Yauheni Kaliuta]

On Wed, Mar 31, 2021 at 9:26 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Tue, Mar 30, 2021 at 11:11 PM Yauheni Kaliuta
> <yauheni.kaliuta@redhat.com> wrote:
> >
> > Hi, Andrii,
> >
> > On Wed, Mar 31, 2021 at 8:49 AM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Mar 29, 2021 at 8:20 AM Yauheni Kaliuta
> > > <yauheni.kaliuta@redhat.com> wrote:
> > > >
> > > > On Sun, Mar 28, 2021 at 8:03 AM Andrii Nakryiko
> > > > <andrii.nakryiko@gmail.com> wrote:
> > > >
> > > > [...]
> > > >
> > > > > >
> > > > > >  struct {
> > > > > >         __uint(type, BPF_MAP_TYPE_ARRAY);
> > > > > > -       __uint(max_entries, 4096);
> > > > > > +       __uint(max_entries, PAGE_SIZE);
> > > > >
> > > > >
> > > > > so you can set map size at runtime before bpf_object__load (or
> > > > > skeleton's load) with bpf_map__set_max_entries. That way you don't
> > > > > have to do any assumptions. Just omit max_entries in BPF source code,
> > > > > and always set it in userspace.
> > > >
> > > > Will it work for ringbuf_multi? If I just set max_entries for ringbuf1
> > > > and ringbuf2 that way, it gives me
> > > >
> > > > libbpf: map 'ringbuf_arr': failed to create inner map: -22
> > > > libbpf: map 'ringbuf_arr': failed to create: Invalid argument(-22)
> > > > libbpf: failed to load object 'test_ringbuf_multi'
> > > > libbpf: failed to load BPF skeleton 'test_ringbuf_multi': -22
> > > > test_ringbuf_multi:FAIL:skel_load skeleton load failed
> > > >
> > >
> > > You are right, it won't work. We'd need to add something like
> > > bpf_map__inner_map() accessor to allow to adjust the inner map
> > > definition:
> > >
> > > bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), page_size);
> >
> > Thanks!
> >
> > On top on that, for some reason simple ringbuf_multi (converted to use
> > dynamic size) does not work on my 64K page configuration too, haven't
> > investigated why. Works on x86 4K page.
> >
> > >
> > > And some more fixes. Here's minimal diff that made it work, but
> > > probably needs a bit more testing:
> >
> > Thanks again.
> > I could send the patchset with mmap only converted and just increase
> > ringbuf size since it's not selftests only change, but requires libbpf
> > improvements.
> >
> > Or you would prefer to change them all together?
>
> Try to figure out why the 64K case doesn't work.

It was not related to the tests actually.

> And then just post
> all the patches together, it doesn't have to be strictly only
> selftests. bpf_map__inner_map() and bpf_map__set_inner_map_fd()
> resetting inner map are both useful for some cases that need more
> manual control.

I took you libbpf patch as is, will post current version.

>
> >
> >
> >
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 7aad78dbb4b4..ed5586cce227 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -2194,6 +2194,7 @@ static int parse_btf_map_def(struct bpf_object *obj,
> > >              map->inner_map = calloc(1, sizeof(*map->inner_map));
> > >              if (!map->inner_map)
> > >                  return -ENOMEM;
> > > +            map->inner_map->fd = -1;
> > >              map->inner_map->sec_idx = obj->efile.btf_maps_shndx;
> > >              map->inner_map->name = malloc(strlen(map->name) +
> > >                                sizeof(".inner") + 1);
> > > @@ -3845,6 +3846,14 @@ __u32 bpf_map__max_entries(const struct bpf_map *map)
> > >      return map->def.max_entries;
> > >  }
> > >
> > > +struct bpf_map *bpf_map__inner_map(struct bpf_map *map)
> > > +{
> > > +    if (!bpf_map_type__is_map_in_map(map->def.type))
> > > +        return NULL;
> > > +
> > > +    return map->inner_map;
> > > +}
> > > +
> > >  int bpf_map__set_max_entries(struct bpf_map *map, __u32 max_entries)
> > >  {
> > >      if (map->fd >= 0)
> > > @@ -9476,6 +9485,7 @@ int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd)
> > >          pr_warn("error: inner_map_fd already specified\n");
> > >          return -EINVAL;
> > >      }
> > > +    zfree(&map->inner_map);
> > >      map->inner_map_fd = fd;
> > >      return 0;
> > >  }
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index f500621d28e5..bec4e6a6e31d 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -480,6 +480,7 @@ LIBBPF_API int bpf_map__pin(struct bpf_map *map,
> > > const char *path);
> > >  LIBBPF_API int bpf_map__unpin(struct bpf_map *map, const char *path);
> > >
> > >  LIBBPF_API int bpf_map__set_inner_map_fd(struct bpf_map *map, int fd);
> > > +LIBBPF_API struct bpf_map *bpf_map__inner_map(struct bpf_map *map);
> > >
> > >  LIBBPF_API long libbpf_get_error(const void *ptr);
> > >
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index f5990f7208ce..eeb6d5ebd1cc 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -360,4 +360,5 @@ LIBBPF_0.4.0 {
> > >          bpf_linker__free;
> > >          bpf_linker__new;
> > >          bpf_object__set_kversion;
> > > +        bpf_map__inner_map;
> > >  } LIBBPF_0.3.0;
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > index d37161e59bb2..cdc9c9b1d0e1 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/ringbuf_multi.c
> > > @@ -41,13 +41,23 @@ static int process_sample(void *ctx, void *data, size_t len)
> > >  void test_ringbuf_multi(void)
> > >  {
> > >      struct test_ringbuf_multi *skel;
> > > -    struct ring_buffer *ringbuf;
> > > +    struct ring_buffer *ringbuf = NULL;
> > >      int err;
> > >
> > > -    skel = test_ringbuf_multi__open_and_load();
> > > +    skel = test_ringbuf_multi__open();
> > >      if (CHECK(!skel, "skel_open_load", "skeleton open&load failed\n"))
> > >          return;
> > >
> > > +    bpf_map__set_max_entries(skel->maps.ringbuf1, 4096);
> > > +    bpf_map__set_max_entries(skel->maps.ringbuf2, 4096);
> > > +    bpf_map__set_max_entries(bpf_map__inner_map(skel->maps.ringbuf_arr), 4096);
> > > +
> > > +    err = test_ringbuf_multi__load(skel);
> > > +    if (!ASSERT_OK(err, "skel_load"))
> > > +        goto cleanup;
> > > +
> > >      /* only trigger BPF program for current process */
> > >      skel->bss->pid = getpid();
> > >
> > > diff --git a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > index edf3b6953533..055c10b2ff80 100644
> > > --- a/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > +++ b/tools/testing/selftests/bpf/progs/test_ringbuf_multi.c
> > > @@ -15,7 +15,6 @@ struct sample {
> > >
> > >  struct ringbuf_map {
> > >      __uint(type, BPF_MAP_TYPE_RINGBUF);
> > > -    __uint(max_entries, 1 << 12);
> > >  } ringbuf1 SEC(".maps"),
> > >    ringbuf2 SEC(".maps");
> > >
> >
> >
> > --
> > WBR, Yauheni
> >
>


-- 
WBR, Yauheni