From: Geliang Tang tanggeliang@kylinos.cn
For moving dctcp test dedicated code out of do_test() into test_dctcp(). This patchset adds a new helper start_test() in bpf_tcp_ca.c to refactor do_test().
Address Martin's comments for the previous series.
Geliang Tang (5): selftests/bpf: Use connect_to_fd_opts in do_test in bpf_tcp_ca selftests/bpf: Add start_test helper in bpf_tcp_ca selftests/bpf: Use start_test in test_dctcp_fallback in bpf_tcp_ca selftests/bpf: Use start_test in test_dctcp in bpf_tcp_ca selftests/bpf: Drop useless arguments of do_test in bpf_tcp_ca
.../selftests/bpf/prog_tests/bpf_tcp_ca.c | 140 +++++++++++------- 1 file changed, 85 insertions(+), 55 deletions(-)
From: Geliang Tang tanggeliang@kylinos.cn
This patch uses connect_to_fd_opts() instead of using connect_fd_to_fd() and settcpca() in do_test() in prog_tests/bpf_tcp_ca.c to accept a struct network_helper_opts argument.
Then define a dctcp dedicated post_socket_cb callback stg_post_socket_cb(), invoking both settcpca() and bpf_map_update_elem() in it, and set it in test_dctcp(). For passing map_fd into stg_post_socket_cb() callback, a new member map_fd is added in struct cb_opts.
Add another "const struct network_helper_opts *cli_opts" to do_test() to separate it from the server "opts".
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 61 +++++++++++-------- 1 file changed, 34 insertions(+), 27 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index ebc7d4616880..2f9d373feb0a 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -25,6 +25,7 @@ static int expected_stg = 0xeB9F;
struct cb_opts { const char *cc; + int map_fd; };
static int settcpca(int fd, const char *tcp_ca) @@ -39,9 +40,9 @@ static int settcpca(int fd, const char *tcp_ca) }
static void do_test(const struct network_helper_opts *opts, + const struct network_helper_opts *cli_opts, const struct bpf_map *sk_stg_map) { - struct cb_opts *cb_opts = (struct cb_opts *)opts->cb_opts; int lfd = -1, fd = -1; int err;
@@ -49,25 +50,9 @@ static void do_test(const struct network_helper_opts *opts, if (!ASSERT_NEQ(lfd, -1, "socket")) return;
- fd = socket(AF_INET6, SOCK_STREAM, 0); - if (!ASSERT_NEQ(fd, -1, "socket")) { - close(lfd); - return; - } - - if (settcpca(fd, cb_opts->cc)) - goto done; - - if (sk_stg_map) { - err = bpf_map_update_elem(bpf_map__fd(sk_stg_map), &fd, - &expected_stg, BPF_NOEXIST); - if (!ASSERT_OK(err, "bpf_map_update_elem(sk_stg_map)")) - goto done; - } - /* connect to server */ - err = connect_fd_to_fd(fd, lfd, 0); - if (!ASSERT_NEQ(err, -1, "connect")) + fd = connect_to_fd_opts(lfd, cli_opts); + if (!ASSERT_NEQ(fd, -1, "connect_to_fd_opts")) goto done;
if (sk_stg_map) { @@ -116,7 +101,7 @@ static void test_cubic(void) return; }
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL);
ASSERT_EQ(cubic_skel->bss->bpf_cubic_acked_called, 1, "pkts_acked called");
@@ -124,6 +109,23 @@ static void test_cubic(void) bpf_cubic__destroy(cubic_skel); }
+static int stg_post_socket_cb(int fd, void *opts) +{ + struct cb_opts *cb_opts = (struct cb_opts *)opts; + int err; + + err = settcpca(fd, cb_opts->cc); + if (err) + return err; + + err = bpf_map_update_elem(cb_opts->map_fd, &fd, + &expected_stg, BPF_NOEXIST); + if (!ASSERT_OK(err, "bpf_map_update_elem(sk_stg_map)")) + return err; + + return 0; +} + static void test_dctcp(void) { struct cb_opts cb_opts = { @@ -133,6 +135,10 @@ static void test_dctcp(void) .post_socket_cb = cc_cb, .cb_opts = &cb_opts, }; + struct network_helper_opts cli_opts = { + .post_socket_cb = stg_post_socket_cb, + .cb_opts = &cb_opts, + }; struct bpf_dctcp *dctcp_skel; struct bpf_link *link;
@@ -146,7 +152,8 @@ static void test_dctcp(void) return; }
- do_test(&opts, dctcp_skel->maps.sk_stg_map); + cb_opts.map_fd = bpf_map__fd(dctcp_skel->maps.sk_stg_map); + do_test(&opts, &cli_opts, dctcp_skel->maps.sk_stg_map); ASSERT_EQ(dctcp_skel->bss->stg_result, expected_stg, "stg_result");
bpf_link__destroy(link); @@ -350,14 +357,14 @@ static void test_update_ca(void) link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); ASSERT_OK_PTR(link, "attach_struct_ops");
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL); saved_ca1_cnt = skel->bss->ca1_cnt; ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
err = bpf_link__update_map(link, skel->maps.ca_update_2); ASSERT_OK(err, "update_map");
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL); ASSERT_EQ(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt"); ASSERT_GT(skel->bss->ca2_cnt, 0, "ca2_ca2_cnt");
@@ -386,14 +393,14 @@ static void test_update_wrong(void) link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); ASSERT_OK_PTR(link, "attach_struct_ops");
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL); saved_ca1_cnt = skel->bss->ca1_cnt; ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
err = bpf_link__update_map(link, skel->maps.ca_wrong); ASSERT_ERR(err, "update_map");
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL); ASSERT_GT(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt");
bpf_link__destroy(link); @@ -423,7 +430,7 @@ static void test_mixed_links(void) link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); ASSERT_OK_PTR(link, "attach_struct_ops");
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL); ASSERT_GT(skel->bss->ca1_cnt, 0, "ca1_ca1_cnt");
err = bpf_link__update_map(link, skel->maps.ca_no_link); @@ -530,7 +537,7 @@ static void test_cc_cubic(void) return; }
- do_test(&opts, NULL); + do_test(&opts, &opts, NULL);
bpf_link__destroy(link); bpf_cc_cubic__destroy(cc_cubic_skel);
From: Geliang Tang tanggeliang@kylinos.cn
For moving the "if (sk_stg_map)" block out of do_test(), extract the code before this block as a new function start_test(). It creates server-side and client-side sockets and returns them to the caller.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 42 +++++++++++++++---- 1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 2f9d373feb0a..794651ce0629 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -39,6 +39,34 @@ static int settcpca(int fd, const char *tcp_ca) return 0; }
+static bool start_test(char *addr_str, + const struct network_helper_opts *srv_opts, + const struct network_helper_opts *cli_opts, + int *srv_fd, int *cli_fd) +{ + *srv_fd = start_server_str(AF_INET6, SOCK_STREAM, addr_str, 0, srv_opts); + if (!ASSERT_NEQ(*srv_fd, -1, "start_server_str")) + goto err; + + /* connect to server */ + *cli_fd = connect_to_fd_opts(*srv_fd, cli_opts); + if (!ASSERT_NEQ(*cli_fd, -1, "connect_to_fd_opts")) + goto err; + + return true; + +err: + if (*srv_fd != -1) { + close(*srv_fd); + *srv_fd = -1; + } + if (*cli_fd != -1) { + close(*cli_fd); + *cli_fd = -1; + } + return false; +} + static void do_test(const struct network_helper_opts *opts, const struct network_helper_opts *cli_opts, const struct bpf_map *sk_stg_map) @@ -46,13 +74,7 @@ static void do_test(const struct network_helper_opts *opts, int lfd = -1, fd = -1; int err;
- lfd = start_server_str(AF_INET6, SOCK_STREAM, NULL, 0, opts); - if (!ASSERT_NEQ(lfd, -1, "socket")) - return; - - /* connect to server */ - fd = connect_to_fd_opts(lfd, cli_opts); - if (!ASSERT_NEQ(fd, -1, "connect_to_fd_opts")) + if (!start_test(NULL, opts, cli_opts, &lfd, &fd)) goto done;
if (sk_stg_map) { @@ -68,8 +90,10 @@ static void do_test(const struct network_helper_opts *opts, ASSERT_OK(send_recv_data(lfd, fd, total_bytes), "send_recv_data");
done: - close(lfd); - close(fd); + if (lfd != -1) + close(lfd); + if (fd != -1) + close(fd); }
static int cc_cb(int fd, void *opts)
From: Geliang Tang tanggeliang@kylinos.cn
The newly added helper start_test() can be used in test_dctcp_fallback() too, to replace start_server_str() and connect_to_fd_opts(). In that way, two network_helper_opts srv_opts and cli_opts are used instead of the previously shared opts.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 794651ce0629..d10217169ff8 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -229,17 +229,22 @@ static void test_invalid_license(void) static void test_dctcp_fallback(void) { int err, lfd = -1, cli_fd = -1, srv_fd = -1; - struct network_helper_opts opts = { - .post_socket_cb = cc_cb, - }; struct bpf_dctcp *dctcp_skel; struct bpf_link *link = NULL; struct cb_opts dctcp = { .cc = "bpf_dctcp", }; + struct network_helper_opts srv_opts = { + .post_socket_cb = cc_cb, + .cb_opts = &dctcp, + }; struct cb_opts cubic = { .cc = "cubic", }; + struct network_helper_opts cli_opts = { + .post_socket_cb = cc_cb, + .cb_opts = &cubic, + }; char srv_cc[16]; socklen_t cc_len = sizeof(srv_cc);
@@ -254,14 +259,7 @@ static void test_dctcp_fallback(void) if (!ASSERT_OK_PTR(link, "dctcp link")) goto done;
- opts.cb_opts = &dctcp; - lfd = start_server_str(AF_INET6, SOCK_STREAM, "::1", 0, &opts); - if (!ASSERT_GE(lfd, 0, "lfd")) - goto done; - - opts.cb_opts = &cubic; - cli_fd = connect_to_fd_opts(lfd, &opts); - if (!ASSERT_GE(cli_fd, 0, "cli_fd")) + if (!start_test("::1", &srv_opts, &cli_opts, &lfd, &cli_fd)) goto done;
srv_fd = accept(lfd, NULL, 0);
From: Geliang Tang tanggeliang@kylinos.cn
The "if (sk_stg_map)" block in do_test() is only used by test_dctcp(), it makes sense to move it from do_test() into test_dctcp(). Then do_test() can be used by other tests except test_dctcp().
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 27 ++++++++++--------- 1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index d10217169ff8..1b27d0232cbd 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -72,21 +72,10 @@ static void do_test(const struct network_helper_opts *opts, const struct bpf_map *sk_stg_map) { int lfd = -1, fd = -1; - int err;
if (!start_test(NULL, opts, cli_opts, &lfd, &fd)) goto done;
- if (sk_stg_map) { - int tmp_stg; - - err = bpf_map_lookup_elem(bpf_map__fd(sk_stg_map), &fd, - &tmp_stg); - if (!ASSERT_ERR(err, "bpf_map_lookup_elem(sk_stg_map)") || - !ASSERT_EQ(errno, ENOENT, "bpf_map_lookup_elem(sk_stg_map)")) - goto done; - } - ASSERT_OK(send_recv_data(lfd, fd, total_bytes), "send_recv_data");
done: @@ -163,6 +152,7 @@ static void test_dctcp(void) .post_socket_cb = stg_post_socket_cb, .cb_opts = &cb_opts, }; + int lfd = -1, fd = -1, tmp_stg, err; struct bpf_dctcp *dctcp_skel; struct bpf_link *link;
@@ -177,11 +167,24 @@ static void test_dctcp(void) }
cb_opts.map_fd = bpf_map__fd(dctcp_skel->maps.sk_stg_map); - do_test(&opts, &cli_opts, dctcp_skel->maps.sk_stg_map); + if (!start_test(NULL, &opts, &cli_opts, &lfd, &fd)) + goto done; + + err = bpf_map_lookup_elem(cb_opts.map_fd, &fd, &tmp_stg); + if (!ASSERT_ERR(err, "bpf_map_lookup_elem(sk_stg_map)") || + !ASSERT_EQ(errno, ENOENT, "bpf_map_lookup_elem(sk_stg_map)")) + goto done; + + ASSERT_OK(send_recv_data(lfd, fd, total_bytes), "send_recv_data"); ASSERT_EQ(dctcp_skel->bss->stg_result, expected_stg, "stg_result");
+done: bpf_link__destroy(link); bpf_dctcp__destroy(dctcp_skel); + if (lfd != -1) + close(lfd); + if (fd != -1) + close(fd); }
static char *err_str;
From: Geliang Tang tanggeliang@kylinos.cn
bpf_map_lookup_elem() has been removed from do_test(), it makes the sk_stg_map argument of do_test() useless. In addition, two exactly the same opts are passed in all the places where do_test() is invoked, so cli_opts argument can be dropped too.
This patch drops these two useless arguments of do_test() in bpf_tcp_ca.c.
Signed-off-by: Geliang Tang tanggeliang@kylinos.cn --- .../selftests/bpf/prog_tests/bpf_tcp_ca.c | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c index 1b27d0232cbd..67358adf5db3 100644 --- a/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c +++ b/tools/testing/selftests/bpf/prog_tests/bpf_tcp_ca.c @@ -67,13 +67,11 @@ static bool start_test(char *addr_str, return false; }
-static void do_test(const struct network_helper_opts *opts, - const struct network_helper_opts *cli_opts, - const struct bpf_map *sk_stg_map) +static void do_test(const struct network_helper_opts *opts) { int lfd = -1, fd = -1;
- if (!start_test(NULL, opts, cli_opts, &lfd, &fd)) + if (!start_test(NULL, opts, opts, &lfd, &fd)) goto done;
ASSERT_OK(send_recv_data(lfd, fd, total_bytes), "send_recv_data"); @@ -114,7 +112,7 @@ static void test_cubic(void) return; }
- do_test(&opts, &opts, NULL); + do_test(&opts);
ASSERT_EQ(cubic_skel->bss->bpf_cubic_acked_called, 1, "pkts_acked called");
@@ -382,14 +380,14 @@ static void test_update_ca(void) link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); ASSERT_OK_PTR(link, "attach_struct_ops");
- do_test(&opts, &opts, NULL); + do_test(&opts); saved_ca1_cnt = skel->bss->ca1_cnt; ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
err = bpf_link__update_map(link, skel->maps.ca_update_2); ASSERT_OK(err, "update_map");
- do_test(&opts, &opts, NULL); + do_test(&opts); ASSERT_EQ(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt"); ASSERT_GT(skel->bss->ca2_cnt, 0, "ca2_ca2_cnt");
@@ -418,14 +416,14 @@ static void test_update_wrong(void) link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); ASSERT_OK_PTR(link, "attach_struct_ops");
- do_test(&opts, &opts, NULL); + do_test(&opts); saved_ca1_cnt = skel->bss->ca1_cnt; ASSERT_GT(saved_ca1_cnt, 0, "ca1_ca1_cnt");
err = bpf_link__update_map(link, skel->maps.ca_wrong); ASSERT_ERR(err, "update_map");
- do_test(&opts, &opts, NULL); + do_test(&opts); ASSERT_GT(skel->bss->ca1_cnt, saved_ca1_cnt, "ca2_ca1_cnt");
bpf_link__destroy(link); @@ -455,7 +453,7 @@ static void test_mixed_links(void) link = bpf_map__attach_struct_ops(skel->maps.ca_update_1); ASSERT_OK_PTR(link, "attach_struct_ops");
- do_test(&opts, &opts, NULL); + do_test(&opts); ASSERT_GT(skel->bss->ca1_cnt, 0, "ca1_ca1_cnt");
err = bpf_link__update_map(link, skel->maps.ca_no_link); @@ -562,7 +560,7 @@ static void test_cc_cubic(void) return; }
- do_test(&opts, &opts, NULL); + do_test(&opts);
bpf_link__destroy(link); bpf_cc_cubic__destroy(cc_cubic_skel);
Hello:
This series was applied to bpf/bpf-next.git (master) by Daniel Borkmann daniel@iogearbox.net:
On Thu, 30 May 2024 15:41:07 +0800 you wrote:
From: Geliang Tang tanggeliang@kylinos.cn
For moving dctcp test dedicated code out of do_test() into test_dctcp(). This patchset adds a new helper start_test() in bpf_tcp_ca.c to refactor do_test().
Address Martin's comments for the previous series.
[...]
Here is the summary with links: - [bpf-next,1/5] selftests/bpf: Use connect_to_fd_opts in do_test in bpf_tcp_ca https://git.kernel.org/bpf/bpf-next/c/9abdfd8a2123 - [bpf-next,2/5] selftests/bpf: Add start_test helper in bpf_tcp_ca https://git.kernel.org/bpf/bpf-next/c/fee97d0c9a14 - [bpf-next,3/5] selftests/bpf: Use start_test in test_dctcp_fallback in bpf_tcp_ca https://git.kernel.org/bpf/bpf-next/c/224eeb5598c3 - [bpf-next,4/5] selftests/bpf: Use start_test in test_dctcp in bpf_tcp_ca https://git.kernel.org/bpf/bpf-next/c/cd984b2ed624 - [bpf-next,5/5] selftests/bpf: Drop useless arguments of do_test in bpf_tcp_ca https://git.kernel.org/bpf/bpf-next/c/f85af9d955ac
You are awesome, thank you!
linux-kselftest-mirror@lists.linaro.org
-
Geliang Tang -
patchwork-bot+netdevbpf＠kernel.org