All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH mptcp-next 0/4] run sched tests in a dedicated ns
@ 2023-05-17  4:32 Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test" Geliang Tang
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Geliang Tang @ 2023-05-17  4:32 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

Run mptcp sched tests in a dedicated netns.

Geliang Tang (4):
  Squash to "selftests/bpf: Add bpf_first test"
  Squash to "selftests/bpf: Add bpf_bkup test"
  Squash to "selftests/bpf: Add bpf_rr test"
  Squash to "selftests/bpf: Add bpf_red test"

 .../testing/selftests/bpf/prog_tests/mptcp.c  | 88 ++++++++++++++-----
 1 file changed, 67 insertions(+), 21 deletions(-)

-- 
2.35.3


^ permalink raw reply	[flat|nested] 9+ messages in thread
* [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
  2023-05-17  4:32 [PATCH mptcp-next 0/4] run sched tests in a dedicated ns Geliang Tang
@ 2023-05-17  4:32 ` Geliang Tang
  2023-05-17  7:59   ` Matthieu Baerts
  2023-05-17  4:32 ` [PATCH mptcp-next 2/4] Squash to "selftests/bpf: Add bpf_bkup test" Geliang Tang
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Geliang Tang @ 2023-05-17  4:32 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

Run mptcp sched test in a dedicated netns.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 .../testing/selftests/bpf/prog_tests/mptcp.c  | 61 ++++++++++++++-----
 1 file changed, 46 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index de8b6c68fc30..c6fd5c2449f3 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -271,42 +271,69 @@ static void send_data(int lfd, int fd)
 #define ADDR_1	"10.0.1.1"
 #define ADDR_2	"10.0.1.2"
 
-static void sched_init(char *flags, char *sched)
+static struct nstoken *sched_init(char *flags, char *sched)
 {
+	struct nstoken *nstoken = NULL;
 	char cmd[64];
 
-	system("ip link add veth1 type veth peer name veth2");
-	snprintf(cmd, sizeof(cmd), "ip addr add %s/24 dev veth1", ADDR_1);
+	SYS(fail, "ip netns add %s", NS_TEST);
+	SYS(fail, "ip -net %s link set dev lo up", NS_TEST);
+
+	nstoken = open_netns(NS_TEST);
+	if (!ASSERT_OK_PTR(nstoken, "open_netns"))
+		goto fail;
+
+	snprintf(cmd, sizeof(cmd), "ip -net %s link add veth1 type veth peer name veth2", NS_TEST);
+	system(cmd);
+	snprintf(cmd, sizeof(cmd), "ip -net %s addr add %s/24 dev veth1", NS_TEST, ADDR_1);
+	system(cmd);
+	snprintf(cmd, sizeof(cmd), "ip -net %s link set veth1 up", NS_TEST);
+	system(cmd);
+	snprintf(cmd, sizeof(cmd), "ip -net %s addr add %s/24 dev veth2", NS_TEST, ADDR_2);
 	system(cmd);
-	system("ip link set veth1 up");
-	snprintf(cmd, sizeof(cmd), "ip addr add %s/24 dev veth2", ADDR_2);
+	snprintf(cmd, sizeof(cmd), "ip -net %s link set veth2 up", NS_TEST);
 	system(cmd);
-	system("ip link set veth2 up");
 
-	snprintf(cmd, sizeof(cmd), "ip mptcp endpoint add %s %s", ADDR_2, flags);
+	snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint add %s %s", NS_TEST, ADDR_2, flags);
 	system(cmd);
-	snprintf(cmd, sizeof(cmd), "sysctl -qw net.mptcp.scheduler=%s", sched);
+	snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s",
+		 NS_TEST, sched);
 	system(cmd);
+
+fail:
+	return nstoken;
 }
 
-static void sched_cleanup(void)
+static void sched_cleanup(struct nstoken *nstoken)
 {
-	system("sysctl -qw net.mptcp.scheduler=default");
-	system("ip mptcp endpoint flush");
-	system("ip link del veth1");
+	char cmd[64];
+
+	snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=default",
+		 NS_TEST);
+	system(cmd);
+	snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint flush", NS_TEST);
+	system(cmd);
+	snprintf(cmd, sizeof(cmd), "ip -net %s link del veth1", NS_TEST);
+	system(cmd);
+
+	if (nstoken)
+		close_netns(nstoken);
+	SYS_NOFAIL("ip netns del " NS_TEST " &> /dev/null");
 }
 
 static int has_bytes_sent(char *addr)
 {
 	char cmd[64];
 
-	snprintf(cmd, sizeof(cmd), "ss -it dst %s | grep -q 'bytes_sent:'", addr);
+	snprintf(cmd, sizeof(cmd), "ip netns exec %s ss -it dst %s | grep -q bytes_sent:",
+		 NS_TEST, addr);
 	return system(cmd);
 }
 
 static void test_first(void)
 {
 	struct mptcp_bpf_first *first_skel;
+	struct nstoken *nstoken = NULL;
 	int server_fd, client_fd;
 	struct bpf_link *link;
 
@@ -320,7 +347,10 @@ static void test_first(void)
 		return;
 	}
 
-	sched_init("subflow", "bpf_first");
+	nstoken = sched_init("subflow", "bpf_first");
+	if (!ASSERT_OK_PTR(nstoken, "sched_init"))
+		goto fail;
+
 	server_fd = start_mptcp_server(AF_INET, ADDR_1, 0, 0);
 	client_fd = connect_to_fd(server_fd, 0);
 
@@ -330,7 +360,8 @@ static void test_first(void)
 
 	close(client_fd);
 	close(server_fd);
-	sched_cleanup();
+fail:
+	sched_cleanup(nstoken);
 	bpf_link__destroy(link);
 	mptcp_bpf_first__destroy(first_skel);
 }
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH mptcp-next 2/4] Squash to "selftests/bpf: Add bpf_bkup test"
  2023-05-17  4:32 [PATCH mptcp-next 0/4] run sched tests in a dedicated ns Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test" Geliang Tang
@ 2023-05-17  4:32 ` Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 3/4] Squash to "selftests/bpf: Add bpf_rr test" Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 4/4] Squash to "selftests/bpf: Add bpf_red test" Geliang Tang
  3 siblings, 0 replies; 9+ messages in thread
