[bpf-next PATCH v2 0/5] selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework

[bpf-next PATCH v2 0/5] selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework

[Alexander Duyck]

Move the test functionality from test_tcpbpf_user into the test_progs
framework so that it will be run any time the test_progs framework is run.
This will help to prevent future test escapes as the individual tests, such
as test_tcpbpf_user, are less likely to be run by developers and CI
tests.

As a part of moving it over the series goes through and updates the code to
make use of the existing APIs included in the test_progs framework. This is
meant to simplify and streamline the test code and avoid duplication of
effort.

v2: Dropped test_tcpbpf_user from .gitignore
    Replaced CHECK_FAIL calls with CHECK calls
    Minimized changes in patch 1 when moving the file
    Updated stg mail command line to display renames in submission
    Added shutdown logic to end of run_test function to guarantee close
    Added patch that replaces the two maps with use of global variables    

---

Alexander Duyck (5):
      selftests/bpf: Move test_tcppbf_user into test_progs
      selftests/bpf: Drop python client/server in favor of threads
      selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results
      selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton
      selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern


 .../selftests/bpf/prog_tests/tcpbpf_user.c    | 239 +++++++++---------
 .../selftests/bpf/progs/test_tcpbpf_kern.c    |  86 +------
 tools/testing/selftests/bpf/tcp_client.py     |  50 ----
 tools/testing/selftests/bpf/tcp_server.py     |  80 ------
 tools/testing/selftests/bpf/test_tcpbpf.h     |   2 +
 5 files changed, 135 insertions(+), 322 deletions(-)
 delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
 delete mode 100755 tools/testing/selftests/bpf/tcp_server.py

--

[bpf-next PATCH v2 1/5] selftests/bpf: Move test_tcppbf_user into test_progs

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

Recently a bug was missed due to the fact that test_tcpbpf_user is not a
part of test_progs. In order to prevent similar issues in the future move
the test functionality into test_progs. By doing this we can make certain
that it is a part of standard testing and will not be overlooked.

As a part of moving the functionality into test_progs it is necessary to
integrate with the test_progs framework and to drop any redundant code.
This patch:
1. Cleans up the include headers
2. Dropped a duplicate definition of bpf_find_map
3. Switched over to using test_progs specific cgroup functions
4. Replaced printf calls with fprintf to stderr
5. Renamed main to test_tcpbpf_user
6. Dropped return value in favor of CHECK_FAIL to check for errors

The general idea is that I wanted to keep the changes as small as possible
while moving the file into the test_progs framework. The follow-on patches
are meant to clean up the remaining issues such as the use of CHECK_FAIL.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 tools/testing/selftests/bpf/.gitignore             |    1 
 tools/testing/selftests/bpf/Makefile               |    3 -
 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   63 ++++++--------------
 3 files changed, 21 insertions(+), 46 deletions(-)
 rename tools/testing/selftests/bpf/{test_tcpbpf_user.c => prog_tests/tcpbpf_user.c} (70%)

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 3ab1200e172f..395ae040ce1f 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -8,7 +8,6 @@ FEATURE-DUMP.libbpf
 fixdep
 test_dev_cgroup
 /test_progs*
-test_tcpbpf_user
 test_verifier_log
 feature
 test_sock
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 542768f5195b..50e5b18fc455 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -32,7 +32,7 @@ LDLIBS += -lcap -lelf -lz -lrt -lpthread
 
 # Order correspond to 'make run_tests' order
 TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
-	test_verifier_log test_dev_cgroup test_tcpbpf_user \
+	test_verifier_log test_dev_cgroup \
 	test_sock test_sockmap get_cgroup_id_user test_socket_cookie \
 	test_cgroup_storage \
 	test_netcnt test_tcpnotify_user test_sysctl \
@@ -163,7 +163,6 @@ $(OUTPUT)/test_sock: cgroup_helpers.c
 $(OUTPUT)/test_sock_addr: cgroup_helpers.c
 $(OUTPUT)/test_socket_cookie: cgroup_helpers.c
 $(OUTPUT)/test_sockmap: cgroup_helpers.c
-$(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
 $(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c
 $(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
 $(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
diff --git a/tools/testing/selftests/bpf/test_tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
similarity index 70%
rename from tools/testing/selftests/bpf/test_tcpbpf_user.c
rename to tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index 74a9e49988b6..54f1dce97729 100644
--- a/tools/testing/selftests/bpf/test_tcpbpf_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
@@ -1,28 +1,18 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <inttypes.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <unistd.h>
-#include <errno.h>
-#include <string.h>
-#include <linux/bpf.h>
-#include <sys/types.h>
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "bpf_rlimit.h"
-#include "bpf_util.h"
-#include "cgroup_helpers.h"
+#include <test_progs.h>
 
 #include "test_tcpbpf.h"
 
+#define CG_NAME "/tcpbpf-user-test"
+
 /* 3 comes from one listening socket + both ends of the connection */
 #define EXPECTED_CLOSE_EVENTS		3
 
 #define EXPECT_EQ(expected, actual, fmt)			\
 	do {							\
 		if ((expected) != (actual)) {			\
-			printf("  Value of: " #actual "\n"	\
+			fprintf(stderr, "  Value of: " #actual "\n"	\
 			       "    Actual: %" fmt "\n"		\
 			       "  Expected: %" fmt "\n",	\
 			       (actual), (expected));		\
@@ -76,25 +66,11 @@ int verify_sockopt_result(int sock_map_fd)
 	return ret;
 }
 
-static int bpf_find_map(const char *test, struct bpf_object *obj,
-			const char *name)
-{
-	struct bpf_map *map;
-
-	map = bpf_object__find_map_by_name(obj, name);
-	if (!map) {
-		printf("%s:FAIL:map '%s' not found\n", test, name);
-		return -1;
-	}
-	return bpf_map__fd(map);
-}
-
-int main(int argc, char **argv)
+void test_tcpbpf_user(void)
 {
 	const char *file = "test_tcpbpf_kern.o";
 	int prog_fd, map_fd, sock_map_fd;
 	struct tcpbpf_globals g = {0};
-	const char *cg_path = "/foo";
 	int error = EXIT_FAILURE;
 	struct bpf_object *obj;
 	int cg_fd = -1;
@@ -102,24 +78,24 @@ int main(int argc, char **argv)
 	__u32 key = 0;
 	int rv;
 
-	cg_fd = cgroup_setup_and_join(cg_path);
+	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)) {
-		printf("FAILED: load_bpf_file failed for: %s\n", file);
+		fprintf(stderr, "FAILED: load_bpf_file failed for: %s\n", file);
 		goto err;
 	}
 
 	rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
 	if (rv) {
-		printf("FAILED: bpf_prog_attach: %d (%s)\n",
-		       error, strerror(errno));
+		fprintf(stderr, "FAILED: bpf_prog_attach: %d (%s)\n",
+		       errno, strerror(errno));
 		goto err;
 	}
 
 	if (system("./tcp_server.py")) {
-		printf("FAILED: TCP server\n");
+		fprintf(stderr, "FAILED: TCP server\n");
 		goto err;
 	}
 
@@ -134,32 +110,33 @@ int main(int argc, char **argv)
 retry_lookup:
 	rv = bpf_map_lookup_elem(map_fd, &key, &g);
 	if (rv != 0) {
-		printf("FAILED: bpf_map_lookup_elem returns %d\n", rv);
+		fprintf(stderr, "FAILED: bpf_map_lookup_elem returns %d\n", rv);
 		goto err;
 	}
 
 	if (g.num_close_events != EXPECTED_CLOSE_EVENTS && retry--) {
-		printf("Unexpected number of close events (%d), retrying!\n",
-		       g.num_close_events);
+		fprintf(stderr,
+			"Unexpected number of close events (%d), retrying!\n",
+			g.num_close_events);
 		usleep(100);
 		goto retry_lookup;
 	}
 
 	if (verify_result(&g)) {
-		printf("FAILED: Wrong stats\n");
+		fprintf(stderr, "FAILED: Wrong stats\n");
 		goto err;
 	}
 
 	if (verify_sockopt_result(sock_map_fd)) {
-		printf("FAILED: Wrong sockopt stats\n");
+		fprintf(stderr, "FAILED: Wrong sockopt stats\n");
 		goto err;
 	}
 
-	printf("PASSED!\n");
 	error = 0;
 err:
 	bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
-	close(cg_fd);
-	cleanup_cgroup_environment();
-	return error;
+	if (cg_fd != -1)
+		close(cg_fd);
+
+	CHECK_FAIL(error);
 }

Re: [bpf-next PATCH v2 1/5] selftests/bpf: Move test_tcppbf_user into test_progs

[Andrii Nakryiko]

On Sat, Oct 31, 2020 at 11:52 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexanderduyck@fb.com>
>
> Recently a bug was missed due to the fact that test_tcpbpf_user is not a
> part of test_progs. In order to prevent similar issues in the future move
> the test functionality into test_progs. By doing this we can make certain
> that it is a part of standard testing and will not be overlooked.
>
> As a part of moving the functionality into test_progs it is necessary to
> integrate with the test_progs framework and to drop any redundant code.
> This patch:
> 1. Cleans up the include headers
> 2. Dropped a duplicate definition of bpf_find_map
> 3. Switched over to using test_progs specific cgroup functions
> 4. Replaced printf calls with fprintf to stderr

This is not necessary. test_progs intercept both stdout and stderr, so
you could have kept the code as is and minimize this diff further. But
it also doesn't matter all that much, so:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

> 5. Renamed main to test_tcpbpf_user
> 6. Dropped return value in favor of CHECK_FAIL to check for errors
>
> The general idea is that I wanted to keep the changes as small as possible
> while moving the file into the test_progs framework. The follow-on patches
> are meant to clean up the remaining issues such as the use of CHECK_FAIL.
>
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>  tools/testing/selftests/bpf/.gitignore             |    1
>  tools/testing/selftests/bpf/Makefile               |    3 -
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   63 ++++++--------------
>  3 files changed, 21 insertions(+), 46 deletions(-)
>  rename tools/testing/selftests/bpf/{test_tcpbpf_user.c => prog_tests/tcpbpf_user.c} (70%)
>

[...]

Re: [bpf-next PATCH v2 1/5] selftests/bpf: Move test_tcppbf_user into test_progs

[Alexander Duyck]

On Tue, Nov 3, 2020 at 10:34 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52 AM Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
> >
> > From: Alexander Duyck <alexanderduyck@fb.com>
> >
> > Recently a bug was missed due to the fact that test_tcpbpf_user is not a
> > part of test_progs. In order to prevent similar issues in the future move
> > the test functionality into test_progs. By doing this we can make certain
> > that it is a part of standard testing and will not be overlooked.
> >
> > As a part of moving the functionality into test_progs it is necessary to
> > integrate with the test_progs framework and to drop any redundant code.
> > This patch:
> > 1. Cleans up the include headers
> > 2. Dropped a duplicate definition of bpf_find_map
> > 3. Switched over to using test_progs specific cgroup functions
> > 4. Replaced printf calls with fprintf to stderr
>
> This is not necessary. test_progs intercept both stdout and stderr, so
> you could have kept the code as is and minimize this diff further. But
> it also doesn't matter all that much, so:

Yeah. I can probably just drop those changes if that is preferred. As
is all the printf/fprintf calls are eliminated by the end anyway.

> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks for the review.

- Alex

[bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

Drop the tcp_client/server.py files in favor of using a client and server
thread within the test case. Specifically we spawn a new thread to play the
role of the server, and the main testing thread plays the role of client.

Add logic to the end of the run_test function to guarantee that the sockets
are closed when we begin verifying results.

Doing this we are able to reduce overhead since we don't have two python
workers possibly floating around. In addition we don't have to worry about
synchronization issues and as such the retry loop waiting for the threads
to close the sockets can be dropped as we will have already closed the
sockets in the local executable and synchronized the server thread.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   96 ++++++++++++++++----
 tools/testing/selftests/bpf/tcp_client.py          |   50 ----------
 tools/testing/selftests/bpf/tcp_server.py          |   80 -----------------
 3 files changed, 78 insertions(+), 148 deletions(-)
 delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
 delete mode 100755 tools/testing/selftests/bpf/tcp_server.py

diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index 54f1dce97729..17d4299435df 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
@@ -1,13 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <inttypes.h>
 #include <test_progs.h>
+#include <network_helpers.h>
 
 #include "test_tcpbpf.h"
 
+#define LO_ADDR6 "::1"
 #define CG_NAME "/tcpbpf-user-test"
 
-/* 3 comes from one listening socket + both ends of the connection */
-#define EXPECTED_CLOSE_EVENTS		3
+static __u32 duration;
 
 #define EXPECT_EQ(expected, actual, fmt)			\
 	do {							\
@@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result)
 	EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
 	EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
 	EXPECT_EQ(1, result->num_listen, PRIu32);
-	EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32);
+
+	/* 3 comes from one listening socket + both ends of the connection */
+	EXPECT_EQ(3, result->num_close_events, PRIu32);
 
 	return ret;
 }
@@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd)
 	return ret;
 }
 
+static int run_test(void)
+{
+	int listen_fd = -1, cli_fd = -1, accept_fd = -1;
+	char buf[1000];
+	int err = -1;
+	int i;
+
+	listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0);
+	if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n",
+		  listen_fd, errno))
+		goto done;
+
+	cli_fd = connect_to_fd(listen_fd, 0);
+	if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)",
+		  "cli_fd:%d errno:%d\n", cli_fd, errno))
+		goto done;
+
+	accept_fd = accept(listen_fd, NULL, NULL);
+	if (CHECK(accept_fd == -1, "accept(listen_fd)",
+		  "accept_fd:%d errno:%d\n", accept_fd, errno))
+		goto done;
+
+	/* Send 1000B of '+'s from cli_fd -> accept_fd */
+	for (i = 0; i < 1000; i++)
+		buf[i] = '+';
+
+	err = send(cli_fd, buf, 1000, 0);
+	if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno))
+		goto done;
+
+	err = recv(accept_fd, buf, 1000, 0);
+	if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno))
+		goto done;
+
+	/* Send 500B of '.'s from accept_fd ->cli_fd */
+	for (i = 0; i < 500; i++)
+		buf[i] = '.';
+
+	err = send(accept_fd, buf, 500, 0);
+	if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno))
+		goto done;
+
+	err = recv(cli_fd, buf, 500, 0);
+	if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno))
+		goto done;
+
+	/*
+	 * shutdown accept first to guarantee correct ordering for
+	 * bytes_received and bytes_acked when we go to verify the results.
+	 */
+	shutdown(accept_fd, SHUT_WR);
+	err = recv(cli_fd, buf, 1, 0);
+	if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno))
+		goto done;
+
+	shutdown(cli_fd, SHUT_WR);
+	err = recv(accept_fd, buf, 1, 0);
+	CHECK(err, "recv(accept_fd) for fin", "err:%d errno:%d\n", err, errno);
+done:
+	if (accept_fd != -1)
+		close(accept_fd);
+	if (cli_fd != -1)
+		close(cli_fd);
+	if (listen_fd != -1)
+		close(listen_fd);
+
+	return err;
+}
+
 void test_tcpbpf_user(void)
 {
 	const char *file = "test_tcpbpf_kern.o";
@@ -74,7 +146,6 @@ void test_tcpbpf_user(void)
 	int error = EXIT_FAILURE;
 	struct bpf_object *obj;
 	int cg_fd = -1;
-	int retry = 10;
 	__u32 key = 0;
 	int rv;
 
@@ -94,11 +165,6 @@ void test_tcpbpf_user(void)
 		goto err;
 	}
 
