Message ID | 20200911091411.37645-1-lmb@cloudflare.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf-next] bpf: selftests: remove shared header from sockmap iter test | expand |
On 9/11/20 2:14 AM, Lorenz Bauer wrote: > The shared header to define SOCKMAP_MAX_ENTRIES is a bit overkill. > Dynamically allocate the sock_fd array based on bpf_map__max_entries > instead. > > Suggested-by: Yonghong Song <yhs@fb.com> > Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> Ack with some nits below. Acked-by: Yonghong Song <yhs@fb.com> > --- > .../selftests/bpf/prog_tests/sockmap_basic.c | 39 ++++++++++--------- > .../selftests/bpf/progs/bpf_iter_sockmap.c | 5 +-- > .../selftests/bpf/progs/bpf_iter_sockmap.h | 3 -- > 3 files changed, 23 insertions(+), 24 deletions(-) > delete mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h > > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > index 4b7a527e7e82..2672a91cd78f 100644 > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c > @@ -8,8 +8,6 @@ > #include "test_sockmap_invalid_update.skel.h" > #include "bpf_iter_sockmap.skel.h" > > -#include "progs/bpf_iter_sockmap.h" > - > #define TCP_REPAIR 19 /* TCP sock is under repair right now */ > > #define TCP_REPAIR_ON 1 > @@ -179,9 +177,9 @@ static void test_sockmap_iter(enum bpf_map_type map_type) > DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts); > int err, len, src_fd, iter_fd, duration = 0; > union bpf_iter_link_info linfo = {0}; > - __s64 sock_fd[SOCKMAP_MAX_ENTRIES]; > - __u32 i, num_sockets, max_elems; > + __u32 i, num_sockets, num_elems; > struct bpf_iter_sockmap *skel; > + __s64 *sock_fd = NULL; > struct bpf_link *link; > struct bpf_map *src; > char buf[64]; > @@ -190,22 +188,26 @@ static void test_sockmap_iter(enum bpf_map_type map_type) > if (CHECK(!skel, "bpf_iter_sockmap__open_and_load", "skeleton open_and_load failed\n")) > return; > > - for (i = 0; i < ARRAY_SIZE(sock_fd); i++) > - sock_fd[i] = -1; > - > - /* Make sure we have at least one "empty" entry to test iteration of > - * an empty slot. > - */ > - num_sockets = ARRAY_SIZE(sock_fd) - 1; > - > if (map_type == BPF_MAP_TYPE_SOCKMAP) { > src = skel->maps.sockmap; > - max_elems = bpf_map__max_entries(src); > + num_elems = bpf_map__max_entries(src); > } else { > src = skel->maps.sockhash; > - max_elems = num_sockets; > + num_elems = bpf_map__max_entries(src) - 1; > } > > + /* Make sure we have at least one "empty" entry to test iteration of > + * an empty slot. > + */ > + num_sockets = bpf_map__max_entries(src) - 1; I found the logic above a little bit confusing in the sense you want num_elems and num_sockets the same for sockhash but from code they are assigned both from bpf_map__max_entries(src) - 1. Maybe the following more clear? /* Make sure we have at least one "empty" entry to test * iteration of an empty slot. */ if (map_type == BPF_MAP_TYPE_SOCKMAP) { src = skel->maps.sockmap; num_elems = bpf_map__max_entries(src); num_sockets = num_elems - 1; } else { src = skel->maps.sockhash; num_sockets = bpf_map__max_entries(src) - 1; num_elems = num_sockets; } > + > + sock_fd = calloc(num_sockets, sizeof(*sock_fd)); > + if (CHECK(!sock_fd, "calloc(sock_fd)", "failed to allocate\n")) > + goto out; calloc will initialize the value to 0, but it is overwritten below with -1. malloc is sufficient here. > + > + for (i = 0; i < num_sockets; i++) > + sock_fd[i] = -1; > + > src_fd = bpf_map__fd(src); > > for (i = 0; i < num_sockets; i++) { [...]
diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c index 4b7a527e7e82..2672a91cd78f 100644 --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c @@ -8,8 +8,6 @@ #include "test_sockmap_invalid_update.skel.h" #include "bpf_iter_sockmap.skel.h" -#include "progs/bpf_iter_sockmap.h" - #define TCP_REPAIR 19 /* TCP sock is under repair right now */ #define TCP_REPAIR_ON 1 @@ -179,9 +177,9 @@ static void test_sockmap_iter(enum bpf_map_type map_type) DECLARE_LIBBPF_OPTS(bpf_iter_attach_opts, opts); int err, len, src_fd, iter_fd, duration = 0; union bpf_iter_link_info linfo = {0}; - __s64 sock_fd[SOCKMAP_MAX_ENTRIES]; - __u32 i, num_sockets, max_elems; + __u32 i, num_sockets, num_elems; struct bpf_iter_sockmap *skel; + __s64 *sock_fd = NULL; struct bpf_link *link; struct bpf_map *src; char buf[64]; @@ -190,22 +188,26 @@ static void test_sockmap_iter(enum bpf_map_type map_type) if (CHECK(!skel, "bpf_iter_sockmap__open_and_load", "skeleton open_and_load failed\n")) return; - for (i = 0; i < ARRAY_SIZE(sock_fd); i++) - sock_fd[i] = -1; - - /* Make sure we have at least one "empty" entry to test iteration of - * an empty slot. - */ - num_sockets = ARRAY_SIZE(sock_fd) - 1; - if (map_type == BPF_MAP_TYPE_SOCKMAP) { src = skel->maps.sockmap; - max_elems = bpf_map__max_entries(src); + num_elems = bpf_map__max_entries(src); } else { src = skel->maps.sockhash; - max_elems = num_sockets; + num_elems = bpf_map__max_entries(src) - 1; } + /* Make sure we have at least one "empty" entry to test iteration of + * an empty slot. + */ + num_sockets = bpf_map__max_entries(src) - 1; + + sock_fd = calloc(num_sockets, sizeof(*sock_fd)); + if (CHECK(!sock_fd, "calloc(sock_fd)", "failed to allocate\n")) + goto out; + + for (i = 0; i < num_sockets; i++) + sock_fd[i] = -1; + src_fd = bpf_map__fd(src); for (i = 0; i < num_sockets; i++) { @@ -236,8 +238,8 @@ static void test_sockmap_iter(enum bpf_map_type map_type) goto close_iter; /* test results */ - if (CHECK(skel->bss->elems != max_elems, "elems", "got %u expected %u\n", - skel->bss->elems, max_elems)) + if (CHECK(skel->bss->elems != num_elems, "elems", "got %u expected %u\n", + skel->bss->elems, num_elems)) goto close_iter; if (CHECK(skel->bss->socks != num_sockets, "socks", "got %u expected %u\n", @@ -249,10 +251,11 @@ static void test_sockmap_iter(enum bpf_map_type map_type) free_link: bpf_link__destroy(link); out: - for (i = 0; i < num_sockets; i++) { + for (i = 0; sock_fd && i < num_sockets; i++) if (sock_fd[i] >= 0) close(sock_fd[i]); - } + if (sock_fd) + free(sock_fd); bpf_iter_sockmap__destroy(skel); } diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c index 0e27f73dd803..1af7555f6057 100644 --- a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c +++ b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.c @@ -2,7 +2,6 @@ /* Copyright (c) 2020 Cloudflare */ #include "bpf_iter.h" #include "bpf_tracing_net.h" -#include "bpf_iter_sockmap.h" #include <bpf/bpf_helpers.h> #include <bpf/bpf_tracing.h> #include <errno.h> @@ -11,14 +10,14 @@ char _license[] SEC("license") = "GPL"; struct { __uint(type, BPF_MAP_TYPE_SOCKMAP); - __uint(max_entries, SOCKMAP_MAX_ENTRIES); + __uint(max_entries, 64); __type(key, __u32); __type(value, __u64); } sockmap SEC(".maps"); struct { __uint(type, BPF_MAP_TYPE_SOCKHASH); - __uint(max_entries, SOCKMAP_MAX_ENTRIES); + __uint(max_entries, 64); __type(key, __u32); __type(value, __u64); } sockhash SEC(".maps"); diff --git a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h b/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h deleted file mode 100644 index 35a675d13c0f..000000000000 --- a/tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h +++ /dev/null @@ -1,3 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ - -#define SOCKMAP_MAX_ENTRIES (64)
The shared header to define SOCKMAP_MAX_ENTRIES is a bit overkill. Dynamically allocate the sock_fd array based on bpf_map__max_entries instead. Suggested-by: Yonghong Song <yhs@fb.com> Signed-off-by: Lorenz Bauer <lmb@cloudflare.com> --- .../selftests/bpf/prog_tests/sockmap_basic.c | 39 ++++++++++--------- .../selftests/bpf/progs/bpf_iter_sockmap.c | 5 +-- .../selftests/bpf/progs/bpf_iter_sockmap.h | 3 -- 3 files changed, 23 insertions(+), 24 deletions(-) delete mode 100644 tools/testing/selftests/bpf/progs/bpf_iter_sockmap.h