From: Geliang Tang @ 2023-05-17  4:32 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

Run mptcp sched test in a dedicated netns.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index c6fd5c2449f3..ee5ab996dc2e 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -369,6 +369,7 @@ static void test_first(void)
 static void test_bkup(void)
 {
 	struct mptcp_bpf_bkup *bkup_skel;
+	struct nstoken *nstoken = NULL;
 	int server_fd, client_fd;
 	struct bpf_link *link;
 
@@ -382,7 +383,10 @@ static void test_bkup(void)
 		return;
 	}
 
-	sched_init("subflow backup", "bpf_bkup");
+	nstoken = sched_init("subflow backup", "bpf_bkup");
+	if (!ASSERT_OK_PTR(nstoken, "sched_init"))
+		goto fail;
+
 	server_fd = start_mptcp_server(AF_INET, ADDR_1, 0, 0);
 	client_fd = connect_to_fd(server_fd, 0);
 
@@ -392,7 +396,8 @@ static void test_bkup(void)
 
 	close(client_fd);
 	close(server_fd);
-	sched_cleanup();
+fail:
+	sched_cleanup(nstoken);
 	bpf_link__destroy(link);
 	mptcp_bpf_bkup__destroy(bkup_skel);
 }
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH mptcp-next 3/4] Squash to "selftests/bpf: Add bpf_rr test"
  2023-05-17  4:32 [PATCH mptcp-next 0/4] run sched tests in a dedicated ns Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test" Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 2/4] Squash to "selftests/bpf: Add bpf_bkup test" Geliang Tang
@ 2023-05-17  4:32 ` Geliang Tang
  2023-05-17  4:32 ` [PATCH mptcp-next 4/4] Squash to "selftests/bpf: Add bpf_red test" Geliang Tang
  3 siblings, 0 replies; 9+ messages in thread
