diff mbox series

[mptcp-next,v11,6/9] selftests/bpf: Add mptcp_subflow bpf_iter subtest

Message ID 67cbbd7ff515dcc421605b24c14c734fd3263e42.1729242644.git.tanggeliang@kylinos.cn (mailing list archive)
State Superseded, archived
Headers show
Series add mptcp_subflow bpf_iter | expand

Checks

Context Check Description
matttbe/KVM_Validation__normal success Success! ✅
matttbe/KVM_Validation__debug success Success! ✅
matttbe/KVM_Validation__btf-normal__only_bpftest_all_ success Success! ✅
matttbe/KVM_Validation__btf-debug__only_bpftest_all_ success Success! ✅
matttbe/checkpatch success total: 0 errors, 0 warnings, 0 checks, 87 lines checked
matttbe/shellcheck success MPTCP selftests files have not been modified
matttbe/build success Build and static analysis OK

Commit Message

Geliang Tang Oct. 18, 2024, 9:18 a.m. UTC 
From: Geliang Tang <tanggeliang@kylinos.cn>

This patch adds a subtest named test_iters_subflow to load and verify the
newly added mptcp_subflow type bpf_iter example in test_mptcp. Use the
helper endpoint_init() to add 3 new subflow endpoints. Send a byte of
message to start the mptcp connection, and wait for new subflows to be
added. getsockopt() is invoked to trigger the "cgroup/getsockopt" test
program "iters_subflow". Check if skel->bss->ids equals 10 to verify
whether this mptcp_subflow bpf_iter loops correctly as expected.

Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)

Comments

Matthieu Baerts Oct. 18, 2024, 11:06 a.m. UTC | #1 
Hi Geliang,

Thank you for the new version.

On 18/10/2024 11:18, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> This patch adds a subtest named test_iters_subflow to load and verify the
> newly added mptcp_subflow type bpf_iter example in test_mptcp. Use the
> helper endpoint_init() to add 3 new subflow endpoints. Send a byte of
> message to start the mptcp connection, and wait for new subflows to be
> added. getsockopt() is invoked to trigger the "cgroup/getsockopt" test
> program "iters_subflow". Check if skel->bss->ids equals 10 to verify
> whether this mptcp_subflow bpf_iter loops correctly as expected.

Can we squash this commit into "selftests/bpf: Add mptcp_subflow
bpf_iter test prog"?

These two commits don't make sense alone, and it might be easier for the
reviewers to see in the same patch what is being done in kernelspace via
BPF, and in userspace, instead of going from one to another. No?

> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 02fdff87df2d..0f00eef7aef5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c

(...)

> @@ -509,6 +510,72 @@ static void test_subflow(void)
>  	close(cgroup_fd);
>  }
>  
> +static void run_iters_subflow(void)
> +{
> +	int server_fd, client_fd;
> +	char cc[TCP_CA_NAME_MAX];
> +	socklen_t len;
> +	int err;
> +
> +	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
> +	if (!ASSERT_OK_FD(server_fd, "start_mptcp_server"))
> +		return;
> +
> +	client_fd = connect_to_fd(server_fd, 0);
> +	if (!ASSERT_OK_FD(client_fd, "connect_to_fd"))
> +		goto close_server;
> +
> +	send_byte(client_fd);
> +	wait_for_new_subflows(client_fd);
> +
> +	len = sizeof(cc);
> +	err = getsockopt(client_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
> +	if (ASSERT_OK(err, "getsockopt(client_fd, TCP_CONGESTION)"))
> +		ASSERT_STREQ(cc, "cubic", "cc");

You cannot assume "cubic" is the expected CC: it can be changed via
kernel config we don't fully control here.

Also, it might be confusing to get the CC, people might ask why while I
guess you are fine with any socket option, all you want is a trigger for
the BPF program I suppose. In this case, why not using TCP_IS_MPTCP (or
MPTCP_INFO)?

  /* mainly to trigger the BPF program */
  err = getsockopt(client_fd, SOL_TCP, TCP_IS_MPTCP, &mptcp, &len);

> +
> +	close(client_fd);
> +close_server:
> +	close(server_fd);
> +}
> +
> +static void test_iters_subflow(void)
> +{
> +	struct mptcp_bpf_iters *skel;
> +	struct nstoken *nstoken;
> +	int cgroup_fd;
> +
> +	cgroup_fd = test__join_cgroup("/iters_subflow");
> +	if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup: iters_subflow"))
> +		return;
> +
> +	skel = mptcp_bpf_iters__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "skel_open_load: iters_subflow"))
> +		goto close_cgroup;
> +
> +	skel->links.iters_subflow = bpf_program__attach_cgroup(skel->progs.iters_subflow,
> +							       cgroup_fd);
> +	if (!ASSERT_OK_PTR(skel->links.iters_subflow, "attach getsockopt"))
> +		goto skel_destroy;
> +
> +	nstoken = create_netns();
> +	if (!ASSERT_OK_PTR(nstoken, "create_netns: iters_subflow"))
> +		goto skel_destroy;
> +
> +	if (endpoint_init("subflow", 4) < 0)
> +		goto close_netns;
> +
> +	run_iters_subflow();
> +
> +	ASSERT_EQ(skel->bss->ids, 10, "subflow ids");

