[PATCH bpf-next 0/5] use network helpers, part 6

[PATCH bpf-next 0/5] use network helpers, part 6

[Geliang Tang]

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(-)

-- 
2.43.0

[PATCH bpf-next 1/5] selftests/bpf: Use connect_to_fd_opts in do_test in bpf_tcp_ca

[Geliang Tang]

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);
-- 
2.43.0

[PATCH bpf-next 2/5] selftests/bpf: Add start_test helper in bpf_tcp_ca

[Geliang Tang]

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)
-- 
2.43.0

[PATCH bpf-next 3/5] selftests/bpf: Use start_test in test_dctcp_fallback in bpf_tcp_ca

[Geliang Tang]

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);
-- 
2.43.0

[PATCH bpf-next 4/5] selftests/bpf: Use start_test in test_dctcp in bpf_tcp_ca

[Geliang Tang]

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;
-- 
2.43.0

[PATCH bpf-next 5/5] selftests/bpf: Drop useless arguments of do_test in bpf_tcp_ca

[Geliang Tang]

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);
-- 
2.43.0

Re: [PATCH bpf-next 0/5] use network helpers, part 6

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html