From: Geliang Tang @ 2023-05-17  4:32 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

Run mptcp sched test in a dedicated netns.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index ee5ab996dc2e..95b9876710fa 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -404,6 +404,7 @@ static void test_bkup(void)
 
 static void test_rr(void)
 {
+	struct nstoken *nstoken = NULL;
 	struct mptcp_bpf_rr *rr_skel;
 	int server_fd, client_fd;
 	struct bpf_link *link;
@@ -418,7 +419,10 @@ static void test_rr(void)
 		return;
 	}
 
-	sched_init("subflow", "bpf_rr");
+	nstoken = sched_init("subflow", "bpf_rr");
+	if (!ASSERT_OK_PTR(nstoken, "sched_init"))
+		goto fail;
+
 	server_fd = start_mptcp_server(AF_INET, ADDR_1, 0, 0);
 	client_fd = connect_to_fd(server_fd, 0);
 
@@ -428,7 +432,8 @@ static void test_rr(void)
 
 	close(client_fd);
 	close(server_fd);
-	sched_cleanup();
+fail:
+	sched_cleanup(nstoken);
 	bpf_link__destroy(link);
 	mptcp_bpf_rr__destroy(rr_skel);
 }
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* [PATCH mptcp-next 4/4] Squash to "selftests/bpf: Add bpf_red test"
  2023-05-17  4:32 [PATCH mptcp-next 0/4] run sched tests in a dedicated ns Geliang Tang
                   ` (2 preceding siblings ...)
  2023-05-17  4:32 ` [PATCH mptcp-next 3/4] Squash to "selftests/bpf: Add bpf_rr test" Geliang Tang
@ 2023-05-17  4:32 ` Geliang Tang
  2023-05-17  6:03   ` Squash to "selftests/bpf: Add bpf_red test": Tests Results MPTCP CI
  3 siblings, 1 reply; 9+ messages in thread
From: Geliang Tang @ 2023-05-17  4:32 UTC (permalink / raw)
  To: mptcp; +Cc: Geliang Tang

Run mptcp sched test in a dedicated netns.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 tools/testing/selftests/bpf/prog_tests/mptcp.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
index 95b9876710fa..24ea3b89aa6c 100644
--- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
+++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
@@ -441,6 +441,7 @@ static void test_rr(void)
 static void test_red(void)
 {
 	struct mptcp_bpf_red *red_skel;
+	struct nstoken *nstoken = NULL;
 	int server_fd, client_fd;
 	struct bpf_link *link;
 
@@ -454,7 +455,10 @@ static void test_red(void)
 		return;
 	}
 
-	sched_init("subflow", "bpf_red");
+	nstoken = sched_init("subflow", "bpf_red");
+	if (!ASSERT_OK_PTR(nstoken, "sched_init"))
+		goto fail;
+
 	server_fd = start_mptcp_server(AF_INET, ADDR_1, 0, 0);
 	client_fd = connect_to_fd(server_fd, 0);
 
@@ -464,7 +468,8 @@ static void test_red(void)
 
 	close(client_fd);
 	close(server_fd);
-	sched_cleanup();
+fail:
+	sched_cleanup(nstoken);
 	bpf_link__destroy(link);
 	mptcp_bpf_red__destroy(red_skel);
 }
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 9+ messages in thread
* Re: Squash to "selftests/bpf: Add bpf_red test": Tests Results
  2023-05-17  4:32 ` [PATCH mptcp-next 4/4] Squash to "selftests/bpf: Add bpf_red test" Geliang Tang
@ 2023-05-17  6:03   ` MPTCP CI
  0 siblings, 0 replies; 9+ messages in thread
From: MPTCP CI @ 2023-05-17  6:03 UTC (permalink / raw)
  To: Geliang Tang; +Cc: mptcp

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal (except selftest_mptcp_join):
  - Unstable: 1 failed test(s): packetdrill_fastopen 🔴:
  - Task: https://cirrus-ci.com/task/4555438918729728
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/4555438918729728/summary/summary.txt