-	if (system("./tcp_server.py")) {
-		fprintf(stderr, "FAILED: TCP server\n");
-		goto err;
-	}
-
 	map_fd = bpf_find_map(__func__, obj, "global_map");
 	if (map_fd < 0)
 		goto err;
@@ -107,21 +173,15 @@ void test_tcpbpf_user(void)
 	if (sock_map_fd < 0)
 		goto err;
 
-retry_lookup:
+	if (run_test())
+		goto err;
+
 	rv = bpf_map_lookup_elem(map_fd, &key, &g);
 	if (rv != 0) {
 		fprintf(stderr, "FAILED: bpf_map_lookup_elem returns %d\n", rv);
 		goto err;
 	}
 
-	if (g.num_close_events != EXPECTED_CLOSE_EVENTS && retry--) {
-		fprintf(stderr,
-			"Unexpected number of close events (%d), retrying!\n",
-			g.num_close_events);
-		usleep(100);
-		goto retry_lookup;
-	}
-
 	if (verify_result(&g)) {
 		fprintf(stderr, "FAILED: Wrong stats\n");
 		goto err;
diff --git a/tools/testing/selftests/bpf/tcp_client.py b/tools/testing/selftests/bpf/tcp_client.py
deleted file mode 100755
index bfff82be3fc1..000000000000
--- a/tools/testing/selftests/bpf/tcp_client.py
+++ /dev/null
@@ -1,50 +0,0 @@
-#!/usr/bin/env python3
-#
-# SPDX-License-Identifier: GPL-2.0
-#
-
-import sys, os, os.path, getopt
-import socket, time
-import subprocess
-import select
-
-def read(sock, n):
-    buf = b''
-    while len(buf) < n:
-        rem = n - len(buf)
-        try: s = sock.recv(rem)
-        except (socket.error) as e: return b''
-        buf += s
-    return buf
-
-def send(sock, s):
-    total = len(s)
-    count = 0
-    while count < total:
-        try: n = sock.send(s)
-        except (socket.error) as e: n = 0
-        if n == 0:
-            return count;
-        count += n
-    return count
-
-
-serverPort = int(sys.argv[1])
-
-# create active socket
-sock = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
-try:
-    sock.connect(('::1', serverPort))
-except socket.error as e:
-    sys.exit(1)
-
-buf = b''
-n = 0
-while n < 1000:
-    buf += b'+'
-    n += 1
-
-sock.settimeout(1);
-n = send(sock, buf)
-n = read(sock, 500)
-sys.exit(0)
diff --git a/tools/testing/selftests/bpf/tcp_server.py b/tools/testing/selftests/bpf/tcp_server.py
deleted file mode 100755
index 42ab8882f00f..000000000000
--- a/tools/testing/selftests/bpf/tcp_server.py
+++ /dev/null
@@ -1,80 +0,0 @@
-#!/usr/bin/env python3
-#
-# SPDX-License-Identifier: GPL-2.0
-#
-
-import sys, os, os.path, getopt
-import socket, time
-import subprocess
-import select
-
-def read(sock, n):
-    buf = b''
-    while len(buf) < n:
-        rem = n - len(buf)
-        try: s = sock.recv(rem)
-        except (socket.error) as e: return b''
-        buf += s
-    return buf
-
-def send(sock, s):
-    total = len(s)
-    count = 0
-    while count < total:
-        try: n = sock.send(s)
-        except (socket.error) as e: n = 0
-        if n == 0:
-            return count;
-        count += n
-    return count
-
-
-SERVER_PORT = 12877
-MAX_PORTS = 2
-
-serverPort = SERVER_PORT
-serverSocket = None
-
-# create passive socket
-serverSocket = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
-
-try: serverSocket.bind(('::1', 0))
-except socket.error as msg:
-    print('bind fails: ' + str(msg))
-
-sn = serverSocket.getsockname()
-serverPort = sn[1]
-
-cmdStr = ("./tcp_client.py %d &") % (serverPort)
-os.system(cmdStr)
-
-buf = b''
-n = 0
-while n < 500:
-    buf += b'.'
-    n += 1
-
-serverSocket.listen(MAX_PORTS)
-readList = [serverSocket]
-
-while True:
-    readyRead, readyWrite, inError = \
-        select.select(readList, [], [], 2)
-
-    if len(readyRead) > 0:
-        waitCount = 0
-        for sock in readyRead:
-            if sock == serverSocket:
-                (clientSocket, address) = serverSocket.accept()
-                address = str(address[0])
-                readList.append(clientSocket)
-            else:
-                sock.settimeout(1);
-                s = read(sock, 1000)
-                n = send(sock, buf)
-                sock.close()
-                serverSocket.close()
-                sys.exit(0)
-    else:
-        print('Select timeout!')
-        sys.exit(1)

Re: [bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads

[Martin KaFai Lau]

On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> Drop the tcp_client/server.py files in favor of using a client and server
> thread within the test case. Specifically we spawn a new thread to play the
The thread comment may be outdated in v2.

> role of the server, and the main testing thread plays the role of client.
> 
> Add logic to the end of the run_test function to guarantee that the sockets
> are closed when we begin verifying results.
> 
> Doing this we are able to reduce overhead since we don't have two python
> workers possibly floating around. In addition we don't have to worry about
> synchronization issues and as such the retry loop waiting for the threads
> to close the sockets can be dropped as we will have already closed the
> sockets in the local executable and synchronized the server thread.
> 
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   96 ++++++++++++++++----
>  tools/testing/selftests/bpf/tcp_client.py          |   50 ----------
>  tools/testing/selftests/bpf/tcp_server.py          |   80 -----------------
>  3 files changed, 78 insertions(+), 148 deletions(-)
>  delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
>  delete mode 100755 tools/testing/selftests/bpf/tcp_server.py
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> index 54f1dce97729..17d4299435df 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> @@ -1,13 +1,14 @@
>  // SPDX-License-Identifier: GPL-2.0
>  #include <inttypes.h>
>  #include <test_progs.h>
> +#include <network_helpers.h>
>  
>  #include "test_tcpbpf.h"
>  
> +#define LO_ADDR6 "::1"
>  #define CG_NAME "/tcpbpf-user-test"
>  
> -/* 3 comes from one listening socket + both ends of the connection */
> -#define EXPECTED_CLOSE_EVENTS		3
> +static __u32 duration;
>  
>  #define EXPECT_EQ(expected, actual, fmt)			\
>  	do {							\
> @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result)
>  	EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
>  	EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
>  	EXPECT_EQ(1, result->num_listen, PRIu32);
> -	EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32);
> +
> +	/* 3 comes from one listening socket + both ends of the connection */
> +	EXPECT_EQ(3, result->num_close_events, PRIu32);
>  
>  	return ret;
>  }
> @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd)
>  	return ret;
>  }
>  
> +static int run_test(void)
> +{
> +	int listen_fd = -1, cli_fd = -1, accept_fd = -1;
> +	char buf[1000];
> +	int err = -1;
> +	int i;
> +
> +	listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0);
> +	if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n",
> +		  listen_fd, errno))
> +		goto done;
> +
> +	cli_fd = connect_to_fd(listen_fd, 0);
> +	if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)",
> +		  "cli_fd:%d errno:%d\n", cli_fd, errno))
> +		goto done;
> +
> +	accept_fd = accept(listen_fd, NULL, NULL);
> +	if (CHECK(accept_fd == -1, "accept(listen_fd)",
> +		  "accept_fd:%d errno:%d\n", accept_fd, errno))
> +		goto done;
> +
> +	/* Send 1000B of '+'s from cli_fd -> accept_fd */
> +	for (i = 0; i < 1000; i++)
> +		buf[i] = '+';
> +
> +	err = send(cli_fd, buf, 1000, 0);
> +	if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno))
> +		goto done;
> +
> +	err = recv(accept_fd, buf, 1000, 0);
> +	if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno))
> +		goto done;
> +
> +	/* Send 500B of '.'s from accept_fd ->cli_fd */
> +	for (i = 0; i < 500; i++)
> +		buf[i] = '.';
> +
> +	err = send(accept_fd, buf, 500, 0);
> +	if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno))
> +		goto done;
> +
> +	err = recv(cli_fd, buf, 500, 0);
Unlikely, but err from the above send()/recv() could be 0.


