| Message ID | 160416890683.710453.7723265174628409401.stgit@localhost.localdomain |
|---|---|
| Headers | show |
| Series | selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework | expand |
On Sat, Oct 31, 2020 at 11:31 AM Alexander Duyck <alexander.duyck@gmail.com> wrote: > > Move the test functionality from test_tcpbpf_user into the test_progs > framework so that it will be run any time the test_progs framework is run. > This will help to prevent future test escapes as the individual tests, such > as test_tcpbpf_user, are less likely to be run by developers and CI > tests. > > As a part of moving it over the series goes through and updates the code to > make use of the existing APIs included in the test_progs framework. This is > meant to simplify and streamline the test code and avoid duplication of > effort. > > v2: Dropped test_tcpbpf_user from .gitignore > Replaced CHECK_FAIL calls with CHECK calls > Minimized changes in patch 1 when moving the file > Updated stg mail command line to display renames in submission > Added shutdown logic to end of run_test function to guarantee close > Added patch that replaces the two maps with use of global variables > > --- > > Alexander Duyck (5): > selftests/bpf: Move test_tcppbf_user into test_progs > selftests/bpf: Drop python client/server in favor of threads > selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results > selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton > selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern > > > .../selftests/bpf/prog_tests/tcpbpf_user.c | 239 +++++++++--------- > .../selftests/bpf/progs/test_tcpbpf_kern.c | 86 +------ > tools/testing/selftests/bpf/tcp_client.py | 50 ---- > tools/testing/selftests/bpf/tcp_server.py | 80 ------ > tools/testing/selftests/bpf/test_tcpbpf.h | 2 + > 5 files changed, 135 insertions(+), 322 deletions(-) > delete mode 100755 tools/testing/selftests/bpf/tcp_client.py > delete mode 100755 tools/testing/selftests/bpf/tcp_server.py > > -- It looks like the "--to" on the cover page was dropped and it wasn't delivered to the bpf mailing list. So I am just going to reply to the one that was delivered to netdev so that it is visible for the people on the bpf list. Thanks. - Alex
On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote: > From: Alexander Duyck <alexanderduyck@fb.com> > > Drop the tcp_client/server.py files in favor of using a client and server > thread within the test case. Specifically we spawn a new thread to play the The thread comment may be outdated in v2. > role of the server, and the main testing thread plays the role of client. > > Add logic to the end of the run_test function to guarantee that the sockets > are closed when we begin verifying results. > > Doing this we are able to reduce overhead since we don't have two python > workers possibly floating around. In addition we don't have to worry about > synchronization issues and as such the retry loop waiting for the threads > to close the sockets can be dropped as we will have already closed the > sockets in the local executable and synchronized the server thread. > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com> > --- > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 96 ++++++++++++++++---- > tools/testing/selftests/bpf/tcp_client.py | 50 ---------- > tools/testing/selftests/bpf/tcp_server.py | 80 ----------------- > 3 files changed, 78 insertions(+), 148 deletions(-) > delete mode 100755 tools/testing/selftests/bpf/tcp_client.py > delete mode 100755 tools/testing/selftests/bpf/tcp_server.py > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > index 54f1dce97729..17d4299435df 100644 > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > @@ -1,13 +1,14 @@ > // SPDX-License-Identifier: GPL-2.0 > #include <inttypes.h> > #include <test_progs.h> > +#include <network_helpers.h> > > #include "test_tcpbpf.h" > > +#define LO_ADDR6 "::1" > #define CG_NAME "/tcpbpf-user-test" > > -/* 3 comes from one listening socket + both ends of the connection */ > -#define EXPECTED_CLOSE_EVENTS 3 > +static __u32 duration; > > #define EXPECT_EQ(expected, actual, fmt) \ > do { \ > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result) > EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32); > EXPECT_EQ(0, result->good_cb_test_rv, PRIu32); > EXPECT_EQ(1, result->num_listen, PRIu32); > - EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32); > + > + /* 3 comes from one listening socket + both ends of the connection */ > + EXPECT_EQ(3, result->num_close_events, PRIu32); > > return ret; > } > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd) > return ret; > } > > +static int run_test(void) > +{ > + int listen_fd = -1, cli_fd = -1, accept_fd = -1; > + char buf[1000]; > + int err = -1; > + int i; > + > + listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0); > + if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n", > + listen_fd, errno)) > + goto done; > + > + cli_fd = connect_to_fd(listen_fd, 0); > + if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)", > + "cli_fd:%d errno:%d\n", cli_fd, errno)) > + goto done; > + > + accept_fd = accept(listen_fd, NULL, NULL); > + if (CHECK(accept_fd == -1, "accept(listen_fd)", > + "accept_fd:%d errno:%d\n", accept_fd, errno)) > + goto done; > + > + /* Send 1000B of '+'s from cli_fd -> accept_fd */ > + for (i = 0; i < 1000; i++) > + buf[i] = '+'; > + > + err = send(cli_fd, buf, 1000, 0); > + if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno)) > + goto done; > + > + err = recv(accept_fd, buf, 1000, 0); > + if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno)) > + goto done; > + > + /* Send 500B of '.'s from accept_fd ->cli_fd */ > + for (i = 0; i < 500; i++) > + buf[i] = '.'; > + > + err = send(accept_fd, buf, 500, 0); > + if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno)) > + goto done; > + > + err = recv(cli_fd, buf, 500, 0); Unlikely, but err from the above send()/recv() could be 0. > + if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno)) > + goto done; > + > + /* > + * shutdown accept first to guarantee correct ordering for > + * bytes_received and bytes_acked when we go to verify the results. > + */ > + shutdown(accept_fd, SHUT_WR); > + err = recv(cli_fd, buf, 1, 0); > + if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno)) > + goto done; > + > + shutdown(cli_fd, SHUT_WR); > + err = recv(accept_fd, buf, 1, 0); hmm... I was thinking cli_fd may still be in TCP_LAST_ACK but we can go with this version first and see if CI could really hit this case before resurrecting the idea on testing the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c. > + CHECK(err, "recv(accept_fd) for fin", "err:%d errno:%d\n", err, errno); > +done: > + if (accept_fd != -1) > + close(accept_fd); > + if (cli_fd != -1) > + close(cli_fd); > + if (listen_fd != -1) > + close(listen_fd); > + > + return err; > +} > + > void test_tcpbpf_user(void) > { > const char *file = "test_tcpbpf_kern.o"; > @@ -74,7 +146,6 @@ void test_tcpbpf_user(void) > int error = EXIT_FAILURE; > struct bpf_object *obj; > int cg_fd = -1; > - int retry = 10; > __u32 key = 0; > int rv; > > @@ -94,11 +165,6 @@ void test_tcpbpf_user(void) > goto err; > } > > - if (system("./tcp_server.py")) { > - fprintf(stderr, "FAILED: TCP server\n"); > - goto err; > - } > - > map_fd = bpf_find_map(__func__, obj, "global_map"); > if (map_fd < 0) > goto err; > @@ -107,21 +173,15 @@ void test_tcpbpf_user(void) > if (sock_map_fd < 0) > goto err; > > -retry_lookup: > + if (run_test()) > + goto err; > + > rv = bpf_map_lookup_elem(map_fd, &key, &g); > if (rv != 0) { > fprintf(stderr, "FAILED: bpf_map_lookup_elem returns %d\n", rv); > goto err; > } > > - if (g.num_close_events != EXPECTED_CLOSE_EVENTS && retry--) { > - fprintf(stderr, > - "Unexpected number of close events (%d), retrying!\n", > - g.num_close_events); > - usleep(100); > - goto retry_lookup; > - } > - > if (verify_result(&g)) { > fprintf(stderr, "FAILED: Wrong stats\n"); > goto err;
On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote: > > From: Alexander Duyck <alexanderduyck@fb.com> > > > > Drop the tcp_client/server.py files in favor of using a client and server > > thread within the test case. Specifically we spawn a new thread to play the > The thread comment may be outdated in v2. > > > role of the server, and the main testing thread plays the role of client. > > > > Add logic to the end of the run_test function to guarantee that the sockets > > are closed when we begin verifying results. > > > > Doing this we are able to reduce overhead since we don't have two python > > workers possibly floating around. In addition we don't have to worry about > > synchronization issues and as such the retry loop waiting for the threads > > to close the sockets can be dropped as we will have already closed the > > sockets in the local executable and synchronized the server thread. > > > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com> > > --- > > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 96 ++++++++++++++++---- > > tools/testing/selftests/bpf/tcp_client.py | 50 ---------- > > tools/testing/selftests/bpf/tcp_server.py | 80 ----------------- > > 3 files changed, 78 insertions(+), 148 deletions(-) > > delete mode 100755 tools/testing/selftests/bpf/tcp_client.py > > delete mode 100755 tools/testing/selftests/bpf/tcp_server.py > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > index 54f1dce97729..17d4299435df 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > @@ -1,13 +1,14 @@ > > // SPDX-License-Identifier: GPL-2.0 > > #include <inttypes.h> > > #include <test_progs.h> > > +#include <network_helpers.h> > > > > #include "test_tcpbpf.h" > > > > +#define LO_ADDR6 "::1" > > #define CG_NAME "/tcpbpf-user-test" > > > > -/* 3 comes from one listening socket + both ends of the connection */ > > -#define EXPECTED_CLOSE_EVENTS 3 > > +static __u32 duration; > > > > #define EXPECT_EQ(expected, actual, fmt) \ > > do { \ > > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result) > > EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32); > > EXPECT_EQ(0, result->good_cb_test_rv, PRIu32); > > EXPECT_EQ(1, result->num_listen, PRIu32); > > - EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32); > > + > > + /* 3 comes from one listening socket + both ends of the connection */ > > + EXPECT_EQ(3, result->num_close_events, PRIu32); > > > > return ret; > > } > > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd) > > return ret; > > } > > > > +static int run_test(void) > > +{ > > + int listen_fd = -1, cli_fd = -1, accept_fd = -1; > > + char buf[1000]; > > + int err = -1; > > + int i; > > + > > + listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0); > > + if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n", > > + listen_fd, errno)) > > + goto done; > > + > > + cli_fd = connect_to_fd(listen_fd, 0); > > + if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)", > > + "cli_fd:%d errno:%d\n", cli_fd, errno)) > > + goto done; > > + > > + accept_fd = accept(listen_fd, NULL, NULL); > > + if (CHECK(accept_fd == -1, "accept(listen_fd)", > > + "accept_fd:%d errno:%d\n", accept_fd, errno)) > > + goto done; > > + > > + /* Send 1000B of '+'s from cli_fd -> accept_fd */ > > + for (i = 0; i < 1000; i++) > > + buf[i] = '+'; > > + > > + err = send(cli_fd, buf, 1000, 0); > > + if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno)) > > + goto done; > > + > > + err = recv(accept_fd, buf, 1000, 0); > > + if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno)) > > + goto done; > > + > > + /* Send 500B of '.'s from accept_fd ->cli_fd */ > > + for (i = 0; i < 500; i++) > > + buf[i] = '.'; > > + > > + err = send(accept_fd, buf, 500, 0); > > + if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno)) > > + goto done; > > + > > + err = recv(cli_fd, buf, 500, 0); > Unlikely, but err from the above send()/recv() could be 0. Is that an issue? It would still trigger the check below as that is not 500. > > + if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno)) > > + goto done; > > + > > + /* > > + * shutdown accept first to guarantee correct ordering for > > + * bytes_received and bytes_acked when we go to verify the results. > > + */ > > + shutdown(accept_fd, SHUT_WR); > > + err = recv(cli_fd, buf, 1, 0); > > + if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno)) > > + goto done; > > + > > + shutdown(cli_fd, SHUT_WR); > > + err = recv(accept_fd, buf, 1, 0); > hmm... I was thinking cli_fd may still be in TCP_LAST_ACK > but we can go with this version first and see if CI could > really hit this case before resurrecting the idea on testing > the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c. I ran with this for several hours and saw no issues with over 100K iterations all of them passing. That is why I opted to just drop the TCP_LAST_ACK patch.
On Sat, Oct 31, 2020 at 11:52:31AM -0700, Alexander Duyck 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. > > In addition we can clean up the remaining bits such as the one remaining > CHECK_FAIL at the end of test_tcpbpf_user so that the function only makes > use of CHECK as needed. > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com> Acked-by: Martin KaFai Lau <kafai@fb.com> > --- > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 48 ++++++++------------ > 1 file changed, 18 insertions(+), 30 deletions(-) > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > index d96f4084d2f5..c7a61b0d616a 100644 > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > @@ -4,6 +4,7 @@ > #include <network_helpers.h> > > #include "test_tcpbpf.h" > +#include "test_tcpbpf_kern.skel.h" > > #define LO_ADDR6 "::1" > #define CG_NAME "/tcpbpf-user-test" > @@ -133,44 +134,31 @@ 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; > - int error = EXIT_FAILURE; > - struct bpf_object *obj; > + struct test_tcpbpf_kern *skel; > + int map_fd, sock_map_fd; > int cg_fd = -1; > - int rv; > - > - cg_fd = test__join_cgroup(CG_NAME); > - if (cg_fd < 0) > - goto err; > > - if (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 (rv) { > - fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n", > - errno, strerror(errno)); > - goto err; > - } > + cg_fd = test__join_cgroup(CG_NAME); > + if (CHECK(cg_fd < 0, "test__join_cgroup(" CG_NAME ")", > + "cg_fd:%d errno:%d", cg_fd, errno)) > + goto cleanup_skel; > > - map_fd = bpf_find_map(__func__, obj, "global_map"); > - if (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 (sock_map_fd < 0) > - goto err; > + skel->links.bpf_testcb = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd); > + if (ASSERT_OK_PTR(skel->links.bpf_testcb, "attach_cgroup(bpf_testcb)")) > + goto cleanup_namespace; > > run_test(map_fd, sock_map_fd); > > - error = 0; > -err: > - bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS); > +cleanup_namespace: nit. may be "cleanup_cgroup" instead? or only have one jump label to handle failure since "cg_fd != -1" has been tested already. > if (cg_fd != -1) > close(cg_fd); > - > - CHECK_FAIL(error); > +cleanup_skel: > + test_tcpbpf_kern__destroy(skel); > } > >
On Mon, Nov 02, 2020 at 04:49:42PM -0800, Alexander Duyck wrote: > On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote: > > > From: Alexander Duyck <alexanderduyck@fb.com> > > > > > > Drop the tcp_client/server.py files in favor of using a client and server > > > thread within the test case. Specifically we spawn a new thread to play the > > The thread comment may be outdated in v2. > > > > > role of the server, and the main testing thread plays the role of client. > > > > > > Add logic to the end of the run_test function to guarantee that the sockets > > > are closed when we begin verifying results. > > > > > > Doing this we are able to reduce overhead since we don't have two python > > > workers possibly floating around. In addition we don't have to worry about > > > synchronization issues and as such the retry loop waiting for the threads > > > to close the sockets can be dropped as we will have already closed the > > > sockets in the local executable and synchronized the server thread. > > > > > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com> > > > --- > > > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 96 ++++++++++++++++---- > > > tools/testing/selftests/bpf/tcp_client.py | 50 ---------- > > > tools/testing/selftests/bpf/tcp_server.py | 80 ----------------- > > > 3 files changed, 78 insertions(+), 148 deletions(-) > > > delete mode 100755 tools/testing/selftests/bpf/tcp_client.py > > > delete mode 100755 tools/testing/selftests/bpf/tcp_server.py > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > > index 54f1dce97729..17d4299435df 100644 > > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > > @@ -1,13 +1,14 @@ > > > // SPDX-License-Identifier: GPL-2.0 > > > #include <inttypes.h> > > > #include <test_progs.h> > > > +#include <network_helpers.h> > > > > > > #include "test_tcpbpf.h" > > > > > > +#define LO_ADDR6 "::1" > > > #define CG_NAME "/tcpbpf-user-test" > > > > > > -/* 3 comes from one listening socket + both ends of the connection */ > > > -#define EXPECTED_CLOSE_EVENTS 3 > > > +static __u32 duration; > > > > > > #define EXPECT_EQ(expected, actual, fmt) \ > > > do { \ > > > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result) > > > EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32); > > > EXPECT_EQ(0, result->good_cb_test_rv, PRIu32); > > > EXPECT_EQ(1, result->num_listen, PRIu32); > > > - EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32); > > > + > > > + /* 3 comes from one listening socket + both ends of the connection */ > > > + EXPECT_EQ(3, result->num_close_events, PRIu32); > > > > > > return ret; > > > } > > > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd) > > > return ret; > > > } > > > > > > +static int run_test(void) > > > +{ > > > + int listen_fd = -1, cli_fd = -1, accept_fd = -1; > > > + char buf[1000]; > > > + int err = -1; > > > + int i; > > > + > > > + listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0); > > > + if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n", > > > + listen_fd, errno)) > > > + goto done; > > > + > > > + cli_fd = connect_to_fd(listen_fd, 0); > > > + if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)", > > > + "cli_fd:%d errno:%d\n", cli_fd, errno)) > > > + goto done; > > > + > > > + accept_fd = accept(listen_fd, NULL, NULL); > > > + if (CHECK(accept_fd == -1, "accept(listen_fd)", > > > + "accept_fd:%d errno:%d\n", accept_fd, errno)) > > > + goto done; > > > + > > > + /* Send 1000B of '+'s from cli_fd -> accept_fd */ > > > + for (i = 0; i < 1000; i++) > > > + buf[i] = '+'; > > > + > > > + err = send(cli_fd, buf, 1000, 0); > > > + if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno)) > > > + goto done; > > > + > > > + err = recv(accept_fd, buf, 1000, 0); > > > + if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno)) > > > + goto done; > > > + > > > + /* Send 500B of '.'s from accept_fd ->cli_fd */ > > > + for (i = 0; i < 500; i++) > > > + buf[i] = '.'; > > > + > > > + err = send(accept_fd, buf, 500, 0); > > > + if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno)) > > > + goto done; > > > + > > > + err = recv(cli_fd, buf, 500, 0); > > Unlikely, but err from the above send()/recv() could be 0. > > Is that an issue? It would still trigger the check below as that is not 500. Mostly for consistency. "err" will be returned and tested for non-zero in test_tcpbpf_user(). > > > > + if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno)) > > > + goto done; > > > + > > > + /* > > > + * shutdown accept first to guarantee correct ordering for > > > + * bytes_received and bytes_acked when we go to verify the results. > > > + */ > > > + shutdown(accept_fd, SHUT_WR); > > > + err = recv(cli_fd, buf, 1, 0); > > > + if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno)) > > > + goto done; > > > + > > > + shutdown(cli_fd, SHUT_WR); > > > + err = recv(accept_fd, buf, 1, 0); > > hmm... I was thinking cli_fd may still be in TCP_LAST_ACK > > but we can go with this version first and see if CI could > > really hit this case before resurrecting the idea on testing > > the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c. > > I ran with this for several hours and saw no issues with over 100K > iterations all of them passing. That is why I opted to just drop the > TCP_LAST_ACK patch. Thanks for testing it hard. It is good enough for me.
On Mon, Nov 2, 2020 at 4:55 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Sat, Oct 31, 2020 at 11:52:31AM -0700, Alexander Duyck 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. > > > > In addition we can clean up the remaining bits such as the one remaining > > CHECK_FAIL at the end of test_tcpbpf_user so that the function only makes > > use of CHECK as needed. > > > > Acked-by: Andrii Nakryiko <andrii@kernel.org> > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com> > Acked-by: Martin KaFai Lau <kafai@fb.com> > > > --- > > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 48 ++++++++------------ > > 1 file changed, 18 insertions(+), 30 deletions(-) > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > index d96f4084d2f5..c7a61b0d616a 100644 > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > @@ -4,6 +4,7 @@ > > #include <network_helpers.h> > > > > #include "test_tcpbpf.h" > > +#include "test_tcpbpf_kern.skel.h" > > > > #define LO_ADDR6 "::1" > > #define CG_NAME "/tcpbpf-user-test" > > @@ -133,44 +134,31 @@ 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; > > - int error = EXIT_FAILURE; > > - struct bpf_object *obj; > > + struct test_tcpbpf_kern *skel; > > + int map_fd, sock_map_fd; > > int cg_fd = -1; > > - int rv; > > - > > - cg_fd = test__join_cgroup(CG_NAME); > > - if (cg_fd < 0) > > - goto err; > > > > - if (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 (rv) { > > - fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n", > > - errno, strerror(errno)); > > - goto err; > > - } > > + cg_fd = test__join_cgroup(CG_NAME); > > + if (CHECK(cg_fd < 0, "test__join_cgroup(" CG_NAME ")", > > + "cg_fd:%d errno:%d", cg_fd, errno)) > > + goto cleanup_skel; > > > > - map_fd = bpf_find_map(__func__, obj, "global_map"); > > - if (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 (sock_map_fd < 0) > > - goto err; > > + skel->links.bpf_testcb = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd); > > + if (ASSERT_OK_PTR(skel->links.bpf_testcb, "attach_cgroup(bpf_testcb)")) > > + goto cleanup_namespace; > > > > run_test(map_fd, sock_map_fd); > > > > - error = 0; > > -err: > > - bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS); > > +cleanup_namespace: > nit. > > may be "cleanup_cgroup" instead? > > or only have one jump label to handle failure since "cg_fd != -1" has been > tested already. Good point. I can go through and just drop the second label and simplify this. Will fix for v3.
On Mon, Nov 2, 2020 at 5:33 PM Martin KaFai Lau <kafai@fb.com> wrote: > > On Mon, Nov 02, 2020 at 04:49:42PM -0800, Alexander Duyck wrote: > > On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote: > > > > > > On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote: > > > > From: Alexander Duyck <alexanderduyck@fb.com> > > > > > > > > Drop the tcp_client/server.py files in favor of using a client and server > > > > thread within the test case. Specifically we spawn a new thread to play the > > > The thread comment may be outdated in v2. > > > > > > > role of the server, and the main testing thread plays the role of client. > > > > > > > > Add logic to the end of the run_test function to guarantee that the sockets > > > > are closed when we begin verifying results. > > > > > > > > Doing this we are able to reduce overhead since we don't have two python > > > > workers possibly floating around. In addition we don't have to worry about > > > > synchronization issues and as such the retry loop waiting for the threads > > > > to close the sockets can be dropped as we will have already closed the > > > > sockets in the local executable and synchronized the server thread. > > > > > > > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com> > > > > --- > > > > .../testing/selftests/bpf/prog_tests/tcpbpf_user.c | 96 ++++++++++++++++---- > > > > tools/testing/selftests/bpf/tcp_client.py | 50 ---------- > > > > tools/testing/selftests/bpf/tcp_server.py | 80 ----------------- > > > > 3 files changed, 78 insertions(+), 148 deletions(-) > > > > delete mode 100755 tools/testing/selftests/bpf/tcp_client.py > > > > delete mode 100755 tools/testing/selftests/bpf/tcp_server.py > > > > > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > > > index 54f1dce97729..17d4299435df 100644 > > > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c > > > > @@ -1,13 +1,14 @@ > > > > // SPDX-License-Identifier: GPL-2.0 > > > > #include <inttypes.h> > > > > #include <test_progs.h> > > > > +#include <network_helpers.h> > > > > > > > > #include "test_tcpbpf.h" > > > > > > > > +#define LO_ADDR6 "::1" > > > > #define CG_NAME "/tcpbpf-user-test" > > > > > > > > -/* 3 comes from one listening socket + both ends of the connection */ > > > > -#define EXPECTED_CLOSE_EVENTS 3 > > > > +static __u32 duration; > > > > > > > > #define EXPECT_EQ(expected, actual, fmt) \ > > > > do { \ > > > > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result) > > > > EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32); > > > > EXPECT_EQ(0, result->good_cb_test_rv, PRIu32); > > > > EXPECT_EQ(1, result->num_listen, PRIu32); > > > > - EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32); > > > > + > > > > + /* 3 comes from one listening socket + both ends of the connection */ > > > > + EXPECT_EQ(3, result->num_close_events, PRIu32); > > > > > > > > return ret; > > > > } > > > > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd) > > > > return ret; > > > > } > > > > > > > > +static int run_test(void) > > > > +{ > > > > + int listen_fd = -1, cli_fd = -1, accept_fd = -1; > > > > + char buf[1000]; > > > > + int err = -1; > > > > + int i; > > > > + > > > > + listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0); > > > > + if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n", > > > > + listen_fd, errno)) > > > > + goto done; > > > > + > > > > + cli_fd = connect_to_fd(listen_fd, 0); > > > > + if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)", > > > > + "cli_fd:%d errno:%d\n", cli_fd, errno)) > > > > + goto done; > > > > + > > > > + accept_fd = accept(listen_fd, NULL, NULL); > > > > + if (CHECK(accept_fd == -1, "accept(listen_fd)", > > > > + "accept_fd:%d errno:%d\n", accept_fd, errno)) > > > > + goto done; > > > > + > > > > + /* Send 1000B of '+'s from cli_fd -> accept_fd */ > > > > + for (i = 0; i < 1000; i++) > > > > + buf[i] = '+'; > > > > + > > > > + err = send(cli_fd, buf, 1000, 0); > > > > + if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno)) > > > > + goto done; > > > > + > > > > + err = recv(accept_fd, buf, 1000, 0); > > > > + if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno)) > > > > + goto done; > > > > + > > > > + /* Send 500B of '.'s from accept_fd ->cli_fd */ > > > > + for (i = 0; i < 500; i++) > > > > + buf[i] = '.'; > > > > + > > > > + err = send(accept_fd, buf, 500, 0); > > > > + if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno)) > > > > + goto done; > > > > + > > > > + err = recv(cli_fd, buf, 500, 0); > > > Unlikely, but err from the above send()/recv() could be 0. > > > > Is that an issue? It would still trigger the check below as that is not 500. > Mostly for consistency. "err" will be returned and tested for non-zero > in test_tcpbpf_user(). Okay that makes sense. Now that I have looked it over more it does lead to an error in the final product since it will attempt to verify data on a failed connection so I will probably just replaced err with a new variable such as rv for the send/recv part of the function so that err stays at -1 until we are closing the sockets.
Move the test functionality from test_tcpbpf_user into the test_progs framework so that it will be run any time the test_progs framework is run. This will help to prevent future test escapes as the individual tests, such as test_tcpbpf_user, are less likely to be run by developers and CI tests. As a part of moving it over the series goes through and updates the code to make use of the existing APIs included in the test_progs framework. This is meant to simplify and streamline the test code and avoid duplication of effort. v2: Dropped test_tcpbpf_user from .gitignore Replaced CHECK_FAIL calls with CHECK calls Minimized changes in patch 1 when moving the file Updated stg mail command line to display renames in submission Added shutdown logic to end of run_test function to guarantee close Added patch that replaces the two maps with use of global variables --- Alexander Duyck (5): selftests/bpf: Move test_tcppbf_user into test_progs selftests/bpf: Drop python client/server in favor of threads selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern .../selftests/bpf/prog_tests/tcpbpf_user.c | 239 +++++++++--------- .../selftests/bpf/progs/test_tcpbpf_kern.c | 86 +------ tools/testing/selftests/bpf/tcp_client.py | 50 ---- tools/testing/selftests/bpf/tcp_server.py | 80 ------ tools/testing/selftests/bpf/test_tcpbpf.h | 2 + 5 files changed, 135 insertions(+), 322 deletions(-) delete mode 100755 tools/testing/selftests/bpf/tcp_client.py delete mode 100755 tools/testing/selftests/bpf/tcp_server.py --