- KVM Validation: debug (except selftest_mptcp_join):
  - Unstable: 4 failed test(s): packetdrill_add_addr packetdrill_fastopen packetdrill_mp_capable selftest_diag 🔴:
  - Task: https://cirrus-ci.com/task/5118388872151040
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5118388872151040/summary/summary.txt

- KVM Validation: debug (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/6244288778993664
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/6244288778993664/summary/summary.txt

- KVM Validation: normal (only selftest_mptcp_join):
  - Success! ✅:
  - Task: https://cirrus-ci.com/task/5681338825572352
  - Summary: https://api.cirrus-ci.com/v1/artifact/task/5681338825572352/summary/summary.txt

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/38f1f1e48833


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-debug

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (Tessares)

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
  2023-05-17  4:32 ` [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test" Geliang Tang
@ 2023-05-17  7:59   ` Matthieu Baerts
  2023-05-17  8:25     ` Geliang Tang
  0 siblings, 1 reply; 9+ messages in thread
From: Matthieu Baerts @ 2023-05-17  7:59 UTC (permalink / raw)
  To: Geliang Tang, mptcp

Hi Geliang,

On 17/05/2023 06:32, Geliang Tang wrote:
> Run mptcp sched test in a dedicated netns.

Thank you for looking at this!

I just have a few comments and ideas below (the last one is also valid
for the patches 2 to 4/4).

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  .../testing/selftests/bpf/prog_tests/mptcp.c  | 61 ++++++++++++++-----
>  1 file changed, 46 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> index de8b6c68fc30..c6fd5c2449f3 100644
> --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> @@ -271,42 +271,69 @@ static void send_data(int lfd, int fd)
>  #define ADDR_1	"10.0.1.1"
>  #define ADDR_2	"10.0.1.2"
>  
> -static void sched_init(char *flags, char *sched)
> +static struct nstoken *sched_init(char *flags, char *sched)
>  {
> +	struct nstoken *nstoken = NULL;
>  	char cmd[64];
>  
> -	system("ip link add veth1 type veth peer name veth2");
> -	snprintf(cmd, sizeof(cmd), "ip addr add %s/24 dev veth1", ADDR_1);
> +	SYS(fail, "ip netns add %s", NS_TEST);
> +	SYS(fail, "ip -net %s link set dev lo up", NS_TEST);
> +
> +	nstoken = open_netns(NS_TEST);
> +	if (!ASSERT_OK_PTR(nstoken, "open_netns"))
> +		goto fail;
> +
> +	snprintf(cmd, sizeof(cmd), "ip -net %s link add veth1 type veth peer name veth2", NS_TEST);
> +	system(cmd);

While at it, should you not use here and below:

  SYS(fail, "...", ...);

... instead of snprintf() + system()?

With that, we will be able to catch issues with the commands here and below.

(also, cmd[64] might be too small, in SYS(), they use 1024)

> +	snprintf(cmd, sizeof(cmd), "ip -net %s addr add %s/24 dev veth1", NS_TEST, ADDR_1);
> +	system(cmd);
> +	snprintf(cmd, sizeof(cmd), "ip -net %s link set veth1 up", NS_TEST);
> +	system(cmd);
> +	snprintf(cmd, sizeof(cmd), "ip -net %s addr add %s/24 dev veth2", NS_TEST, ADDR_2);
>  	system(cmd);
> -	system("ip link set veth1 up");
> -	snprintf(cmd, sizeof(cmd), "ip addr add %s/24 dev veth2", ADDR_2);
> +	snprintf(cmd, sizeof(cmd), "ip -net %s link set veth2 up", NS_TEST);
>  	system(cmd);
> -	system("ip link set veth2 up");
>  
> -	snprintf(cmd, sizeof(cmd), "ip mptcp endpoint add %s %s", ADDR_2, flags);
> +	snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint add %s %s", NS_TEST, ADDR_2, flags);
>  	system(cmd);
> -	snprintf(cmd, sizeof(cmd), "sysctl -qw net.mptcp.scheduler=%s", sched);
> +	snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s",
> +		 NS_TEST, sched);
>  	system(cmd);
> +
> +fail:
> +	return nstoken;
>  }
>  
> -static void sched_cleanup(void)
> +static void sched_cleanup(struct nstoken *nstoken)
>  {
> -	system("sysctl -qw net.mptcp.scheduler=default");
> -	system("ip mptcp endpoint flush");
> -	system("ip link del veth1");
> +	char cmd[64];
> +
> +	snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=default",
> +		 NS_TEST);
> +	system(cmd);
> +	snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint flush", NS_TEST);
> +	system(cmd);
> +	snprintf(cmd, sizeof(cmd), "ip -net %s link del veth1", NS_TEST);
> +	system(cmd);
> +
> +	if (nstoken)
> +		close_netns(nstoken);
> +	SYS_NOFAIL("ip netns del " NS_TEST " &> /dev/null");

Is it not enough to just destroy the netns? So just the two last steps.

>  }
>  
>  static int has_bytes_sent(char *addr)
>  {
>  	char cmd[64];
>  
> -	snprintf(cmd, sizeof(cmd), "ss -it dst %s | grep -q 'bytes_sent:'", addr);
> +	snprintf(cmd, sizeof(cmd), "ip netns exec %s ss -it dst %s | grep -q bytes_sent:",
> +		 NS_TEST, addr);

Do you not need a larger size for 'cmd'? Maybe safer to bump it to 128
(or more) because the following is 65 bytes long (including the \0 at
the end):

  ip netns exec mptcp_ns ss -it dst 10.0.1.1 | grep -q bytes_sent:


>  	return system(cmd);
>  }
>  
>  static void test_first(void)
>  {
>  	struct mptcp_bpf_first *first_skel;
> +	struct nstoken *nstoken = NULL;
>  	int server_fd, client_fd;
>  	struct bpf_link *link;
>  
> @@ -320,7 +347,10 @@ static void test_first(void)
>  		return;
>  	}
>  
> -	sched_init("subflow", "bpf_first");
> +	nstoken = sched_init("subflow", "bpf_first");
> +	if (!ASSERT_OK_PTR(nstoken, "sched_init"))

It might be good to add the name of the test in the error not to have
the same error for all subtests, no?

  if (!ASSERT_OK_PTR(nstoken, "sched_init: bpf_first"))

(same for the other tests)

> +		goto fail;
> +
>  	server_fd = start_mptcp_server(AF_INET, ADDR_1, 0, 0);
>  	client_fd = connect_to_fd(server_fd, 0);
>  
> @@ -330,7 +360,8 @@ static void test_first(void)
>  
>  	close(client_fd);
>  	close(server_fd);
> -	sched_cleanup();
> +fail:
> +	sched_cleanup(nstoken);
>  	bpf_link__destroy(link);
>  	mptcp_bpf_first__destroy(first_skel);
>  }

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
  2023-05-17  7:59   ` Matthieu Baerts
@ 2023-05-17  8:25     ` Geliang Tang
  2023-05-17  8:41       ` Matthieu Baerts
  0 siblings, 1 reply; 9+ messages in thread
From: Geliang Tang @ 2023-05-17  8:25 UTC (permalink / raw)
  To: Matthieu Baerts; +Cc: Geliang Tang, mptcp

Hi Matt,

Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年5月17日周三 15:59写道：
>
> Hi Geliang,
>
> On 17/05/2023 06:32, Geliang Tang wrote:
> > Run mptcp sched test in a dedicated netns.
>
> Thank you for looking at this!
>
> I just have a few comments and ideas below (the last one is also valid
> for the patches 2 to 4/4).
>
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  .../testing/selftests/bpf/prog_tests/mptcp.c  | 61 ++++++++++++++-----
> >  1 file changed, 46 insertions(+), 15 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/mptcp.c b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > index de8b6c68fc30..c6fd5c2449f3 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/mptcp.c
> > @@ -271,42 +271,69 @@ static void send_data(int lfd, int fd)
> >  #define ADDR_1       "10.0.1.1"
> >  #define ADDR_2       "10.0.1.2"
> >
> > -static void sched_init(char *flags, char *sched)
> > +static struct nstoken *sched_init(char *flags, char *sched)
> >  {
> > +     struct nstoken *nstoken = NULL;
> >       char cmd[64];
> >
> > -     system("ip link add veth1 type veth peer name veth2");
> > -     snprintf(cmd, sizeof(cmd), "ip addr add %s/24 dev veth1", ADDR_1);
> > +     SYS(fail, "ip netns add %s", NS_TEST);
> > +     SYS(fail, "ip -net %s link set dev lo up", NS_TEST);
> > +
> > +     nstoken = open_netns(NS_TEST);
> > +     if (!ASSERT_OK_PTR(nstoken, "open_netns"))
> > +             goto fail;
> > +
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s link add veth1 type veth peer name veth2", NS_TEST);
> > +     system(cmd);
>
> While at it, should you not use here and below:
>
>   SYS(fail, "...", ...);
>
> ... instead of snprintf() + system()?
>
> With that, we will be able to catch issues with the commands here and below.
>
> (also, cmd[64] might be too small, in SYS(), they use 1024)
>
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s addr add %s/24 dev veth1", NS_TEST, ADDR_1);
> > +     system(cmd);
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s link set veth1 up", NS_TEST);
> > +     system(cmd);
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s addr add %s/24 dev veth2", NS_TEST, ADDR_2);
> >       system(cmd);
> > -     system("ip link set veth1 up");
> > -     snprintf(cmd, sizeof(cmd), "ip addr add %s/24 dev veth2", ADDR_2);
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s link set veth2 up", NS_TEST);
> >       system(cmd);
> > -     system("ip link set veth2 up");
> >
> > -     snprintf(cmd, sizeof(cmd), "ip mptcp endpoint add %s %s", ADDR_2, flags);
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint add %s %s", NS_TEST, ADDR_2, flags);
> >       system(cmd);
> > -     snprintf(cmd, sizeof(cmd), "sysctl -qw net.mptcp.scheduler=%s", sched);
> > +     snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=%s",
> > +              NS_TEST, sched);
> >       system(cmd);
> > +
> > +fail:
> > +     return nstoken;
> >  }
> >
> > -static void sched_cleanup(void)
> > +static void sched_cleanup(struct nstoken *nstoken)
> >  {
> > -     system("sysctl -qw net.mptcp.scheduler=default");
> > -     system("ip mptcp endpoint flush");
> > -     system("ip link del veth1");
> > +     char cmd[64];
> > +
> > +     snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=default",
> > +              NS_TEST);
> > +     system(cmd);
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint flush", NS_TEST);
> > +     system(cmd);
> > +     snprintf(cmd, sizeof(cmd), "ip -net %s link del veth1", NS_TEST);
> > +     system(cmd);
> > +
> > +     if (nstoken)
> > +             close_netns(nstoken);
> > +     SYS_NOFAIL("ip netns del " NS_TEST " &> /dev/null");
>
> Is it not enough to just destroy the netns? So just the two last steps.

