1. Patchew
  2. MPTCP
  3. [PATCH mptcp-next 0/4] run sched tests in a dedicated ns
  4. View patch

[PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"

Geliang Tang posted 4 patches 2 years, 7 months ago
Maintainers: Matthieu Baerts <matthieu.baerts@tessares.net>, Mat Martineau <martineau@kernel.org>, Andrii Nakryiko <andrii@kernel.org>, Mykola Lysenko <mykolal@fb.com>, Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, Martin KaFai Lau <martin.lau@linux.dev>, Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>, John Fastabend <john.fastabend@gmail.com>, KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>, Shuah Khan <shuah@kernel.org>
There is a newer version of this series
[PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
Posted by Geliang Tang 2 years, 7 months ago 
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
Re: [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
Posted by Matthieu Baerts 2 years, 7 months ago 
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
Re: [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
Posted by Geliang Tang 2 years, 7 months ago 
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
>
Re: [PATCH mptcp-next 1/4] Squash to "selftests/bpf: Add bpf_first test"
Posted by Matthieu Baerts 2 years, 7 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.