> +	if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno))
> +		goto done;
> +
> +	/*
> +	 * shutdown accept first to guarantee correct ordering for
> +	 * bytes_received and bytes_acked when we go to verify the results.
> +	 */
> +	shutdown(accept_fd, SHUT_WR);
> +	err = recv(cli_fd, buf, 1, 0);
> +	if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno))
> +		goto done;
> +
> +	shutdown(cli_fd, SHUT_WR);
> +	err = recv(accept_fd, buf, 1, 0);
hmm... I was thinking cli_fd may still be in TCP_LAST_ACK
but we can go with this version first and see if CI could
really hit this case before resurrecting the idea on testing
the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c.

> +	CHECK(err, "recv(accept_fd) for fin", "err:%d errno:%d\n", err, errno);
> +done:
> +	if (accept_fd != -1)
> +		close(accept_fd);
> +	if (cli_fd != -1)
> +		close(cli_fd);

> +	if (listen_fd != -1)
> +		close(listen_fd);
> +
> +	return err;
> +}
> +
>  void test_tcpbpf_user(void)
>  {
>  	const char *file = "test_tcpbpf_kern.o";
> @@ -74,7 +146,6 @@ void test_tcpbpf_user(void)
>  	int error = EXIT_FAILURE;
>  	struct bpf_object *obj;
>  	int cg_fd = -1;
> -	int retry = 10;
>  	__u32 key = 0;
>  	int rv;
>  
> @@ -94,11 +165,6 @@ void test_tcpbpf_user(void)
>  		goto err;
>  	}
>  
> -	if (system("./tcp_server.py")) {
> -		fprintf(stderr, "FAILED: TCP server\n");
> -		goto err;
> -	}
> -
>  	map_fd = bpf_find_map(__func__, obj, "global_map");
>  	if (map_fd < 0)
>  		goto err;
> @@ -107,21 +173,15 @@ void test_tcpbpf_user(void)
>  	if (sock_map_fd < 0)
>  		goto err;
>  
> -retry_lookup:
> +	if (run_test())
> +		goto err;
> +
>  	rv = bpf_map_lookup_elem(map_fd, &key, &g);
>  	if (rv != 0) {
>  		fprintf(stderr, "FAILED: bpf_map_lookup_elem returns %d\n", rv);
>  		goto err;
>  	}
>  
> -	if (g.num_close_events != EXPECTED_CLOSE_EVENTS && retry--) {
> -		fprintf(stderr,
> -			"Unexpected number of close events (%d), retrying!\n",
> -			g.num_close_events);
> -		usleep(100);
> -		goto retry_lookup;
> -	}
> -
>  	if (verify_result(&g)) {
>  		fprintf(stderr, "FAILED: Wrong stats\n");
>  		goto err;

Re: [bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads

[Alexander Duyck]

On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@fb.com>
> >
> > Drop the tcp_client/server.py files in favor of using a client and server
> > thread within the test case. Specifically we spawn a new thread to play the
> The thread comment may be outdated in v2.
>
> > role of the server, and the main testing thread plays the role of client.
> >
> > Add logic to the end of the run_test function to guarantee that the sockets
> > are closed when we begin verifying results.
> >
> > Doing this we are able to reduce overhead since we don't have two python
> > workers possibly floating around. In addition we don't have to worry about
> > synchronization issues and as such the retry loop waiting for the threads
> > to close the sockets can be dropped as we will have already closed the
> > sockets in the local executable and synchronized the server thread.
> >
> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> > ---
> >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   96 ++++++++++++++++----
> >  tools/testing/selftests/bpf/tcp_client.py          |   50 ----------
> >  tools/testing/selftests/bpf/tcp_server.py          |   80 -----------------
> >  3 files changed, 78 insertions(+), 148 deletions(-)
> >  delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
> >  delete mode 100755 tools/testing/selftests/bpf/tcp_server.py
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > index 54f1dce97729..17d4299435df 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > @@ -1,13 +1,14 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  #include <inttypes.h>
> >  #include <test_progs.h>
> > +#include <network_helpers.h>
> >
> >  #include "test_tcpbpf.h"
> >
> > +#define LO_ADDR6 "::1"
> >  #define CG_NAME "/tcpbpf-user-test"
> >
> > -/* 3 comes from one listening socket + both ends of the connection */
> > -#define EXPECTED_CLOSE_EVENTS                3
> > +static __u32 duration;
> >
> >  #define EXPECT_EQ(expected, actual, fmt)                     \
> >       do {                                                    \
> > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result)
> >       EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> >       EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> >       EXPECT_EQ(1, result->num_listen, PRIu32);
> > -     EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32);
> > +
> > +     /* 3 comes from one listening socket + both ends of the connection */
> > +     EXPECT_EQ(3, result->num_close_events, PRIu32);
> >
> >       return ret;
> >  }
> > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd)
> >       return ret;
> >  }
> >
> > +static int run_test(void)
> > +{
> > +     int listen_fd = -1, cli_fd = -1, accept_fd = -1;
> > +     char buf[1000];
> > +     int err = -1;
> > +     int i;
> > +
> > +     listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0);
> > +     if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n",
> > +               listen_fd, errno))
> > +             goto done;
> > +
> > +     cli_fd = connect_to_fd(listen_fd, 0);
> > +     if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)",
> > +               "cli_fd:%d errno:%d\n", cli_fd, errno))
> > +             goto done;
> > +
> > +     accept_fd = accept(listen_fd, NULL, NULL);
> > +     if (CHECK(accept_fd == -1, "accept(listen_fd)",
> > +               "accept_fd:%d errno:%d\n", accept_fd, errno))
> > +             goto done;
> > +
> > +     /* Send 1000B of '+'s from cli_fd -> accept_fd */
> > +     for (i = 0; i < 1000; i++)
> > +             buf[i] = '+';
> > +
> > +     err = send(cli_fd, buf, 1000, 0);
> > +     if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno))
> > +             goto done;
> > +
> > +     err = recv(accept_fd, buf, 1000, 0);
> > +     if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno))
> > +             goto done;
> > +
> > +     /* Send 500B of '.'s from accept_fd ->cli_fd */
> > +     for (i = 0; i < 500; i++)
> > +             buf[i] = '.';
> > +
> > +     err = send(accept_fd, buf, 500, 0);
> > +     if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno))
> > +             goto done;
> > +
> > +     err = recv(cli_fd, buf, 500, 0);
> Unlikely, but err from the above send()/recv() could be 0.

Is that an issue? It would still trigger the check below as that is not 500.

> > +     if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno))
> > +             goto done;
> > +
> > +     /*
> > +      * shutdown accept first to guarantee correct ordering for
> > +      * bytes_received and bytes_acked when we go to verify the results.
> > +      */
> > +     shutdown(accept_fd, SHUT_WR);
> > +     err = recv(cli_fd, buf, 1, 0);
> > +     if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno))
> > +             goto done;
> > +
> > +     shutdown(cli_fd, SHUT_WR);
> > +     err = recv(accept_fd, buf, 1, 0);
> hmm... I was thinking cli_fd may still be in TCP_LAST_ACK
> but we can go with this version first and see if CI could
> really hit this case before resurrecting the idea on testing
> the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c.

I ran with this for several hours and saw no issues with over 100K
iterations all of them passing. That is why I opted to just drop the
TCP_LAST_ACK patch.

Re: [bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads

[Martin KaFai Lau]

On Mon, Nov 02, 2020 at 04:49:42PM -0800, Alexander Duyck wrote:
> On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote:
> > > From: Alexander Duyck <alexanderduyck@fb.com>
> > >
> > > Drop the tcp_client/server.py files in favor of using a client and server
> > > thread within the test case. Specifically we spawn a new thread to play the
> > The thread comment may be outdated in v2.
> >
> > > role of the server, and the main testing thread plays the role of client.
> > >
> > > Add logic to the end of the run_test function to guarantee that the sockets
> > > are closed when we begin verifying results.
> > >
> > > Doing this we are able to reduce overhead since we don't have two python
> > > workers possibly floating around. In addition we don't have to worry about
> > > synchronization issues and as such the retry loop waiting for the threads
> > > to close the sockets can be dropped as we will have already closed the
> > > sockets in the local executable and synchronized the server thread.
> > >
> > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> > > ---
> > >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   96 ++++++++++++++++----
> > >  tools/testing/selftests/bpf/tcp_client.py          |   50 ----------
> > >  tools/testing/selftests/bpf/tcp_server.py          |   80 -----------------
> > >  3 files changed, 78 insertions(+), 148 deletions(-)
> > >  delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
> > >  delete mode 100755 tools/testing/selftests/bpf/tcp_server.py
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > index 54f1dce97729..17d4299435df 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > @@ -1,13 +1,14 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  #include <inttypes.h>
> > >  #include <test_progs.h>
> > > +#include <network_helpers.h>
> > >
> > >  #include "test_tcpbpf.h"
> > >
> > > +#define LO_ADDR6 "::1"
> > >  #define CG_NAME "/tcpbpf-user-test"
> > >
> > > -/* 3 comes from one listening socket + both ends of the connection */
> > > -#define EXPECTED_CLOSE_EVENTS                3
> > > +static __u32 duration;
> > >
> > >  #define EXPECT_EQ(expected, actual, fmt)                     \
> > >       do {                                                    \
> > > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result)
> > >       EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> > >       EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> > >       EXPECT_EQ(1, result->num_listen, PRIu32);
> > > -     EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32);
> > > +
> > > +     /* 3 comes from one listening socket + both ends of the connection */
> > > +     EXPECT_EQ(3, result->num_close_events, PRIu32);
> > >
> > >       return ret;
> > >  }
> > > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd)
> > >       return ret;
> > >  }
> > >
> > > +static int run_test(void)
> > > +{
> > > +     int listen_fd = -1, cli_fd = -1, accept_fd = -1;
> > > +     char buf[1000];
> > > +     int err = -1;
> > > +     int i;
> > > +
> > > +     listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0);
> > > +     if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n",
> > > +               listen_fd, errno))
> > > +             goto done;
> > > +
> > > +     cli_fd = connect_to_fd(listen_fd, 0);
> > > +     if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)",
> > > +               "cli_fd:%d errno:%d\n", cli_fd, errno))
> > > +             goto done;
> > > +
> > > +     accept_fd = accept(listen_fd, NULL, NULL);
> > > +     if (CHECK(accept_fd == -1, "accept(listen_fd)",
> > > +               "accept_fd:%d errno:%d\n", accept_fd, errno))
> > > +             goto done;
> > > +
> > > +     /* Send 1000B of '+'s from cli_fd -> accept_fd */
> > > +     for (i = 0; i < 1000; i++)
> > > +             buf[i] = '+';
> > > +
> > > +     err = send(cli_fd, buf, 1000, 0);
> > > +     if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno))
> > > +             goto done;
> > > +
> > > +     err = recv(accept_fd, buf, 1000, 0);
> > > +     if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno))
> > > +             goto done;
> > > +
> > > +     /* Send 500B of '.'s from accept_fd ->cli_fd */
> > > +     for (i = 0; i < 500; i++)
> > > +             buf[i] = '.';
> > > +
> > > +     err = send(accept_fd, buf, 500, 0);
> > > +     if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno))
> > > +             goto done;
> > > +
> > > +     err = recv(cli_fd, buf, 500, 0);
> > Unlikely, but err from the above send()/recv() could be 0.
> 
> Is that an issue? It would still trigger the check below as that is not 500.
Mostly for consistency.  "err" will be returned and tested for non-zero
in test_tcpbpf_user().

