| Message ID | 160384964803.698509.11020605670605638967.stgit@localhost.localdomain |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for bpf-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | success | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | fail | Errors and warnings before: 4 this patch: 4 |
| jkicinski/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | fail | Link |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Thu, Oct 29, 2020 at 12:57 AM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > 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> > .../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. > + 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 > + if (CHECK(IS_ERR(link), "attach_cgroup(estab)", "err: %ld\n", > + PTR_ERR(link))) there is a convenient ASSERT_OK_PTR() specifically for pointers like this (and NULL ones as well, of course); saves a lot of typing and encapsulates error extraction internally. > + goto cleanup_namespace; > > run_test(map_fd, sock_map_fd); > -err: > - bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS); > + > + bpf_link__destroy(link); > +cleanup_namespace: > close(cg_fd); > - cleanup_cgroup_environment(); > +cleanup_skel: > + test_tcpbpf_kern__destroy(skel); > } > >
On Thu, Oct 29, 2020 at 4:20 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Oct 29, 2020 at 12:57 AM Alexander Duyck > <alexander.duyck@gmail.com> wrote: > > > > 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> > > > .../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. In addition I was looking at simplifying the first patch which should just be the move with minimal changes to allow the functionality to build as a part of the test_progs framework. The end result should be the same it just helps to make the fact that the first patch should just be a move a little more clear. > > + 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. > > + if (CHECK(IS_ERR(link), "attach_cgroup(estab)", "err: %ld\n", > > + PTR_ERR(link))) > > there is a convenient ASSERT_OK_PTR() specifically for pointers like > this (and NULL ones as well, of course); saves a lot of typing and > encapsulates error extraction internally. I'll update the code to address that.
On Thu, Oct 29, 2020 at 5:42 PM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > On Thu, Oct 29, 2020 at 4:20 PM Andrii Nakryiko > <andrii.nakryiko@gmail.com> wrote: > > > > On Thu, Oct 29, 2020 at 12:57 AM Alexander Duyck > > <alexander.duyck@gmail.com> wrote: > > > > > > 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> > > > > > .../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. > > In addition I was looking at simplifying the first patch which should > just be the move with minimal changes to allow the functionality to > build as a part of the test_progs framework. The end result should be > the same it just helps to make the fact that the first patch should > just be a move a little more clear. sure > > > > + 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. > > > > + if (CHECK(IS_ERR(link), "attach_cgroup(estab)", "err: %ld\n", > > > + PTR_ERR(link))) > > > > there is a convenient ASSERT_OK_PTR() specifically for pointers like > > this (and NULL ones as well, of course); saves a lot of typing and > > encapsulates error extraction internally. > > I'll update the code to address that.
On Thu, Oct 29, 2020 at 7:30 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Thu, Oct 29, 2020 at 5:42 PM Alexander Duyck > <alexander.duyck@gmail.com> wrote: > > > > On Thu, Oct 29, 2020 at 4:20 PM Andrii Nakryiko > > <andrii.nakryiko@gmail.com> wrote: > > > > > > On Thu, Oct 29, 2020 at 12:57 AM Alexander Duyck > > > <alexander.duyck@gmail.com> wrote: > > > > > > > > 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> > > > > > > > .../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. > > > > > > + 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
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)) + 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); + if (CHECK(IS_ERR(link), "attach_cgroup(estab)", "err: %ld\n", + PTR_ERR(link))) + goto cleanup_namespace; run_test(map_fd, sock_map_fd); -err: - bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS); + + bpf_link__destroy(link); +cleanup_namespace: close(cg_fd); - cleanup_cgroup_environment(); +cleanup_skel: + test_tcpbpf_kern__destroy(skel); }