Sorry, I didn't get this. Could you please explain more about it. Thanks.

-Geliang

>
> >  }
> >
> >  static int has_bytes_sent(char *addr)
> >  {
> >       char cmd[64];
> >
> > -     snprintf(cmd, sizeof(cmd), "ss -it dst %s | grep -q 'bytes_sent:'", addr);
> > +     snprintf(cmd, sizeof(cmd), "ip netns exec %s ss -it dst %s | grep -q bytes_sent:",
> > +              NS_TEST, addr);
>
> Do you not need a larger size for 'cmd'? Maybe safer to bump it to 128
> (or more) because the following is 65 bytes long (including the \0 at
> the end):
>
>   ip netns exec mptcp_ns ss -it dst 10.0.1.1 | grep -q bytes_sent:
>
>
> >       return system(cmd);
> >  }
> >
> >  static void test_first(void)
> >  {
> >       struct mptcp_bpf_first *first_skel;
> > +     struct nstoken *nstoken = NULL;
> >       int server_fd, client_fd;
> >       struct bpf_link *link;
> >
> > @@ -320,7 +347,10 @@ static void test_first(void)
> >               return;
> >       }
> >
> > -     sched_init("subflow", "bpf_first");
> > +     nstoken = sched_init("subflow", "bpf_first");
> > +     if (!ASSERT_OK_PTR(nstoken, "sched_init"))
>
> It might be good to add the name of the test in the error not to have
> the same error for all subtests, no?
>
>   if (!ASSERT_OK_PTR(nstoken, "sched_init: bpf_first"))
>
> (same for the other tests)
>
> > +             goto fail;
> > +
> >       server_fd = start_mptcp_server(AF_INET, ADDR_1, 0, 0);
> >       client_fd = connect_to_fd(server_fd, 0);
> >
> > @@ -330,7 +360,8 @@ static void test_first(void)
> >
> >       close(client_fd);
> >       close(server_fd);
> > -     sched_cleanup();
> > +fail:
> > +     sched_cleanup(nstoken);
> >       bpf_link__destroy(link);
> >       mptcp_bpf_first__destroy(first_skel);
> >  }
>
> Cheers,
> Matt
> --
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>

