diff mbox series

[bpf-next,v2,4/5] selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton

Message ID 160417035105.2823.2453428685023319711.stgit@localhost.localdomain (mailing list archive)
State Accepted
Delegated to: BPF
Headers show
Series selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework | expand

Commit Message

Alexander Duyck Oct. 31, 2020, 6:52 p.m. UTC
From: Alexander Duyck <alexanderduyck@fb.com>

Update tcpbpf_user.c to make use of the BPF skeleton. Doing this we can
simplify test_tcpbpf_user and reduce the overhead involved in setting up
the test.

In addition we can clean up the remaining bits such as the one remaining
CHECK_FAIL at the end of test_tcpbpf_user so that the function only makes
use of CHECK as needed.

Acked-by: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   48 ++++++++------------
 1 file changed, 18 insertions(+), 30 deletions(-)

Comments

Martin KaFai Lau Nov. 3, 2020, 12:55 a.m. UTC | #1
On Sat, Oct 31, 2020 at 11:52:31AM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> Update tcpbpf_user.c to make use of the BPF skeleton. Doing this we can
> simplify test_tcpbpf_user and reduce the overhead involved in setting up
> the test.
> 
> In addition we can clean up the remaining bits such as the one remaining
> CHECK_FAIL at the end of test_tcpbpf_user so that the function only makes
> use of CHECK as needed.
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>

> ---
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   48 ++++++++------------
>  1 file changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> index d96f4084d2f5..c7a61b0d616a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> @@ -4,6 +4,7 @@
>  #include <network_helpers.h>
>  
>  #include "test_tcpbpf.h"
> +#include "test_tcpbpf_kern.skel.h"
>  
>  #define LO_ADDR6 "::1"
>  #define CG_NAME "/tcpbpf-user-test"
> @@ -133,44 +134,31 @@ static void run_test(int map_fd, int sock_map_fd)
>  
>  void test_tcpbpf_user(void)
>  {
> -	const char *file = "test_tcpbpf_kern.o";
> -	int prog_fd, map_fd, sock_map_fd;
> -	int error = EXIT_FAILURE;
> -	struct bpf_object *obj;
> +	struct test_tcpbpf_kern *skel;
> +	int map_fd, sock_map_fd;
>  	int cg_fd = -1;
> -	int rv;
> -
> -	cg_fd = test__join_cgroup(CG_NAME);
> -	if (cg_fd < 0)
> -		goto err;
>  
> -	if (bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd)) {
> -		fprintf(stderr, "FAILED: load_bpf_file failed for: %s\n", file);
> -		goto err;
> -	}
> +	skel = test_tcpbpf_kern__open_and_load();
> +	if (CHECK(!skel, "open and load skel", "failed"))
> +		return;
>  
> -	rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
> -	if (rv) {
> -		fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n",
> -		       errno, strerror(errno));
> -		goto err;
> -	}
> +	cg_fd = test__join_cgroup(CG_NAME);
> +	if (CHECK(cg_fd < 0, "test__join_cgroup(" CG_NAME ")",
> +		  "cg_fd:%d errno:%d", cg_fd, errno))
> +		goto cleanup_skel;
>  
> -	map_fd = bpf_find_map(__func__, obj, "global_map");
> -	if (map_fd < 0)
> -		goto err;
> +	map_fd = bpf_map__fd(skel->maps.global_map);
> +	sock_map_fd = bpf_map__fd(skel->maps.sockopt_results);
>  
> -	sock_map_fd = bpf_find_map(__func__, obj, "sockopt_results");
> -	if (sock_map_fd < 0)
> -		goto err;
> +	skel->links.bpf_testcb = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd);
> +	if (ASSERT_OK_PTR(skel->links.bpf_testcb, "attach_cgroup(bpf_testcb)"))
> +		goto cleanup_namespace;
>  
>  	run_test(map_fd, sock_map_fd);
>  
> -	error = 0;
> -err:
> -	bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> +cleanup_namespace:
nit.

may be "cleanup_cgroup" instead?

or only have one jump label to handle failure since "cg_fd != -1" has been
tested already.