> 
> > > +     if (CHECK(err != 500, "recv(cli_fd)", "err:%d errno:%d\n", err, errno))
> > > +             goto done;
> > > +
> > > +     /*
> > > +      * shutdown accept first to guarantee correct ordering for
> > > +      * bytes_received and bytes_acked when we go to verify the results.
> > > +      */
> > > +     shutdown(accept_fd, SHUT_WR);
> > > +     err = recv(cli_fd, buf, 1, 0);
> > > +     if (CHECK(err, "recv(cli_fd) for fin", "err:%d errno:%d\n", err, errno))
> > > +             goto done;
> > > +
> > > +     shutdown(cli_fd, SHUT_WR);
> > > +     err = recv(accept_fd, buf, 1, 0);
> > hmm... I was thinking cli_fd may still be in TCP_LAST_ACK
> > but we can go with this version first and see if CI could
> > really hit this case before resurrecting the idea on testing
> > the TCP_LAST_ACK instead of TCP_CLOSE in test_tcpbpf_kern.c.
> 
> I ran with this for several hours and saw no issues with over 100K
> iterations all of them passing. That is why I opted to just drop the
> TCP_LAST_ACK patch.
Thanks for testing it hard.  It is good enough for me.

Re: [bpf-next PATCH v2 2/5] selftests/bpf: Drop python client/server in favor of threads

[Alexander Duyck]