^ permalink raw reply	[flat|nested] 9+ messages in thread
* Re: [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
  2023-05-17  8:25     ` Geliang Tang
@ 2023-05-17  8:41       ` Matthieu Baerts
  0 siblings, 0 replies; 9+ messages in thread
From: Matthieu Baerts @ 2023-05-17  8:41 UTC (permalink / raw)
  To: Geliang Tang; +Cc: Geliang Tang, mptcp

Hi Geliang,

On 17/05/2023 10:25, Geliang Tang wrote:
> Matthieu Baerts <matthieu.baerts@tessares.net> 于2023年5月17日周三 15:59写道：
>> On 17/05/2023 06:32, Geliang Tang wrote:

(...)

>>> -static void sched_cleanup(void)
>>> +static void sched_cleanup(struct nstoken *nstoken)
>>>  {
>>> -     system("sysctl -qw net.mptcp.scheduler=default");
>>> -     system("ip mptcp endpoint flush");
>>> -     system("ip link del veth1");
>>> +     char cmd[64];
>>> +
>>> +     snprintf(cmd, sizeof(cmd), "ip netns exec %s sysctl -qw net.mptcp.scheduler=default",
>>> +              NS_TEST);
>>> +     system(cmd);
>>> +     snprintf(cmd, sizeof(cmd), "ip -net %s mptcp endpoint flush", NS_TEST);
>>> +     system(cmd);
>>> +     snprintf(cmd, sizeof(cmd), "ip -net %s link del veth1", NS_TEST);
>>> +     system(cmd);
>>> +
>>> +     if (nstoken)
>>> +             close_netns(nstoken);
>>> +     SYS_NOFAIL("ip netns del " NS_TEST " &> /dev/null");
>>
>> Is it not enough to just destroy the netns? So just the two last steps.
> 
> Sorry, I didn't get this. Could you please explain more about it. Thanks.

If you delete the netns, I guess that will remove everything associated
to it, no? So no need to change the sysctl, flush the endpoint and
remove the veth, no?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

^ permalink raw reply	[flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-05-17  8:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-17  4:32 [PATCH mptcp-next 0/4] run sched tests in a dedicated ns Geliang Tang
2023-05-17  4:32 ` [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test" Geliang Tang
2023-05-17  7:59   ` Matthieu Baerts
2023-05-17  8:25     ` Geliang Tang
2023-05-17  8:41       ` Matthieu Baerts
2023-05-17  4:32 ` [PATCH mptcp-next 2/4] Squash to "selftests/bpf: Add bpf_bkup test" Geliang Tang
2023-05-17  4:32 ` [PATCH mptcp-next 3/4] Squash to "selftests/bpf: Add bpf_rr test" Geliang Tang
2023-05-17  4:32 ` [PATCH mptcp-next 4/4] Squash to "selftests/bpf: Add bpf_red test" Geliang Tang
2023-05-17  6:03   ` Squash to "selftests/bpf: Add bpf_red test": Tests Results MPTCP CI

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.