>  	if (cg_fd != -1)
>  		close(cg_fd);
> -
> -	CHECK_FAIL(error);
> +cleanup_skel:
> +	test_tcpbpf_kern__destroy(skel);
>  }
> 
>
Alexander Duyck Nov. 3, 2020, 3:44 p.m. UTC | #2
On Mon, Nov 2, 2020 at 4:55 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:31AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@fb.com>
> >
> > Update tcpbpf_user.c to make use of the BPF skeleton. Doing this we can
> > simplify test_tcpbpf_user and reduce the overhead involved in setting up
> > the test.
> >
> > In addition we can clean up the remaining bits such as the one remaining
> > CHECK_FAIL at the end of test_tcpbpf_user so that the function only makes
> > use of CHECK as needed.
> >
> > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> > ---
> >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   48 ++++++++------------
> >  1 file changed, 18 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > index d96f4084d2f5..c7a61b0d616a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > @@ -4,6 +4,7 @@
> >  #include <network_helpers.h>
> >
> >  #include "test_tcpbpf.h"
> > +#include "test_tcpbpf_kern.skel.h"
> >
> >  #define LO_ADDR6 "::1"
> >  #define CG_NAME "/tcpbpf-user-test"
> > @@ -133,44 +134,31 @@ static void run_test(int map_fd, int sock_map_fd)
> >
> >  void test_tcpbpf_user(void)
> >  {
> > -     const char *file = "test_tcpbpf_kern.o";
> > -     int prog_fd, map_fd, sock_map_fd;
> > -     int error = EXIT_FAILURE;
> > -     struct bpf_object *obj;
> > +     struct test_tcpbpf_kern *skel;
> > +     int map_fd, sock_map_fd;
> >       int cg_fd = -1;
> > -     int rv;
> > -
> > -     cg_fd = test__join_cgroup(CG_NAME);
> > -     if (cg_fd < 0)
> > -             goto err;
> >
> > -     if (bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd)) {
> > -             fprintf(stderr, "FAILED: load_bpf_file failed for: %s\n", file);
> > -             goto err;
> > -     }
> > +     skel = test_tcpbpf_kern__open_and_load();
> > +     if (CHECK(!skel, "open and load skel", "failed"))
> > +             return;
> >
> > -     rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
> > -     if (rv) {
> > -             fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n",
> > -                    errno, strerror(errno));
> > -             goto err;
> > -     }
> > +     cg_fd = test__join_cgroup(CG_NAME);
> > +     if (CHECK(cg_fd < 0, "test__join_cgroup(" CG_NAME ")",
> > +               "cg_fd:%d errno:%d", cg_fd, errno))
> > +             goto cleanup_skel;
> >
> > -     map_fd = bpf_find_map(__func__, obj, "global_map");
> > -     if (map_fd < 0)
> > -             goto err;
> > +     map_fd = bpf_map__fd(skel->maps.global_map);
> > +     sock_map_fd = bpf_map__fd(skel->maps.sockopt_results);
> >
> > -     sock_map_fd = bpf_find_map(__func__, obj, "sockopt_results");
> > -     if (sock_map_fd < 0)
> > -             goto err;
> > +     skel->links.bpf_testcb = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd);
> > +     if (ASSERT_OK_PTR(skel->links.bpf_testcb, "attach_cgroup(bpf_testcb)"))
> > +             goto cleanup_namespace;
> >
> >       run_test(map_fd, sock_map_fd);
> >
> > -     error = 0;
> > -err:
> > -     bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
> > +cleanup_namespace:
> nit.
>
> may be "cleanup_cgroup" instead?
>
> or only have one jump label to handle failure since "cg_fd != -1" has been
> tested already.

Good point. I can go through and just drop the second label and
simplify this. Will fix for v3.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index d96f4084d2f5..c7a61b0d616a 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
@@ -4,6 +4,7 @@ 
 #include <network_helpers.h>
 
 #include "test_tcpbpf.h"
+#include "test_tcpbpf_kern.skel.h"
 
 #define LO_ADDR6 "::1"
 #define CG_NAME "/tcpbpf-user-test"
@@ -133,44 +134,31 @@  static void run_test(int map_fd, int sock_map_fd)
 
 void test_tcpbpf_user(void)
 {
-	const char *file = "test_tcpbpf_kern.o";
-	int prog_fd, map_fd, sock_map_fd;
-	int error = EXIT_FAILURE;
-	struct bpf_object *obj;
+	struct test_tcpbpf_kern *skel;
+	int map_fd, sock_map_fd;
 	int cg_fd = -1;
-	int rv;
-
-	cg_fd = test__join_cgroup(CG_NAME);
-	if (cg_fd < 0)
-		goto err;
 
-	if (bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd)) {
-		fprintf(stderr, "FAILED: load_bpf_file failed for: %s\n", file);
-		goto err;
-	}
+	skel = test_tcpbpf_kern__open_and_load();
+	if (CHECK(!skel, "open and load skel", "failed"))
+		return;
 
-	rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
-	if (rv) {
-		fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n",
-		       errno, strerror(errno));
-		goto err;
-	}
+	cg_fd = test__join_cgroup(CG_NAME);
+	if (CHECK(cg_fd < 0, "test__join_cgroup(" CG_NAME ")",
+		  "cg_fd:%d errno:%d", cg_fd, errno))
+		goto cleanup_skel;
 
-	map_fd = bpf_find_map(__func__, obj, "global_map");
-	if (map_fd < 0)
-		goto err;
+	map_fd = bpf_map__fd(skel->maps.global_map);
+	sock_map_fd = bpf_map__fd(skel->maps.sockopt_results);
 
-	sock_map_fd = bpf_find_map(__func__, obj, "sockopt_results");
-	if (sock_map_fd < 0)
-		goto err;
+	skel->links.bpf_testcb = bpf_program__attach_cgroup(skel->progs.bpf_testcb, cg_fd);
+	if (ASSERT_OK_PTR(skel->links.bpf_testcb, "attach_cgroup(bpf_testcb)"))
+		goto cleanup_namespace;
 
 	run_test(map_fd, sock_map_fd);
 
-	error = 0;
-err:
-	bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
+cleanup_namespace:
 	if (cg_fd != -1)
 		close(cg_fd);
-
-	CHECK_FAIL(error);
+cleanup_skel:
+	test_tcpbpf_kern__destroy(skel);
 }