Can you add a comment just above to explain why 10? Simply:

  /* 1 + 2 + 3 + 4 = 10 */

That will help reviewers and devs later.

Cheers,
Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 02fdff87df2d..0f00eef7aef5 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -11,6 +11,7 @@ 
 #include "mptcp_sock.skel.h"
 #include "mptcpify.skel.h"
 #include "mptcp_subflow.skel.h"
+#include "mptcp_bpf_iters.skel.h"
 #include "mptcp_bpf_first.skel.h"
 #include "mptcp_bpf_bkup.skel.h"
 #include "mptcp_bpf_rr.skel.h"
@@ -509,6 +510,72 @@  static void test_subflow(void)
 	close(cgroup_fd);
 }
 
+static void run_iters_subflow(void)
+{
+	int server_fd, client_fd;
+	char cc[TCP_CA_NAME_MAX];
+	socklen_t len;
+	int err;
+
+	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
+	if (!ASSERT_OK_FD(server_fd, "start_mptcp_server"))
+		return;
+
+	client_fd = connect_to_fd(server_fd, 0);
+	if (!ASSERT_OK_FD(client_fd, "connect_to_fd"))
+		goto close_server;
+
+	send_byte(client_fd);
+	wait_for_new_subflows(client_fd);
+
+	len = sizeof(cc);
+	err = getsockopt(client_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
+	if (ASSERT_OK(err, "getsockopt(client_fd, TCP_CONGESTION)"))
+		ASSERT_STREQ(cc, "cubic", "cc");
+
+	close(client_fd);
+close_server:
+	close(server_fd);
+}
+
+static void test_iters_subflow(void)
+{
+	struct mptcp_bpf_iters *skel;
+	struct nstoken *nstoken;
+	int cgroup_fd;
+
+	cgroup_fd = test__join_cgroup("/iters_subflow");
+	if (!ASSERT_OK_FD(cgroup_fd, "join_cgroup: iters_subflow"))
+		return;
+
+	skel = mptcp_bpf_iters__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "skel_open_load: iters_subflow"))
+		goto close_cgroup;
+
+	skel->links.iters_subflow = bpf_program__attach_cgroup(skel->progs.iters_subflow,
+							       cgroup_fd);
+	if (!ASSERT_OK_PTR(skel->links.iters_subflow, "attach getsockopt"))
+		goto skel_destroy;
+
+	nstoken = create_netns();
+	if (!ASSERT_OK_PTR(nstoken, "create_netns: iters_subflow"))
+		goto skel_destroy;
+
+	if (endpoint_init("subflow", 4) < 0)
+		goto close_netns;
+
+	run_iters_subflow();
+
+	ASSERT_EQ(skel->bss->ids, 10, "subflow ids");
+
+close_netns:
+	cleanup_netns(nstoken);
+skel_destroy:
+	mptcp_bpf_iters__destroy(skel);
+close_cgroup:
+	close(cgroup_fd);
+}
+
 static struct nstoken *sched_init(char *flags, char *sched)
 {
 	struct nstoken *nstoken;
@@ -690,6 +757,8 @@  void test_mptcp(void)
 		test_mptcpify();
 	if (test__start_subtest("subflow"))
 		test_subflow();
+	if (test__start_subtest("iters_subflow"))
+		test_iters_subflow();
 	if (test__start_subtest("default"))
 		test_default();
 	if (test__start_subtest("first"))