Re: [bpf-next PATCH 4/4] selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton

On Thu, Oct 29, 2020 at 7:30 PM Andrii Nakryiko
[off-list ref] wrote:

On Thu, Oct 29, 2020 at 5:42 PM Alexander Duyck
[off-list ref] wrote:

quoted

On Thu, Oct 29, 2020 at 4:20 PM Andrii Nakryiko
[off-list ref] wrote:

quoted

On Thu, Oct 29, 2020 at 12:57 AM Alexander Duyck
[off-list ref] wrote:

quoted

From: Alexander Duyck <alexanderduyck@fb.com>

Update tcpbpf_user.c to make use of the BPF skeleton. Doing this we can
simplify test_tcpbpf_user and reduce the overhead involved in setting up
the test.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---

Few suggestions below, but overall looks good:

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

quoted

 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   48 +++++++++-----------
 1 file changed, 21 insertions(+), 27 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index 4e1190894e1e..7e92c37976ac 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c

@@ -5,6 +5,7 @@

 #include "test_tcpbpf.h"
 #include "cgroup_helpers.h"
+#include "test_tcpbpf_kern.skel.h"

 #define LO_ADDR6 "::1"
 #define CG_NAME "/tcpbpf-user-test"

@@ -162,39 +163,32 @@ static void run_test(int map_fd, int sock_map_fd)

 void test_tcpbpf_user(void)
 {
-       const char *file = "test_tcpbpf_kern.o";
-       int prog_fd, map_fd, sock_map_fd;
-       struct bpf_object *obj;
+       struct test_tcpbpf_kern *skel;
+       int map_fd, sock_map_fd;
+       struct bpf_link *link;
        int cg_fd = -1;
-       int rv;
-
-       cg_fd = cgroup_setup_and_join(CG_NAME);
-       if (CHECK_FAIL(cg_fd < 0))
-               goto err;

-       if (CHECK_FAIL(bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd))) {
-               fprintf(stderr, "FAILED: load_bpf_file failed for: %s\n", file);
-               goto err;
-       }
+       skel = test_tcpbpf_kern__open_and_load();
+       if (CHECK(!skel, "open and load skel", "failed"))
+               return;

-       rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
-       if (CHECK_FAIL(rv)) {
-               fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n",
-                      errno, strerror(errno));
-               goto err;
-       }
+       cg_fd = test__join_cgroup(CG_NAME);
+       if (CHECK_FAIL(cg_fd < 0))

please use either CHECK() or one of the newer ASSERT_xxx() macro (also
defined in test_progs.h), CHECK_FAIL should be avoided in general.

So the plan I had was to actually move over to the following:
        cg_fd = test__join_cgroup(CG_NAME);
        if (CHECK_FAIL(cg_fd < 0))
                goto cleanup_skel;

It still makes use of CHECK_FAIL but it looks like test__join_cgroup
already takes care of error messaging so using CHECK_FAIL in this case
makes more sense.

CHECK (and ASSERT) leave a paper-trail in verbose test log, so it
makes it easier to debug tests, if something fails. CHECK_FAIL is
invisible, unless if fails. So CHECK_FAIL should be used only for
things that are happening on the order of hundreds of instances per
test, or more.

Okay, well in that case I will go through and replace the CHECK_FAIL
calls with a CHECK.

quoted

quoted

quoted

+               goto cleanup_skel;

-       map_fd = bpf_find_map(__func__, obj, "global_map");
-       if (CHECK_FAIL(map_fd < 0))
-               goto err;
+       map_fd = bpf_map__fd(skel->maps.global_map);
+       sock_map_fd = bpf_map__fd(skel->maps.sockopt_results);

-       sock_map_fd = bpf_find_map(__func__, obj, "sockopt_results");
-       if (CHECK_FAIL(sock_map_fd < 0))
-               goto err;
+       link = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd);

you can do skel->links.bpf_testcb = bpf_program__attach_cgroup() and
skeleton's __destroy() call will take care of destroying the link

Okay, I can look into using that. Actually this has me wondering if
there wouldn't be some way to make use of test_tcpbpf_kern__attach to
achieve this in some standard way. I'll get that sorted before I
submit v2.

cgroup BPF programs can't be auto-attached, because they expect cgroup
FD, which you can't provide at compilation time (declaratively) in BPF
code. So it has to be manually attached. skeleton's attach() will just
ignore such programs.

Yeah, I figured that out after reviewing the code further.

Thanks.

- Alex