[v5] fixes for "new MPTCP subflow subtest v4"

[PATCH mptcp-next v5 2/3] Squash to "selftests/bpf: Add mptcp subflow subtest"

Posted by Geliang Tang 1 year, 5 months ago

From: Geliang Tang <tanggeliang@kylinos.cn>

Drop ss_search() from run_subflow().

Use the "cgroup/getsockopt" way to inspect the subflows.

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

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 73adc58cd776..85883515c67b 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -368,22 +368,13 @@ static int endpoint_init(char *flags)
 	return -1;
 }
 
-static int _ss_search(char *src, char *dst, char *port, char *keyword)
-{
-	return SYS_NOFAIL("ip netns exec %s ss -enita src %s dst %s %s %d | grep -q '%s'",
-			  NS_TEST, src, dst, port, PORT_1, keyword);
-}
-
-static int ss_search(char *src, char *keyword)
-{
-	return _ss_search(src, ADDR_1, "dport", keyword);
-}
-
 static void run_subflow(char *new)
 {
-	int server_fd, client_fd, err;
+	int server_fd, client_fd, err, i;
 	char cc[TCP_CA_NAME_MAX];
+	unsigned int mark;
 	socklen_t len;
+	u8 subflows;
 
 	server_fd = start_mptcp_server(AF_INET, ADDR_1, PORT_1, 0);
 	if (!ASSERT_GE(server_fd, 0, "start_mptcp_server"))
@@ -393,17 +384,29 @@ static void run_subflow(char *new)
 	if (!ASSERT_GE(client_fd, 0, "connect to fd"))
 		goto close_server;
 
-	len = sizeof(cc);
-	err = getsockopt(server_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
-	if (!ASSERT_OK(err, "getsockopt(server_fd, TCP_CONGESTION)"))
-		goto close_client;
-
 	send_byte(client_fd);
 
-	ASSERT_OK(ss_search(ADDR_1, "fwmark:0x1"), "ss_search fwmark:0x1");
-	ASSERT_OK(ss_search(ADDR_2, "fwmark:0x2"), "ss_search fwmark:0x2");
-	ASSERT_OK(ss_search(ADDR_1, new), "ss_search new cc");
-	ASSERT_OK(ss_search(ADDR_2, cc), "ss_search default cc");
+	len = sizeof(subflows);
+	/* Wait max 1 sec for new subflows to be created */
+	for (i = 0; i < 10; i++) {
+		err = getsockopt(client_fd, SOL_MPTCP, MPTCP_INFO, &subflows, &len);
+		if (!err && subflows > 0)
+			break;
+
+		sleep(0.1);
+	}
+
+	len = sizeof(mark);
+	err = getsockopt(client_fd, SOL_SOCKET, SO_MARK, &mark, &len);
+	if (!ASSERT_OK(err, "getsockopt(client_fd, SO_MARK)"))
+		goto close_client;
+	ASSERT_EQ(mark, 0, "mark");
+
+	len = sizeof(cc);
+	err = getsockopt(client_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
+	if (!ASSERT_OK(err, "getsockopt(client_fd, TCP_CONGESTION)"))
+		goto close_client;
+	ASSERT_STREQ(cc, new, "cc");
 
 close_client:
 	close(client_fd);
@@ -416,6 +419,7 @@ static void test_subflow(void)
 	int cgroup_fd, prog_fd, err;
 	struct mptcp_subflow *skel;
 	struct nstoken *nstoken;
+	struct bpf_link *link;
 
 	cgroup_fd = test__join_cgroup("/mptcp_subflow");
 	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup: mptcp_subflow"))
@@ -441,6 +445,11 @@ static void test_subflow(void)
 	if (endpoint_init("subflow") < 0)
 		goto close_netns;
 
+	link = bpf_program__attach_cgroup(skel->progs._getsockopt_subflow,
+					  cgroup_fd);
+	if (!ASSERT_OK_PTR(link, "getsockopt prog"))
+		goto close_netns;
+
 	run_subflow(skel->data->cc);
 
 close_netns:
@@ -448,6 +457,7 @@ static void test_subflow(void)
 skel_destroy:
 	mptcp_subflow__destroy(skel);
 close_cgroup:
+	bpf_link__destroy(link);
 	close(cgroup_fd);
 }
 
-- 
2.43.0

Re: [PATCH mptcp-next v5 2/3] Squash to "selftests/bpf: Add mptcp subflow subtest"

Posted by Matthieu Baerts 1 year, 5 months ago

Hi Geliang,

Thank you for the update.

On 29/08/2024 04:53, Geliang Tang wrote:
> From: Geliang Tang <tanggeliang@kylinos.cn>
> 
> Drop ss_search() from run_subflow().
> 
> Use the "cgroup/getsockopt" way to inspect the subflows.
> 
> Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>
> ---
>  .../testing/selftests/bpf/prog_tests/mptcp.c  | 52 +++++++++++--------
>  1 file changed, 31 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index 73adc58cd776..85883515c67b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c

(...)

> @@ -393,17 +384,29 @@ static void run_subflow(char *new)
>  	if (!ASSERT_GE(client_fd, 0, "connect to fd"))
>  		goto close_server;
>  
> -	len = sizeof(cc);
> -	err = getsockopt(server_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
> -	if (!ASSERT_OK(err, "getsockopt(server_fd, TCP_CONGESTION)"))
> -		goto close_client;
> -
>  	send_byte(client_fd);
>  
> -	ASSERT_OK(ss_search(ADDR_1, "fwmark:0x1"), "ss_search fwmark:0x1");
> -	ASSERT_OK(ss_search(ADDR_2, "fwmark:0x2"), "ss_search fwmark:0x2");
> -	ASSERT_OK(ss_search(ADDR_1, new), "ss_search new cc");
> -	ASSERT_OK(ss_search(ADDR_2, cc), "ss_search default cc");
> +	len = sizeof(subflows);
> +	/* Wait max 1 sec for new subflows to be created */
> +	for (i = 0; i < 10; i++) {
> +		err = getsockopt(client_fd, SOL_MPTCP, MPTCP_INFO, &subflows, &len);
> +		if (!err && subflows > 0)
> +			break;
> +
> +		sleep(0.1);
> +	}

(detail: it might be better to have that in a new helper, but up to you,
maybe easier to keep it here for the moment).

> +
> +	len = sizeof(mark);
> +	err = getsockopt(client_fd, SOL_SOCKET, SO_MARK, &mark, &len);
> +	if (!ASSERT_OK(err, "getsockopt(client_fd, SO_MARK)"))
> +		goto close_client;

You still don't need this 'goto close_client': it is better to do as
many checks as possible. If needed, you can do something like:

  if (ASSERT_OK(err, "getsockopt(client_fd, SO_MARK)"))
          ASSERT_EQ(mark, 0, "mark");

But it is better to also check the TCP_CC, to better understand what
went wrong.

> +	ASSERT_EQ(mark, 0, "mark");
> +
> +	len = sizeof(cc);
> +	err = getsockopt(client_fd, SOL_TCP, TCP_CONGESTION, cc, &len);
> +	if (!ASSERT_OK(err, "getsockopt(client_fd, TCP_CONGESTION)"))
> +		goto close_client;

Same here.

> +	ASSERT_STREQ(cc, new, "cc");

Did you drop the first patch of the v4 to ease the check here, and not
to have to look at the default value? I suppose that's OK.

>  close_client:
>  	close(client_fd);
> @@ -416,6 +419,7 @@ static void test_subflow(void)
>  	int cgroup_fd, prog_fd, err;
>  	struct mptcp_subflow *skel;
>  	struct nstoken *nstoken;
> +	struct bpf_link *link;
>  
>  	cgroup_fd = test__join_cgroup("/mptcp_subflow");
>  	if (!ASSERT_GE(cgroup_fd, 0, "join_cgroup: mptcp_subflow"))
> @@ -441,6 +445,11 @@ static void test_subflow(void)
>  	if (endpoint_init("subflow") < 0)
>  		goto close_netns;
>  
> +	link = bpf_program__attach_cgroup(skel->progs._getsockopt_subflow,
> +					  cgroup_fd);
> +	if (!ASSERT_OK_PTR(link, "getsockopt prog"))
> +		goto close_netns;
> +
>  	run_subflow(skel->data->cc);
>  
>  close_netns:
> @@ -448,6 +457,7 @@ static void test_subflow(void)
>  skel_destroy:
>  	mptcp_subflow__destroy(skel);
>  close_cgroup:
> +	bpf_link__destroy(link);

Mmh, I see you moved it, but I don't think it is OK to have here, right?

I should be placed just before the 'close_netns' label, not to call it
with an undefined 'link' variable, no?

>  	close(cgroup_fd);
>  }
>  

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next v5 1/3] selftests/bpf: Add getsockopt to inspect mptcp subflow
[PATCH mptcp-next v5 2/3] Squash to "selftests/bpf: Add mptcp subflow subtest"
[PATCH mptcp-next v5 3/3] Squash to "selftests/bpf: Add bpf scheduler test"