On Mon, Nov 2, 2020 at 5:33 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Mon, Nov 02, 2020 at 04:49:42PM -0800, Alexander Duyck wrote:
> > On Mon, Nov 2, 2020 at 4:38 PM Martin KaFai Lau <kafai@fb.com> wrote:
> > >
> > > On Sat, Oct 31, 2020 at 11:52:18AM -0700, Alexander Duyck wrote:
> > > > From: Alexander Duyck <alexanderduyck@fb.com>
> > > >
> > > > Drop the tcp_client/server.py files in favor of using a client and server
> > > > thread within the test case. Specifically we spawn a new thread to play the
> > > The thread comment may be outdated in v2.
> > >
> > > > role of the server, and the main testing thread plays the role of client.
> > > >
> > > > Add logic to the end of the run_test function to guarantee that the sockets
> > > > are closed when we begin verifying results.
> > > >
> > > > Doing this we are able to reduce overhead since we don't have two python
> > > > workers possibly floating around. In addition we don't have to worry about
> > > > synchronization issues and as such the retry loop waiting for the threads
> > > > to close the sockets can be dropped as we will have already closed the
> > > > sockets in the local executable and synchronized the server thread.
> > > >
> > > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> > > > ---
> > > >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   96 ++++++++++++++++----
> > > >  tools/testing/selftests/bpf/tcp_client.py          |   50 ----------
> > > >  tools/testing/selftests/bpf/tcp_server.py          |   80 -----------------
> > > >  3 files changed, 78 insertions(+), 148 deletions(-)
> > > >  delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
> > > >  delete mode 100755 tools/testing/selftests/bpf/tcp_server.py
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > > index 54f1dce97729..17d4299435df 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > > @@ -1,13 +1,14 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  #include <inttypes.h>
> > > >  #include <test_progs.h>
> > > > +#include <network_helpers.h>
> > > >
> > > >  #include "test_tcpbpf.h"
> > > >
> > > > +#define LO_ADDR6 "::1"
> > > >  #define CG_NAME "/tcpbpf-user-test"
> > > >
> > > > -/* 3 comes from one listening socket + both ends of the connection */
> > > > -#define EXPECTED_CLOSE_EVENTS                3
> > > > +static __u32 duration;
> > > >
> > > >  #define EXPECT_EQ(expected, actual, fmt)                     \
> > > >       do {                                                    \
> > > > @@ -42,7 +43,9 @@ int verify_result(const struct tcpbpf_globals *result)
> > > >       EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> > > >       EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> > > >       EXPECT_EQ(1, result->num_listen, PRIu32);
> > > > -     EXPECT_EQ(EXPECTED_CLOSE_EVENTS, result->num_close_events, PRIu32);
> > > > +
> > > > +     /* 3 comes from one listening socket + both ends of the connection */
> > > > +     EXPECT_EQ(3, result->num_close_events, PRIu32);
> > > >
> > > >       return ret;
> > > >  }
> > > > @@ -66,6 +69,75 @@ int verify_sockopt_result(int sock_map_fd)
> > > >       return ret;
> > > >  }
> > > >
> > > > +static int run_test(void)
> > > > +{
> > > > +     int listen_fd = -1, cli_fd = -1, accept_fd = -1;
> > > > +     char buf[1000];
> > > > +     int err = -1;
> > > > +     int i;
> > > > +
> > > > +     listen_fd = start_server(AF_INET6, SOCK_STREAM, LO_ADDR6, 0, 0);
> > > > +     if (CHECK(listen_fd == -1, "start_server", "listen_fd:%d errno:%d\n",
> > > > +               listen_fd, errno))
> > > > +             goto done;
> > > > +
> > > > +     cli_fd = connect_to_fd(listen_fd, 0);
> > > > +     if (CHECK(cli_fd == -1, "connect_to_fd(listen_fd)",
> > > > +               "cli_fd:%d errno:%d\n", cli_fd, errno))
> > > > +             goto done;
> > > > +
> > > > +     accept_fd = accept(listen_fd, NULL, NULL);
> > > > +     if (CHECK(accept_fd == -1, "accept(listen_fd)",
> > > > +               "accept_fd:%d errno:%d\n", accept_fd, errno))
> > > > +             goto done;
> > > > +
> > > > +     /* Send 1000B of '+'s from cli_fd -> accept_fd */
> > > > +     for (i = 0; i < 1000; i++)
> > > > +             buf[i] = '+';
> > > > +
> > > > +     err = send(cli_fd, buf, 1000, 0);
> > > > +     if (CHECK(err != 1000, "send(cli_fd)", "err:%d errno:%d\n", err, errno))
> > > > +             goto done;
> > > > +
> > > > +     err = recv(accept_fd, buf, 1000, 0);
> > > > +     if (CHECK(err != 1000, "recv(accept_fd)", "err:%d errno:%d\n", err, errno))
> > > > +             goto done;
> > > > +
> > > > +     /* Send 500B of '.'s from accept_fd ->cli_fd */
> > > > +     for (i = 0; i < 500; i++)
> > > > +             buf[i] = '.';
> > > > +
> > > > +     err = send(accept_fd, buf, 500, 0);
> > > > +     if (CHECK(err != 500, "send(accept_fd)", "err:%d errno:%d\n", err, errno))
> > > > +             goto done;
> > > > +
> > > > +     err = recv(cli_fd, buf, 500, 0);
> > > Unlikely, but err from the above send()/recv() could be 0.
> >
> > Is that an issue? It would still trigger the check below as that is not 500.
> Mostly for consistency.  "err" will be returned and tested for non-zero
> in test_tcpbpf_user().

Okay that makes sense. Now that I have looked it over more it does
lead to an error in the final product since it will attempt to verify
data on a failed connection so I will probably just replaced err with
a new variable such as rv for the send/recv part of the function so
that err stays at -1 until we are closing the sockets.

[bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

There is already logic in test_progs.h for asserting that a value is
expected to be another value. So instead of reinventing it we should just
make use of ASSERT_EQ in tcpbpf_user.c. This will allow for better
debugging and integrates much more closely with the test_progs framework.

In addition we can refactor the code a bit to merge together the two
verify functions and tie them together into a single function. Doing this
helps to clean the code up a bit and makes it more readable as all the
verification is now done in one function.

Lastly we can relocate the verification to the end of the run_test since it
is logically part of the test itself. With this we can drop the need for a
return value from run_test since verification becomes the last step of the
call and then immediately following is the tear down of the test setup.

Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
 1 file changed, 44 insertions(+), 70 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index 17d4299435df..d96f4084d2f5 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
@@ -10,66 +10,58 @@
 
 static __u32 duration;
 
-#define EXPECT_EQ(expected, actual, fmt)			\
-	do {							\
-		if ((expected) != (actual)) {			\
-			fprintf(stderr, "  Value of: " #actual "\n"	\
-			       "    Actual: %" fmt "\n"		\
-			       "  Expected: %" fmt "\n",	\
-			       (actual), (expected));		\
-			ret--;					\
-		}						\
-	} while (0)
-
-int verify_result(const struct tcpbpf_globals *result)
-{
-	__u32 expected_events;
-	int ret = 0;
-
-	expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
-			   (1 << BPF_SOCK_OPS_RWND_INIT) |
-			   (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
-			   (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
-			   (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
-			   (1 << BPF_SOCK_OPS_NEEDS_ECN) |
-			   (1 << BPF_SOCK_OPS_STATE_CB) |
-			   (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
-
-	EXPECT_EQ(expected_events, result->event_map, "#" PRIx32);
-	EXPECT_EQ(501ULL, result->bytes_received, "llu");
-	EXPECT_EQ(1002ULL, result->bytes_acked, "llu");
-	EXPECT_EQ(1, result->data_segs_in, PRIu32);
-	EXPECT_EQ(1, result->data_segs_out, PRIu32);
-	EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
-	EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
-	EXPECT_EQ(1, result->num_listen, PRIu32);
-
-	/* 3 comes from one listening socket + both ends of the connection */
-	EXPECT_EQ(3, result->num_close_events, PRIu32);
-
-	return ret;
-}
-
-int verify_sockopt_result(int sock_map_fd)
+static void verify_result(int map_fd, int sock_map_fd)
 {
+	__u32 expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
+				 (1 << BPF_SOCK_OPS_RWND_INIT) |
+				 (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
+				 (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
+				 (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
+				 (1 << BPF_SOCK_OPS_NEEDS_ECN) |
+				 (1 << BPF_SOCK_OPS_STATE_CB) |
+				 (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
+	struct tcpbpf_globals result = { 0 };
 	__u32 key = 0;
-	int ret = 0;
 	int res;
 	int rv;
 
+	rv = bpf_map_lookup_elem(map_fd, &key, &result);
+	if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
+		  rv, errno))
+		return;
+
+	/* check global map */
+	CHECK(expected_events != result.event_map, "event_map",
+	      "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
+	      result.event_map, expected_events);
+
+	ASSERT_EQ(result.bytes_received, 501, "bytes_received");
+	ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
+	ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
+	ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
+	ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
+	ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
+	ASSERT_EQ(result.num_listen, 1, "num_listen");
+
+	/* 3 comes from one listening socket + both ends of the connection */
+	ASSERT_EQ(result.num_close_events, 3, "num_close_events");
+
 	/* check setsockopt for SAVE_SYN */
+	key = 0;
 	rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
-	EXPECT_EQ(0, rv, "d");
-	EXPECT_EQ(0, res, "d");
-	key = 1;
+	CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
+	      rv, errno);
+	ASSERT_EQ(res, 0, "bpf_setsockopt(TCP_SAVE_SYN)");
+
 	/* check getsockopt for SAVED_SYN */
+	key = 1;
 	rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
-	EXPECT_EQ(0, rv, "d");
-	EXPECT_EQ(1, res, "d");
-	return ret;
+	CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
+	      rv, errno);
+	ASSERT_EQ(res, 1, "bpf_getsockopt(TCP_SAVED_SYN)");
 }
 
-static int run_test(void)
+static void run_test(int map_fd, int sock_map_fd)
 {
 	int listen_fd = -1, cli_fd = -1, accept_fd = -1;
 	char buf[1000];
@@ -135,18 +127,17 @@ static int run_test(void)
 	if (listen_fd != -1)
 		close(listen_fd);
 
-	return err;
+	if (!err)
+		verify_result(map_fd, sock_map_fd);
 }
 
 void test_tcpbpf_user(void)
 {
 	const char *file = "test_tcpbpf_kern.o";
 	int prog_fd, map_fd, sock_map_fd;
-	struct tcpbpf_globals g = {0};
 	int error = EXIT_FAILURE;
 	struct bpf_object *obj;
 	int cg_fd = -1;
-	__u32 key = 0;
 	int rv;
 
 	cg_fd = test__join_cgroup(CG_NAME);
@@ -173,24 +164,7 @@ void test_tcpbpf_user(void)
 	if (sock_map_fd < 0)
 		goto err;
 
-	if (run_test())
-		goto err;
-
-	rv = bpf_map_lookup_elem(map_fd, &key, &g);
-	if (rv != 0) {
-		fprintf(stderr, "FAILED: bpf_map_lookup_elem returns %d\n", rv);
-		goto err;
-	}
-
-	if (verify_result(&g)) {
-		fprintf(stderr, "FAILED: Wrong stats\n");
-		goto err;
-	}
-
-	if (verify_sockopt_result(sock_map_fd)) {
-		fprintf(stderr, "FAILED: Wrong sockopt stats\n");
-		goto err;
-	}
+	run_test(map_fd, sock_map_fd);
 
 	error = 0;
 err:

Re: [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results

[Martin KaFai Lau]

On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> From: Alexander Duyck <alexanderduyck@fb.com>
> 
> There is already logic in test_progs.h for asserting that a value is
> expected to be another value. So instead of reinventing it we should just
> make use of ASSERT_EQ in tcpbpf_user.c. This will allow for better
> debugging and integrates much more closely with the test_progs framework.
> 
> In addition we can refactor the code a bit to merge together the two
> verify functions and tie them together into a single function. Doing this
> helps to clean the code up a bit and makes it more readable as all the
> verification is now done in one function.
> 
> Lastly we can relocate the verification to the end of the run_test since it
> is logically part of the test itself. With this we can drop the need for a
> return value from run_test since verification becomes the last step of the
> call and then immediately following is the tear down of the test setup.
> 
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
Acked-by: Martin KaFai Lau <kafai@fb.com>

> ---
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
>  1 file changed, 44 insertions(+), 70 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> index 17d4299435df..d96f4084d2f5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> @@ -10,66 +10,58 @@
>  
>  static __u32 duration;
>  
> -#define EXPECT_EQ(expected, actual, fmt)			\
> -	do {							\
> -		if ((expected) != (actual)) {			\
> -			fprintf(stderr, "  Value of: " #actual "\n"	\
> -			       "    Actual: %" fmt "\n"		\
> -			       "  Expected: %" fmt "\n",	\
> -			       (actual), (expected));		\
> -			ret--;					\
> -		}						\
> -	} while (0)
> -
> -int verify_result(const struct tcpbpf_globals *result)
> -{
> -	__u32 expected_events;
> -	int ret = 0;
> -
> -	expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> -			   (1 << BPF_SOCK_OPS_RWND_INIT) |
> -			   (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> -			   (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> -			   (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> -			   (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> -			   (1 << BPF_SOCK_OPS_STATE_CB) |
> -			   (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> -
> -	EXPECT_EQ(expected_events, result->event_map, "#" PRIx32);
> -	EXPECT_EQ(501ULL, result->bytes_received, "llu");
> -	EXPECT_EQ(1002ULL, result->bytes_acked, "llu");
> -	EXPECT_EQ(1, result->data_segs_in, PRIu32);
> -	EXPECT_EQ(1, result->data_segs_out, PRIu32);
> -	EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> -	EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> -	EXPECT_EQ(1, result->num_listen, PRIu32);
> -
> -	/* 3 comes from one listening socket + both ends of the connection */
> -	EXPECT_EQ(3, result->num_close_events, PRIu32);
> -
> -	return ret;
> -}
> -
> -int verify_sockopt_result(int sock_map_fd)
> +static void verify_result(int map_fd, int sock_map_fd)
>  {
> +	__u32 expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> +				 (1 << BPF_SOCK_OPS_RWND_INIT) |
> +				 (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> +				 (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> +				 (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> +				 (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> +				 (1 << BPF_SOCK_OPS_STATE_CB) |
> +				 (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> +	struct tcpbpf_globals result = { 0 };
nit. init is not needed.

>  	__u32 key = 0;
> -	int ret = 0;
>  	int res;
>  	int rv;
>  
> +	rv = bpf_map_lookup_elem(map_fd, &key, &result);
> +	if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
> +		  rv, errno))
> +		return;
> +
> +	/* check global map */
> +	CHECK(expected_events != result.event_map, "event_map",
> +	      "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
> +	      result.event_map, expected_events);
> +
> +	ASSERT_EQ(result.bytes_received, 501, "bytes_received");
> +	ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
> +	ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
> +	ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
> +	ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
> +	ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
> +	ASSERT_EQ(result.num_listen, 1, "num_listen");
> +
> +	/* 3 comes from one listening socket + both ends of the connection */
> +	ASSERT_EQ(result.num_close_events, 3, "num_close_events");
> +
>  	/* check setsockopt for SAVE_SYN */
> +	key = 0;
nit. not needed.

>  	rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> -	EXPECT_EQ(0, rv, "d");
> -	EXPECT_EQ(0, res, "d");
> -	key = 1;
> +	CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> +	      rv, errno);
> +	ASSERT_EQ(res, 0, "bpf_setsockopt(TCP_SAVE_SYN)");
> +
>  	/* check getsockopt for SAVED_SYN */
> +	key = 1;
>  	rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> -	EXPECT_EQ(0, rv, "d");
> -	EXPECT_EQ(1, res, "d");
> -	return ret;
> +	CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> +	      rv, errno);
> +	ASSERT_EQ(res, 1, "bpf_getsockopt(TCP_SAVED_SYN)");
>  }

Re: [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results

[Alexander Duyck]

On Mon, Nov 2, 2020 at 4:42 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> > From: Alexander Duyck <alexanderduyck@fb.com>
> >
> > There is already logic in test_progs.h for asserting that a value is
> > expected to be another value. So instead of reinventing it we should just
> > make use of ASSERT_EQ in tcpbpf_user.c. This will allow for better
> > debugging and integrates much more closely with the test_progs framework.
> >
> > In addition we can refactor the code a bit to merge together the two
> > verify functions and tie them together into a single function. Doing this
> > helps to clean the code up a bit and makes it more readable as all the
> > verification is now done in one function.
> >
> > Lastly we can relocate the verification to the end of the run_test since it
> > is logically part of the test itself. With this we can drop the need for a
> > return value from run_test since verification becomes the last step of the
> > call and then immediately following is the tear down of the test setup.
> >
> > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> Acked-by: Martin KaFai Lau <kafai@fb.com>

Thanks for the review.

> > ---
> >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
> >  1 file changed, 44 insertions(+), 70 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > index 17d4299435df..d96f4084d2f5 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > @@ -10,66 +10,58 @@
> >
> >  static __u32 duration;
> >
> > -#define EXPECT_EQ(expected, actual, fmt)                     \
> > -     do {                                                    \
> > -             if ((expected) != (actual)) {                   \
> > -                     fprintf(stderr, "  Value of: " #actual "\n"     \
> > -                            "    Actual: %" fmt "\n"         \
> > -                            "  Expected: %" fmt "\n",        \
> > -                            (actual), (expected));           \
> > -                     ret--;                                  \
> > -             }                                               \
> > -     } while (0)
> > -
> > -int verify_result(const struct tcpbpf_globals *result)
> > -{
> > -     __u32 expected_events;
> > -     int ret = 0;
> > -
> > -     expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> > -                        (1 << BPF_SOCK_OPS_RWND_INIT) |
> > -                        (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> > -                        (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> > -                        (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> > -                        (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> > -                        (1 << BPF_SOCK_OPS_STATE_CB) |
> > -                        (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> > -
> > -     EXPECT_EQ(expected_events, result->event_map, "#" PRIx32);
> > -     EXPECT_EQ(501ULL, result->bytes_received, "llu");
> > -     EXPECT_EQ(1002ULL, result->bytes_acked, "llu");
> > -     EXPECT_EQ(1, result->data_segs_in, PRIu32);
> > -     EXPECT_EQ(1, result->data_segs_out, PRIu32);
> > -     EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> > -     EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> > -     EXPECT_EQ(1, result->num_listen, PRIu32);
> > -
> > -     /* 3 comes from one listening socket + both ends of the connection */
> > -     EXPECT_EQ(3, result->num_close_events, PRIu32);
> > -
> > -     return ret;
> > -}
> > -
> > -int verify_sockopt_result(int sock_map_fd)
> > +static void verify_result(int map_fd, int sock_map_fd)
> >  {
> > +     __u32 expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> > +                              (1 << BPF_SOCK_OPS_RWND_INIT) |
> > +                              (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> > +                              (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> > +                              (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> > +                              (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> > +                              (1 << BPF_SOCK_OPS_STATE_CB) |
> > +                              (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> > +     struct tcpbpf_globals result = { 0 };
> nit. init is not needed.

I had copied/pasted it from the original code that was defining this.
If a v3 is needed I can drop the initialization.

> >       __u32 key = 0;
> > -     int ret = 0;
> >       int res;
> >       int rv;
> >
> > +     rv = bpf_map_lookup_elem(map_fd, &key, &result);
> > +     if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
> > +               rv, errno))
> > +             return;
> > +
> > +     /* check global map */
> > +     CHECK(expected_events != result.event_map, "event_map",
> > +           "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
> > +           result.event_map, expected_events);
> > +
> > +     ASSERT_EQ(result.bytes_received, 501, "bytes_received");
> > +     ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
> > +     ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
> > +     ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
> > +     ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
> > +     ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
> > +     ASSERT_EQ(result.num_listen, 1, "num_listen");
> > +
> > +     /* 3 comes from one listening socket + both ends of the connection */
> > +     ASSERT_EQ(result.num_close_events, 3, "num_close_events");
> > +
> >       /* check setsockopt for SAVE_SYN */
> > +     key = 0;
> nit. not needed.

I assume you mean it is redundant since it was initialized to 0 when
we declared key in the first place?

> >       rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> > -     EXPECT_EQ(0, rv, "d");
> > -     EXPECT_EQ(0, res, "d");
> > -     key = 1;
> > +     CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> > +           rv, errno);
> > +     ASSERT_EQ(res, 0, "bpf_setsockopt(TCP_SAVE_SYN)");
> > +
> >       /* check getsockopt for SAVED_SYN */
> > +     key = 1;
> >       rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> > -     EXPECT_EQ(0, rv, "d");
> > -     EXPECT_EQ(1, res, "d");
> > -     return ret;
> > +     CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> > +           rv, errno);
> > +     ASSERT_EQ(res, 1, "bpf_getsockopt(TCP_SAVED_SYN)");
> >  }

Re: [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results

[Martin KaFai Lau]

On Mon, Nov 02, 2020 at 04:56:37PM -0800, Alexander Duyck wrote:
> On Mon, Nov 2, 2020 at 4:42 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Sat, Oct 31, 2020 at 11:52:24AM -0700, Alexander Duyck wrote:
> > > From: Alexander Duyck <alexanderduyck@fb.com>
> > >
> > > There is already logic in test_progs.h for asserting that a value is
> > > expected to be another value. So instead of reinventing it we should just
> > > make use of ASSERT_EQ in tcpbpf_user.c. This will allow for better
> > > debugging and integrates much more closely with the test_progs framework.
> > >
> > > In addition we can refactor the code a bit to merge together the two
> > > verify functions and tie them together into a single function. Doing this
> > > helps to clean the code up a bit and makes it more readable as all the
> > > verification is now done in one function.
> > >
> > > Lastly we can relocate the verification to the end of the run_test since it
> > > is logically part of the test itself. With this we can drop the need for a
> > > return value from run_test since verification becomes the last step of the
> > > call and then immediately following is the tear down of the test setup.
> > >
> > > Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> > Acked-by: Martin KaFai Lau <kafai@fb.com>
> 
> Thanks for the review.
> 
> > > ---
> > >  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
> > >  1 file changed, 44 insertions(+), 70 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > index 17d4299435df..d96f4084d2f5 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
> > > @@ -10,66 +10,58 @@
> > >
> > >  static __u32 duration;
> > >
> > > -#define EXPECT_EQ(expected, actual, fmt)                     \
> > > -     do {                                                    \
> > > -             if ((expected) != (actual)) {                   \
> > > -                     fprintf(stderr, "  Value of: " #actual "\n"     \
> > > -                            "    Actual: %" fmt "\n"         \
> > > -                            "  Expected: %" fmt "\n",        \
> > > -                            (actual), (expected));           \
> > > -                     ret--;                                  \
> > > -             }                                               \
> > > -     } while (0)
> > > -
> > > -int verify_result(const struct tcpbpf_globals *result)
> > > -{
> > > -     __u32 expected_events;
> > > -     int ret = 0;
> > > -
> > > -     expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> > > -                        (1 << BPF_SOCK_OPS_RWND_INIT) |
> > > -                        (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> > > -                        (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> > > -                        (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> > > -                        (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> > > -                        (1 << BPF_SOCK_OPS_STATE_CB) |
> > > -                        (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> > > -
> > > -     EXPECT_EQ(expected_events, result->event_map, "#" PRIx32);
> > > -     EXPECT_EQ(501ULL, result->bytes_received, "llu");
> > > -     EXPECT_EQ(1002ULL, result->bytes_acked, "llu");
> > > -     EXPECT_EQ(1, result->data_segs_in, PRIu32);
> > > -     EXPECT_EQ(1, result->data_segs_out, PRIu32);
> > > -     EXPECT_EQ(0x80, result->bad_cb_test_rv, PRIu32);
> > > -     EXPECT_EQ(0, result->good_cb_test_rv, PRIu32);
> > > -     EXPECT_EQ(1, result->num_listen, PRIu32);
> > > -
> > > -     /* 3 comes from one listening socket + both ends of the connection */
> > > -     EXPECT_EQ(3, result->num_close_events, PRIu32);
> > > -
> > > -     return ret;
> > > -}
> > > -
> > > -int verify_sockopt_result(int sock_map_fd)
> > > +static void verify_result(int map_fd, int sock_map_fd)
> > >  {
> > > +     __u32 expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
> > > +                              (1 << BPF_SOCK_OPS_RWND_INIT) |
> > > +                              (1 << BPF_SOCK_OPS_TCP_CONNECT_CB) |
> > > +                              (1 << BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB) |
> > > +                              (1 << BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB) |
> > > +                              (1 << BPF_SOCK_OPS_NEEDS_ECN) |
> > > +                              (1 << BPF_SOCK_OPS_STATE_CB) |
> > > +                              (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
> > > +     struct tcpbpf_globals result = { 0 };
> > nit. init is not needed.
> 
> I had copied/pasted it from the original code that was defining this.
> If a v3 is needed I can drop the initialization.
> 
> > >       __u32 key = 0;
> > > -     int ret = 0;
> > >       int res;
> > >       int rv;
> > >
> > > +     rv = bpf_map_lookup_elem(map_fd, &key, &result);
> > > +     if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
> > > +               rv, errno))
> > > +             return;
> > > +
> > > +     /* check global map */
> > > +     CHECK(expected_events != result.event_map, "event_map",
> > > +           "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
> > > +           result.event_map, expected_events);
> > > +
> > > +     ASSERT_EQ(result.bytes_received, 501, "bytes_received");
> > > +     ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
> > > +     ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
> > > +     ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
> > > +     ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
> > > +     ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
> > > +     ASSERT_EQ(result.num_listen, 1, "num_listen");
> > > +
> > > +     /* 3 comes from one listening socket + both ends of the connection */
> > > +     ASSERT_EQ(result.num_close_events, 3, "num_close_events");
> > > +
> > >       /* check setsockopt for SAVE_SYN */
> > > +     key = 0;
> > nit. not needed.
> 
> I assume you mean it is redundant since it was initialized to 0 when
> we declared key in the first place?
Correct.

My eariler comment in this patch can be ignored.  I just noticed that this
will go away in the last patch.

I was nit-picking a little here because people will copy-and-paste codes
from selftests.  just don't want to give a wrong impression that
those are necessary for calling bpf_map_lookup_elem().

> 
> > >       rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> > > -     EXPECT_EQ(0, rv, "d");
> > > -     EXPECT_EQ(0, res, "d");
> > > -     key = 1;
> > > +     CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> > > +           rv, errno);
> > > +     ASSERT_EQ(res, 0, "bpf_setsockopt(TCP_SAVE_SYN)");
> > > +
> > >       /* check getsockopt for SAVED_SYN */
> > > +     key = 1;
> > >       rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
> > > -     EXPECT_EQ(0, rv, "d");
> > > -     EXPECT_EQ(1, res, "d");
> > > -     return ret;
> > > +     CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
> > > +           rv, errno);
> > > +     ASSERT_EQ(res, 1, "bpf_getsockopt(TCP_SAVED_SYN)");
> > >  }

Re: [bpf-next PATCH v2 3/5] selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results

[Andrii Nakryiko]

On Sat, Oct 31, 2020 at 11:52 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexanderduyck@fb.com>
>
> There is already logic in test_progs.h for asserting that a value is
> expected to be another value. So instead of reinventing it we should just
> make use of ASSERT_EQ in tcpbpf_user.c. This will allow for better
> debugging and integrates much more closely with the test_progs framework.
>
> In addition we can refactor the code a bit to merge together the two
> verify functions and tie them together into a single function. Doing this
> helps to clean the code up a bit and makes it more readable as all the
> verification is now done in one function.
>
> Lastly we can relocate the verification to the end of the run_test since it
> is logically part of the test itself. With this we can drop the need for a
> return value from run_test since verification becomes the last step of the
> call and then immediately following is the tear down of the test setup.
>
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |  114 ++++++++------------
>  1 file changed, 44 insertions(+), 70 deletions(-)
>

[...]

> +       rv = bpf_map_lookup_elem(map_fd, &key, &result);
> +       if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
> +                 rv, errno))
> +               return;
> +
> +       /* check global map */
> +       CHECK(expected_events != result.event_map, "event_map",
> +             "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
> +             result.event_map, expected_events);

nit: libbpf and selftests don't use PRI modifiers approach. Just cast
to a consistent long, int, unsigned, whichever matches the needs and
use appropriate explicit % specifier.

> +
> +       ASSERT_EQ(result.bytes_received, 501, "bytes_received");
> +       ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
> +       ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
> +       ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
> +       ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
> +       ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
> +       ASSERT_EQ(result.num_listen, 1, "num_listen");
> +

[...]

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

[Alexander Duyck]

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(-)

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);
 }

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

[Martin KaFai Lau]

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);
>  }
> 
> 

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

[Alexander Duyck]

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.

[bpf-next PATCH v2 5/5] selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern

[Alexander Duyck]

From: Alexander Duyck <alexanderduyck@fb.com>

Use global variables instead of global_map and sockopt_results_map to track
test data. Doing this greatly simplifies the code as there is not need to
take the extra steps of updating the maps or looking up elements.

Suggested-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
---
 .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   53 ++++--------
 .../testing/selftests/bpf/progs/test_tcpbpf_kern.c |   86 +++-----------------
 tools/testing/selftests/bpf/test_tcpbpf.h          |    2 
 3 files changed, 31 insertions(+), 110 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
index c7a61b0d616a..bceca6fce09a 100644
--- a/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
+++ b/tools/testing/selftests/bpf/prog_tests/tcpbpf_user.c
@@ -11,7 +11,7 @@
 
 static __u32 duration;
 
-static void verify_result(int map_fd, int sock_map_fd)
+static void verify_result(struct tcpbpf_globals *result)
 {
 	__u32 expected_events = ((1 << BPF_SOCK_OPS_TIMEOUT_INIT) |
 				 (1 << BPF_SOCK_OPS_RWND_INIT) |
@@ -21,48 +21,31 @@ static void verify_result(int map_fd, int sock_map_fd)
 				 (1 << BPF_SOCK_OPS_NEEDS_ECN) |
 				 (1 << BPF_SOCK_OPS_STATE_CB) |
 				 (1 << BPF_SOCK_OPS_TCP_LISTEN_CB));
-	struct tcpbpf_globals result = { 0 };
-	__u32 key = 0;
-	int res;
-	int rv;
-
-	rv = bpf_map_lookup_elem(map_fd, &key, &result);
-	if (CHECK(rv, "bpf_map_lookup_elem(map_fd)", "err:%d errno:%d",
-		  rv, errno))
-		return;
 
 	/* check global map */
-	CHECK(expected_events != result.event_map, "event_map",
+	CHECK(expected_events != result->event_map, "event_map",
 	      "unexpected event_map: actual %#" PRIx32" != expected %#" PRIx32 "\n",
-	      result.event_map, expected_events);
+	      result->event_map, expected_events);
 
-	ASSERT_EQ(result.bytes_received, 501, "bytes_received");
-	ASSERT_EQ(result.bytes_acked, 1002, "bytes_acked");
-	ASSERT_EQ(result.data_segs_in, 1, "data_segs_in");
-	ASSERT_EQ(result.data_segs_out, 1, "data_segs_out");
-	ASSERT_EQ(result.bad_cb_test_rv, 0x80, "bad_cb_test_rv");
-	ASSERT_EQ(result.good_cb_test_rv, 0, "good_cb_test_rv");
-	ASSERT_EQ(result.num_listen, 1, "num_listen");
+	ASSERT_EQ(result->bytes_received, 501, "bytes_received");
+	ASSERT_EQ(result->bytes_acked, 1002, "bytes_acked");
+	ASSERT_EQ(result->data_segs_in, 1, "data_segs_in");
+	ASSERT_EQ(result->data_segs_out, 1, "data_segs_out");
+	ASSERT_EQ(result->bad_cb_test_rv, 0x80, "bad_cb_test_rv");
+	ASSERT_EQ(result->good_cb_test_rv, 0, "good_cb_test_rv");
+	ASSERT_EQ(result->num_listen, 1, "num_listen");
 
 	/* 3 comes from one listening socket + both ends of the connection */
-	ASSERT_EQ(result.num_close_events, 3, "num_close_events");
+	ASSERT_EQ(result->num_close_events, 3, "num_close_events");
 
 	/* check setsockopt for SAVE_SYN */
-	key = 0;
-	rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
-	CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
-	      rv, errno);
-	ASSERT_EQ(res, 0, "bpf_setsockopt(TCP_SAVE_SYN)");
+	ASSERT_EQ(result->tcp_save_syn, 0, "tcp_save_syn");
 
 	/* check getsockopt for SAVED_SYN */
-	key = 1;
-	rv = bpf_map_lookup_elem(sock_map_fd, &key, &res);
-	CHECK(rv, "bpf_map_lookup_elem(sock_map_fd)", "err:%d errno:%d",
-	      rv, errno);
-	ASSERT_EQ(res, 1, "bpf_getsockopt(TCP_SAVED_SYN)");
+	ASSERT_EQ(result->tcp_saved_syn, 1, "tcp_saved_syn");
 }
 
-static void run_test(int map_fd, int sock_map_fd)
+static void run_test(struct tcpbpf_globals *result)
 {
 	int listen_fd = -1, cli_fd = -1, accept_fd = -1;
 	char buf[1000];
@@ -129,13 +112,12 @@ static void run_test(int map_fd, int sock_map_fd)
 		close(listen_fd);
 
 	if (!err)
-		verify_result(map_fd, sock_map_fd);
+		verify_result(result);
 }
 
 void test_tcpbpf_user(void)
 {
 	struct test_tcpbpf_kern *skel;
-	int map_fd, sock_map_fd;
 	int cg_fd = -1;
 
 	skel = test_tcpbpf_kern__open_and_load();
@@ -147,14 +129,11 @@ void test_tcpbpf_user(void)
 		  "cg_fd:%d errno:%d", cg_fd, errno))
 		goto cleanup_skel;
 
-	map_fd = bpf_map__fd(skel->maps.global_map);
-	sock_map_fd = bpf_map__fd(skel->maps.sockopt_results);
-
 	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);
+	run_test(&skel->bss->global);
 
 cleanup_namespace:
 	if (cg_fd != -1)
diff --git a/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c b/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c
index 3e6912e4df3d..c0250142e1f6 100644
--- a/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_tcpbpf_kern.c
@@ -14,40 +14,7 @@
 #include <bpf/bpf_endian.h>
 #include "test_tcpbpf.h"
 
-struct {
-	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 4);
-	__type(key, __u32);
-	__type(value, struct tcpbpf_globals);
-} global_map SEC(".maps");
-
-struct {
-	__uint(type, BPF_MAP_TYPE_ARRAY);
-	__uint(max_entries, 2);
-	__type(key, __u32);
-	__type(value, int);
-} sockopt_results SEC(".maps");
-
-static inline void update_event_map(int event)
-{
-	__u32 key = 0;
-	struct tcpbpf_globals g, *gp;
-
-	gp = bpf_map_lookup_elem(&global_map, &key);
-	if (gp == NULL) {
-		struct tcpbpf_globals g = {0};
-
-		g.event_map |= (1 << event);
-		bpf_map_update_elem(&global_map, &key, &g,
-			    BPF_ANY);
-	} else {
-		g = *gp;
-		g.event_map |= (1 << event);
-		bpf_map_update_elem(&global_map, &key, &g,
-			    BPF_ANY);
-	}
-}
-
+struct tcpbpf_globals global = { 0 };
 int _version SEC("version") = 1;
 
 SEC("sockops")
@@ -105,29 +72,15 @@ int bpf_testcb(struct bpf_sock_ops *skops)
 
 	op = (int) skops->op;
 
-	update_event_map(op);
+	global.event_map |= (1 << op);
 
 	switch (op) {
 	case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
 		/* Test failure to set largest cb flag (assumes not defined) */
-		bad_call_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
+		global.bad_cb_test_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
 		/* Set callback */
-		good_call_rv = bpf_sock_ops_cb_flags_set(skops,
+		global.good_cb_test_rv = bpf_sock_ops_cb_flags_set(skops,
 						 BPF_SOCK_OPS_STATE_CB_FLAG);
-		/* Update results */
-		{
-			__u32 key = 0;
-			struct tcpbpf_globals g, *gp;
-
-			gp = bpf_map_lookup_elem(&global_map, &key);
-			if (!gp)
-				break;
-			g = *gp;
-			g.bad_cb_test_rv = bad_call_rv;
-			g.good_cb_test_rv = good_call_rv;
-			bpf_map_update_elem(&global_map, &key, &g,
-					    BPF_ANY);
-		}
 		break;
 	case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
 		skops->sk_txhash = 0x12345f;
@@ -143,10 +96,8 @@ int bpf_testcb(struct bpf_sock_ops *skops)
 
 				thdr = (struct tcphdr *)(header + offset);
 				v = thdr->syn;
-				__u32 key = 1;
 
-				bpf_map_update_elem(&sockopt_results, &key, &v,
-						    BPF_ANY);
+				global.tcp_saved_syn = v;
 			}
 		}
 		break;
@@ -156,25 +107,16 @@ int bpf_testcb(struct bpf_sock_ops *skops)
 		break;
 	case BPF_SOCK_OPS_STATE_CB:
 		if (skops->args[1] == BPF_TCP_CLOSE) {
-			__u32 key = 0;
-			struct tcpbpf_globals g, *gp;
-
-			gp = bpf_map_lookup_elem(&global_map, &key);
-			if (!gp)
-				break;
-			g = *gp;
 			if (skops->args[0] == BPF_TCP_LISTEN) {
-				g.num_listen++;
+				global.num_listen++;
 			} else {
-				g.total_retrans = skops->total_retrans;
-				g.data_segs_in = skops->data_segs_in;
-				g.data_segs_out = skops->data_segs_out;
-				g.bytes_received = skops->bytes_received;
-				g.bytes_acked = skops->bytes_acked;
+				global.total_retrans = skops->total_retrans;
+				global.data_segs_in = skops->data_segs_in;
+				global.data_segs_out = skops->data_segs_out;
+				global.bytes_received = skops->bytes_received;
+				global.bytes_acked = skops->bytes_acked;
 			}
-			g.num_close_events++;
-			bpf_map_update_elem(&global_map, &key, &g,
-					    BPF_ANY);
+			global.num_close_events++;
 		}
 		break;
 	case BPF_SOCK_OPS_TCP_LISTEN_CB:
@@ -182,9 +124,7 @@ int bpf_testcb(struct bpf_sock_ops *skops)
 		v = bpf_setsockopt(skops, IPPROTO_TCP, TCP_SAVE_SYN,
 				   &save_syn, sizeof(save_syn));
 		/* Update global map w/ result of setsock opt */
-		__u32 key = 0;
-
-		bpf_map_update_elem(&sockopt_results, &key, &v, BPF_ANY);
+		global.tcp_save_syn = v;
 		break;
 	default:
 		rv = -1;
diff --git a/tools/testing/selftests/bpf/test_tcpbpf.h b/tools/testing/selftests/bpf/test_tcpbpf.h
index 6220b95cbd02..0ed33521cbbb 100644
--- a/tools/testing/selftests/bpf/test_tcpbpf.h
+++ b/tools/testing/selftests/bpf/test_tcpbpf.h
@@ -14,5 +14,7 @@ struct tcpbpf_globals {
 	__u64 bytes_acked;
 	__u32 num_listen;
 	__u32 num_close_events;
+	__u32 tcp_save_syn;
+	__u32 tcp_saved_syn;
 };
 #endif

Re: [bpf-next PATCH v2 5/5] selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern

[Martin KaFai Lau]

On Sat, Oct 31, 2020 at 11:52:37AM -0700, Alexander Duyck wrote:
[ ... ]

> +struct tcpbpf_globals global = { 0 };
>  int _version SEC("version") = 1;
>  
>  SEC("sockops")
> @@ -105,29 +72,15 @@ int bpf_testcb(struct bpf_sock_ops *skops)
>  
>  	op = (int) skops->op;
>  
> -	update_event_map(op);
> +	global.event_map |= (1 << op);
>  
>  	switch (op) {
>  	case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
>  		/* Test failure to set largest cb flag (assumes not defined) */
> -		bad_call_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
> +		global.bad_cb_test_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
>  		/* Set callback */
> -		good_call_rv = bpf_sock_ops_cb_flags_set(skops,
> +		global.good_cb_test_rv = bpf_sock_ops_cb_flags_set(skops,
>  						 BPF_SOCK_OPS_STATE_CB_FLAG);
> -		/* Update results */
> -		{
> -			__u32 key = 0;
> -			struct tcpbpf_globals g, *gp;
> -
> -			gp = bpf_map_lookup_elem(&global_map, &key);
> -			if (!gp)
> -				break;
> -			g = *gp;
> -			g.bad_cb_test_rv = bad_call_rv;
> -			g.good_cb_test_rv = good_call_rv;
> -			bpf_map_update_elem(&global_map, &key, &g,
> -					    BPF_ANY);
> -		}
>  		break;
>  	case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
>  		skops->sk_txhash = 0x12345f;
> @@ -143,10 +96,8 @@ int bpf_testcb(struct bpf_sock_ops *skops)
>  
>  				thdr = (struct tcphdr *)(header + offset);
>  				v = thdr->syn;
> -				__u32 key = 1;
>  
> -				bpf_map_update_elem(&sockopt_results, &key, &v,
> -						    BPF_ANY);
> +				global.tcp_saved_syn = v;
>  			}
>  		}
>  		break;
> @@ -156,25 +107,16 @@ int bpf_testcb(struct bpf_sock_ops *skops)
>  		break;
>  	case BPF_SOCK_OPS_STATE_CB:
>  		if (skops->args[1] == BPF_TCP_CLOSE) {
> -			__u32 key = 0;
> -			struct tcpbpf_globals g, *gp;
> -
> -			gp = bpf_map_lookup_elem(&global_map, &key);
> -			if (!gp)
> -				break;
> -			g = *gp;
>  			if (skops->args[0] == BPF_TCP_LISTEN) {
> -				g.num_listen++;
> +				global.num_listen++;
>  			} else {
> -				g.total_retrans = skops->total_retrans;
> -				g.data_segs_in = skops->data_segs_in;
> -				g.data_segs_out = skops->data_segs_out;
> -				g.bytes_received = skops->bytes_received;
> -				g.bytes_acked = skops->bytes_acked;
> +				global.total_retrans = skops->total_retrans;
> +				global.data_segs_in = skops->data_segs_in;
> +				global.data_segs_out = skops->data_segs_out;
> +				global.bytes_received = skops->bytes_received;
> +				global.bytes_acked = skops->bytes_acked;
>  			}
> -			g.num_close_events++;
> -			bpf_map_update_elem(&global_map, &key, &g,
> -					    BPF_ANY);
It is interesting that there is no race in the original "g.num_close_events++"
followed by the bpf_map_update_elem().  It seems quite fragile though.

> +			global.num_close_events++;
There is __sync_fetch_and_add().

not sure about the global.event_map though, may be use an individual
variable for each _CB.  Thoughts?

>  		}
>  		break;
>  	case BPF_SOCK_OPS_TCP_LISTEN_CB:
> @@ -182,9 +124,7 @@ int bpf_testcb(struct bpf_sock_ops *skops)
>  		v = bpf_setsockopt(skops, IPPROTO_TCP, TCP_SAVE_SYN,
>  				   &save_syn, sizeof(save_syn));
>  		/* Update global map w/ result of setsock opt */
> -		__u32 key = 0;
> -
> -		bpf_map_update_elem(&sockopt_results, &key, &v, BPF_ANY);
> +		global.tcp_save_syn = v;
>  		break;
>  	default:
>  		rv = -1;
> diff --git a/tools/testing/selftests/bpf/test_tcpbpf.h b/tools/testing/selftests/bpf/test_tcpbpf.h
> index 6220b95cbd02..0ed33521cbbb 100644
> --- a/tools/testing/selftests/bpf/test_tcpbpf.h
> +++ b/tools/testing/selftests/bpf/test_tcpbpf.h
> @@ -14,5 +14,7 @@ struct tcpbpf_globals {
>  	__u64 bytes_acked;
>  	__u32 num_listen;
>  	__u32 num_close_events;
> +	__u32 tcp_save_syn;
> +	__u32 tcp_saved_syn;
>  };
>  #endif
> 
> 

Re: [bpf-next PATCH v2 5/5] selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern

[Alexander Duyck]

On Mon, Nov 2, 2020 at 5:26 PM Martin KaFai Lau <kafai@fb.com> wrote:
>
> On Sat, Oct 31, 2020 at 11:52:37AM -0700, Alexander Duyck wrote:
> [ ... ]
>
> > +struct tcpbpf_globals global = { 0 };
> >  int _version SEC("version") = 1;
> >
> >  SEC("sockops")
> > @@ -105,29 +72,15 @@ int bpf_testcb(struct bpf_sock_ops *skops)
> >
> >       op = (int) skops->op;
> >
> > -     update_event_map(op);
> > +     global.event_map |= (1 << op);
> >
> >       switch (op) {
> >       case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
> >               /* Test failure to set largest cb flag (assumes not defined) */
> > -             bad_call_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
> > +             global.bad_cb_test_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
> >               /* Set callback */
> > -             good_call_rv = bpf_sock_ops_cb_flags_set(skops,
> > +             global.good_cb_test_rv = bpf_sock_ops_cb_flags_set(skops,
> >                                                BPF_SOCK_OPS_STATE_CB_FLAG);
> > -             /* Update results */
> > -             {
> > -                     __u32 key = 0;
> > -                     struct tcpbpf_globals g, *gp;
> > -
> > -                     gp = bpf_map_lookup_elem(&global_map, &key);
> > -                     if (!gp)
> > -                             break;
> > -                     g = *gp;
> > -                     g.bad_cb_test_rv = bad_call_rv;
> > -                     g.good_cb_test_rv = good_call_rv;
> > -                     bpf_map_update_elem(&global_map, &key, &g,
> > -                                         BPF_ANY);
> > -             }
> >               break;
> >       case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
> >               skops->sk_txhash = 0x12345f;
> > @@ -143,10 +96,8 @@ int bpf_testcb(struct bpf_sock_ops *skops)
> >
> >                               thdr = (struct tcphdr *)(header + offset);
> >                               v = thdr->syn;
> > -                             __u32 key = 1;
> >
> > -                             bpf_map_update_elem(&sockopt_results, &key, &v,
> > -                                                 BPF_ANY);
> > +                             global.tcp_saved_syn = v;
> >                       }
> >               }
> >               break;
> > @@ -156,25 +107,16 @@ int bpf_testcb(struct bpf_sock_ops *skops)
> >               break;
> >       case BPF_SOCK_OPS_STATE_CB:
> >               if (skops->args[1] == BPF_TCP_CLOSE) {
> > -                     __u32 key = 0;
> > -                     struct tcpbpf_globals g, *gp;
> > -
> > -                     gp = bpf_map_lookup_elem(&global_map, &key);
> > -                     if (!gp)
> > -                             break;
> > -                     g = *gp;
> >                       if (skops->args[0] == BPF_TCP_LISTEN) {
> > -                             g.num_listen++;
> > +                             global.num_listen++;
> >                       } else {
> > -                             g.total_retrans = skops->total_retrans;
> > -                             g.data_segs_in = skops->data_segs_in;
> > -                             g.data_segs_out = skops->data_segs_out;
> > -                             g.bytes_received = skops->bytes_received;
> > -                             g.bytes_acked = skops->bytes_acked;
> > +                             global.total_retrans = skops->total_retrans;
> > +                             global.data_segs_in = skops->data_segs_in;
> > +                             global.data_segs_out = skops->data_segs_out;
> > +                             global.bytes_received = skops->bytes_received;
> > +                             global.bytes_acked = skops->bytes_acked;
> >                       }
> > -                     g.num_close_events++;
> > -                     bpf_map_update_elem(&global_map, &key, &g,
> > -                                         BPF_ANY);
> It is interesting that there is no race in the original "g.num_close_events++"
> followed by the bpf_map_update_elem().  It seems quite fragile though.

How would it race with the current code though? At this point we are
controlling the sockets in a single thread. As such the close events
should already be serialized shouldn't they? This may have been a
problem with the old code, but even then it was only two sockets so I
don't think there was much risk of them racing against each other
since the two sockets were linked anyway.

> > +                     global.num_close_events++;
> There is __sync_fetch_and_add().
>
> not sure about the global.event_map though, may be use an individual
> variable for each _CB.  Thoughts?

I think this may be overkill for what we actually need. Since we are
closing the sockets in a single threaded application there isn't much
risk of the sockets all racing against each other in the close is
there?

Re: [bpf-next PATCH v2 5/5] selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern

[Martin KaFai Lau]

On Tue, Nov 03, 2020 at 07:42:46AM -0800, Alexander Duyck wrote:
> On Mon, Nov 2, 2020 at 5:26 PM Martin KaFai Lau <kafai@fb.com> wrote:
> >
> > On Sat, Oct 31, 2020 at 11:52:37AM -0700, Alexander Duyck wrote:
> > [ ... ]
> >
> > > +struct tcpbpf_globals global = { 0 };
> > >  int _version SEC("version") = 1;
> > >
> > >  SEC("sockops")
> > > @@ -105,29 +72,15 @@ int bpf_testcb(struct bpf_sock_ops *skops)
> > >
> > >       op = (int) skops->op;
> > >
> > > -     update_event_map(op);
> > > +     global.event_map |= (1 << op);
> > >
> > >       switch (op) {
> > >       case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
> > >               /* Test failure to set largest cb flag (assumes not defined) */
> > > -             bad_call_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
> > > +             global.bad_cb_test_rv = bpf_sock_ops_cb_flags_set(skops, 0x80);
> > >               /* Set callback */
> > > -             good_call_rv = bpf_sock_ops_cb_flags_set(skops,
> > > +             global.good_cb_test_rv = bpf_sock_ops_cb_flags_set(skops,
> > >                                                BPF_SOCK_OPS_STATE_CB_FLAG);
> > > -             /* Update results */
> > > -             {
> > > -                     __u32 key = 0;
> > > -                     struct tcpbpf_globals g, *gp;
> > > -
> > > -                     gp = bpf_map_lookup_elem(&global_map, &key);
> > > -                     if (!gp)
> > > -                             break;
> > > -                     g = *gp;
> > > -                     g.bad_cb_test_rv = bad_call_rv;
> > > -                     g.good_cb_test_rv = good_call_rv;
> > > -                     bpf_map_update_elem(&global_map, &key, &g,
> > > -                                         BPF_ANY);
> > > -             }
> > >               break;
> > >       case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
> > >               skops->sk_txhash = 0x12345f;
> > > @@ -143,10 +96,8 @@ int bpf_testcb(struct bpf_sock_ops *skops)
> > >
> > >                               thdr = (struct tcphdr *)(header + offset);
> > >                               v = thdr->syn;
> > > -                             __u32 key = 1;
> > >
> > > -                             bpf_map_update_elem(&sockopt_results, &key, &v,
> > > -                                                 BPF_ANY);
> > > +                             global.tcp_saved_syn = v;
> > >                       }
> > >               }
> > >               break;
> > > @@ -156,25 +107,16 @@ int bpf_testcb(struct bpf_sock_ops *skops)
> > >               break;
> > >       case BPF_SOCK_OPS_STATE_CB:
> > >               if (skops->args[1] == BPF_TCP_CLOSE) {
> > > -                     __u32 key = 0;
> > > -                     struct tcpbpf_globals g, *gp;
> > > -
> > > -                     gp = bpf_map_lookup_elem(&global_map, &key);
> > > -                     if (!gp)
> > > -                             break;
> > > -                     g = *gp;
> > >                       if (skops->args[0] == BPF_TCP_LISTEN) {
> > > -                             g.num_listen++;
> > > +                             global.num_listen++;
> > >                       } else {
> > > -                             g.total_retrans = skops->total_retrans;
> > > -                             g.data_segs_in = skops->data_segs_in;
> > > -                             g.data_segs_out = skops->data_segs_out;
> > > -                             g.bytes_received = skops->bytes_received;
> > > -                             g.bytes_acked = skops->bytes_acked;
> > > +                             global.total_retrans = skops->total_retrans;
> > > +                             global.data_segs_in = skops->data_segs_in;
> > > +                             global.data_segs_out = skops->data_segs_out;
> > > +                             global.bytes_received = skops->bytes_received;
> > > +                             global.bytes_acked = skops->bytes_acked;
> > >                       }
> > > -                     g.num_close_events++;
> > > -                     bpf_map_update_elem(&global_map, &key, &g,
> > > -                                         BPF_ANY);
> > It is interesting that there is no race in the original "g.num_close_events++"
> > followed by the bpf_map_update_elem().  It seems quite fragile though.
> 
> How would it race with the current code though? At this point we are
> controlling the sockets in a single thread. As such the close events
> should already be serialized shouldn't they? This may have been a
> problem with the old code, but even then it was only two sockets so I
> don't think there was much risk of them racing against each other
> since the two sockets were linked anyway.
> 
> > > +                     global.num_close_events++;
> > There is __sync_fetch_and_add().
> >
> > not sure about the global.event_map though, may be use an individual
> > variable for each _CB.  Thoughts?
> 
> I think this may be overkill for what we actually need. Since we are
> closing the sockets in a single threaded application there isn't much
> risk of the sockets all racing against each other in the close is
> there?
Make sense.

Acked-by: Martin KaFai Lau <kafai@fb.com>

Re: [bpf-next PATCH v2 5/5] selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern

[Andrii Nakryiko]

On Sat, Oct 31, 2020 at 11:52 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexanderduyck@fb.com>
>
> Use global variables instead of global_map and sockopt_results_map to track
> test data. Doing this greatly simplifies the code as there is not need to
> take the extra steps of updating the maps or looking up elements.
>
> Suggested-by: Martin KaFai Lau <kafai@fb.com>
> Signed-off-by: Alexander Duyck <alexanderduyck@fb.com>
> ---
>  .../testing/selftests/bpf/prog_tests/tcpbpf_user.c |   53 ++++--------
>  .../testing/selftests/bpf/progs/test_tcpbpf_kern.c |   86 +++-----------------
>  tools/testing/selftests/bpf/test_tcpbpf.h          |    2
>  3 files changed, 31 insertions(+), 110 deletions(-)

[...]

> -}
> -
> +struct tcpbpf_globals global = { 0 };

nit: = {0} notation is misleading, just = {}; is equivalent and IMO better.

But don't bother if it's the only change you need to do for the next version.

>  int _version SEC("version") = 1;
>

[...]

Re: [bpf-next PATCH v2 0/5] selftests/bpf: Migrate test_tcpbpf_user to be a part of test_progs framework

[Alexander Duyck]

On Sat, Oct 31, 2020 at 11:31 AM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> Move the test functionality from test_tcpbpf_user into the test_progs
> framework so that it will be run any time the test_progs framework is run.
> This will help to prevent future test escapes as the individual tests, such
> as test_tcpbpf_user, are less likely to be run by developers and CI
> tests.
>
> As a part of moving it over the series goes through and updates the code to
> make use of the existing APIs included in the test_progs framework. This is
> meant to simplify and streamline the test code and avoid duplication of
> effort.
>
> v2: Dropped test_tcpbpf_user from .gitignore
>     Replaced CHECK_FAIL calls with CHECK calls
>     Minimized changes in patch 1 when moving the file
>     Updated stg mail command line to display renames in submission
>     Added shutdown logic to end of run_test function to guarantee close
>     Added patch that replaces the two maps with use of global variables
>
> ---
>
> Alexander Duyck (5):
>       selftests/bpf: Move test_tcppbf_user into test_progs
>       selftests/bpf: Drop python client/server in favor of threads
>       selftests/bpf: Replace EXPECT_EQ with ASSERT_EQ and refactor verify_results
>       selftests/bpf: Migrate tcpbpf_user.c to use BPF skeleton
>       selftest/bpf: Use global variables instead of maps for test_tcpbpf_kern
>
>
>  .../selftests/bpf/prog_tests/tcpbpf_user.c    | 239 +++++++++---------
>  .../selftests/bpf/progs/test_tcpbpf_kern.c    |  86 +------
>  tools/testing/selftests/bpf/tcp_client.py     |  50 ----
>  tools/testing/selftests/bpf/tcp_server.py     |  80 ------
>  tools/testing/selftests/bpf/test_tcpbpf.h     |   2 +
>  5 files changed, 135 insertions(+), 322 deletions(-)
>  delete mode 100755 tools/testing/selftests/bpf/tcp_client.py
>  delete mode 100755 tools/testing/selftests/bpf/tcp_server.py
>
> --

It looks like the "--to" on the cover page was dropped and it wasn't
delivered to the bpf mailing list. So I am just going to reply to the
one that was delivered to netdev so that it is visible for the people
on the bpf list.

Thanks.

- Alex