[PATCH bpf-next] selftests/bpf: move netcnt test under test_progs

[PATCH bpf-next] selftests/bpf: move netcnt test under test_progs

[Stanislav Fomichev]

Rewrite to skel and ASSERT macros as well while we are at it.

Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
 tools/testing/selftests/bpf/Makefile          |   3 +-
 .../testing/selftests/bpf/prog_tests/netcnt.c |  93 +++++++++++
 tools/testing/selftests/bpf/test_netcnt.c     | 148 ------------------
 3 files changed, 94 insertions(+), 150 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/netcnt.c
 delete mode 100644 tools/testing/selftests/bpf/test_netcnt.c

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index f405b20c1e6c..2a58b7b5aea4 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -38,7 +38,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
 	test_verifier_log test_dev_cgroup \
 	test_sock test_sockmap get_cgroup_id_user \
 	test_cgroup_storage \
-	test_netcnt test_tcpnotify_user test_sysctl \
+	test_tcpnotify_user test_sysctl \
 	test_progs-no_alu32
 
 # Also test bpf-gcc, if present
@@ -197,7 +197,6 @@ $(OUTPUT)/test_sockmap: 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
-$(OUTPUT)/test_netcnt: cgroup_helpers.c
 $(OUTPUT)/test_sock_fields: cgroup_helpers.c
 $(OUTPUT)/test_sysctl: cgroup_helpers.c
 
diff --git a/tools/testing/selftests/bpf/prog_tests/netcnt.c b/tools/testing/selftests/bpf/prog_tests/netcnt.c
new file mode 100644
index 000000000000..063a40d228b6
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/netcnt.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <sys/sysinfo.h>
+#include <test_progs.h>
+#include "netcnt_prog.skel.h"
+#include "netcnt_common.h"
+
+#define CG_NAME "/netcnt"
+
+void test_netcnt(void)
+{
+	union percpu_net_cnt *percpu_netcnt = NULL;
+	struct bpf_cgroup_storage_key key;
+	int map_fd, percpu_map_fd;
+	struct netcnt_prog *skel;
+	unsigned long packets;
+	union net_cnt netcnt;
+	unsigned long bytes;
+	int cpu, nproc;
+	int cg_fd = -1;
+
+	skel = netcnt_prog__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "netcnt_prog__open_and_load"))
+		return;
+
+	nproc = get_nprocs_conf();
+	percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
+	if (!ASSERT_OK_PTR(percpu_netcnt, "malloc(percpu_netcnt)"))
+		goto err;
+
+	cg_fd = test__join_cgroup(CG_NAME);
+	if (!ASSERT_GE(cg_fd, 0, "test__join_cgroup"))
+		goto err;
+
+	skel->links.bpf_nextcnt =
+		bpf_program__attach_cgroup(skel->progs.bpf_nextcnt, cg_fd);
+	if (!ASSERT_OK_PTR(skel->links.bpf_nextcnt,
+			   "attach_cgroup(bpf_nextcnt)"))
+		goto err;
+
+	if (system("which ping6 &>/dev/null") == 0)
+		assert(!system("ping6 ::1 -c 10000 -f -q > /dev/null"));
+	else
+		assert(!system("ping -6 ::1 -c 10000 -f -q > /dev/null"));
+
+	map_fd = bpf_map__fd(skel->maps.netcnt);
+	if (!ASSERT_GE(map_fd, 0, "bpf_map__fd(netcnt)"))
+		goto err;
+
+	percpu_map_fd = bpf_map__fd(skel->maps.percpu_netcnt);
+	if (!ASSERT_GE(percpu_map_fd, 0, "bpf_map__fd(percpu_netcnt)"))
+		goto err;
+
+	if (!ASSERT_OK(bpf_map_get_next_key(map_fd, NULL, &key),
+		       "bpf_map_get_next_key"))
+		goto err;
+
+	if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &netcnt),
+		       "bpf_map_lookup_elem(netcnt)"))
+		goto err;
+
+	if (!ASSERT_OK(bpf_map_lookup_elem(percpu_map_fd, &key,
+					   &percpu_netcnt[0]),
+		       "bpf_map_lookup_elem(percpu_netcnt)"))
+		goto err;
+
+	/* Some packets can be still in per-cpu cache, but not more than
+	 * MAX_PERCPU_PACKETS.
+	 */
+	packets = netcnt.packets;
+	bytes = netcnt.bytes;
+	for (cpu = 0; cpu < nproc; cpu++) {
+		ASSERT_LE(percpu_netcnt[cpu].packets, MAX_PERCPU_PACKETS,
+			  "MAX_PERCPU_PACKETS");
+
+		packets += percpu_netcnt[cpu].packets;
+		bytes += percpu_netcnt[cpu].bytes;
+	}
+
+	/* No packets should be lost */
+	ASSERT_EQ(packets, 10000, "packets");
+
+	/* Let's check that bytes counter matches the number of packets
+	 * multiplied by the size of ipv6 ICMP packet.
+	 */
+	ASSERT_EQ(bytes, packets * 104, "bytes");
+
+err:
+	if (cg_fd != -1)
+		close(cg_fd);
+	free(percpu_netcnt);
+	netcnt_prog__destroy(skel);
+}
diff --git a/tools/testing/selftests/bpf/test_netcnt.c b/tools/testing/selftests/bpf/test_netcnt.c
deleted file mode 100644
index 4990a99e7381..000000000000
--- a/tools/testing/selftests/bpf/test_netcnt.c
+++ /dev/null
@@ -1,148 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <errno.h>
-#include <assert.h>
-#include <sys/sysinfo.h>
-#include <sys/time.h>
-
-#include <linux/bpf.h>
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "cgroup_helpers.h"
-#include "bpf_rlimit.h"
-#include "netcnt_common.h"
-
-#define BPF_PROG "./netcnt_prog.o"
-#define TEST_CGROUP "/test-network-counters/"
-
-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)
-{
-	union percpu_net_cnt *percpu_netcnt;
-	struct bpf_cgroup_storage_key key;
-	int map_fd, percpu_map_fd;
-	int error = EXIT_FAILURE;
-	struct bpf_object *obj;
-	int prog_fd, cgroup_fd;
-	unsigned long packets;
-	union net_cnt netcnt;
-	unsigned long bytes;
-	int cpu, nproc;
-	__u32 prog_cnt;
-
-	nproc = get_nprocs_conf();
-	percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
-	if (!percpu_netcnt) {
-		printf("Not enough memory for per-cpu area (%d cpus)\n", nproc);
-		goto err;
-	}
-
-	if (bpf_prog_load(BPF_PROG, BPF_PROG_TYPE_CGROUP_SKB,
-			  &obj, &prog_fd)) {
-		printf("Failed to load bpf program\n");
-		goto out;
-	}
-
-	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
-	if (cgroup_fd < 0)
-		goto err;
-
-	/* Attach bpf program */
-	if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_INET_EGRESS, 0)) {
-		printf("Failed to attach bpf program");
-		goto err;
-	}
-
-	if (system("which ping6 &>/dev/null") == 0)
-		assert(!system("ping6 ::1 -c 10000 -f -q > /dev/null"));
-	else
-		assert(!system("ping -6 ::1 -c 10000 -f -q > /dev/null"));
-
-	if (bpf_prog_query(cgroup_fd, BPF_CGROUP_INET_EGRESS, 0, NULL, NULL,
-			   &prog_cnt)) {
-		printf("Failed to query attached programs");
-		goto err;
-	}
-
-	map_fd = bpf_find_map(__func__, obj, "netcnt");
-	if (map_fd < 0) {
-		printf("Failed to find bpf map with net counters");
-		goto err;
-	}
-
-	percpu_map_fd = bpf_find_map(__func__, obj, "percpu_netcnt");
-	if (percpu_map_fd < 0) {
-		printf("Failed to find bpf map with percpu net counters");
-		goto err;
-	}
-
-	if (bpf_map_get_next_key(map_fd, NULL, &key)) {
-		printf("Failed to get key in cgroup storage\n");
-		goto err;
-	}
-
-	if (bpf_map_lookup_elem(map_fd, &key, &netcnt)) {
-		printf("Failed to lookup cgroup storage\n");
-		goto err;
-	}
-
-	if (bpf_map_lookup_elem(percpu_map_fd, &key, &percpu_netcnt[0])) {
-		printf("Failed to lookup percpu cgroup storage\n");
-		goto err;
-	}
-
-	/* Some packets can be still in per-cpu cache, but not more than
-	 * MAX_PERCPU_PACKETS.
-	 */
-	packets = netcnt.packets;
-	bytes = netcnt.bytes;
-	for (cpu = 0; cpu < nproc; cpu++) {
-		if (percpu_netcnt[cpu].packets > MAX_PERCPU_PACKETS) {
-			printf("Unexpected percpu value: %llu\n",
-			       percpu_netcnt[cpu].packets);
-			goto err;
-		}
-
-		packets += percpu_netcnt[cpu].packets;
-		bytes += percpu_netcnt[cpu].bytes;
-	}
-
-	/* No packets should be lost */
-	if (packets != 10000) {
-		printf("Unexpected packet count: %lu\n", packets);
-		goto err;
-	}
-
-	/* Let's check that bytes counter matches the number of packets
-	 * multiplied by the size of ipv6 ICMP packet.
-	 */
-	if (bytes != packets * 104) {
-		printf("Unexpected bytes count: %lu\n", bytes);
-		goto err;
-	}
-
-	error = 0;
-	printf("test_netcnt:PASS\n");
-
-err:
-	cleanup_cgroup_environment();
-	free(percpu_netcnt);
-
-out:
-	return error;
-}
-- 
2.32.0.554.ge1b32706d8-goog

Re: [PATCH bpf-next] selftests/bpf: move netcnt test under test_progs

[Yonghong Song]

On 7/28/21 8:14 AM, Stanislav Fomichev wrote:
> Rewrite to skel and ASSERT macros as well while we are at it.
> 
> Signed-off-by: Stanislav Fomichev <sdf@google.com>

Thanks for converting test_netcnt to test_progs.
The patch looks good to me except a couple of minor issues.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   tools/testing/selftests/bpf/Makefile          |   3 +-
>   .../testing/selftests/bpf/prog_tests/netcnt.c |  93 +++++++++++
>   tools/testing/selftests/bpf/test_netcnt.c     | 148 ------------------
>   3 files changed, 94 insertions(+), 150 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/prog_tests/netcnt.c
>   delete mode 100644 tools/testing/selftests/bpf/test_netcnt.c
> 
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index f405b20c1e6c..2a58b7b5aea4 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -38,7 +38,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
>   	test_verifier_log test_dev_cgroup \
>   	test_sock test_sockmap get_cgroup_id_user \
>   	test_cgroup_storage \
> -	test_netcnt test_tcpnotify_user test_sysctl \
> +	test_tcpnotify_user test_sysctl \
>   	test_progs-no_alu32
>   
>   # Also test bpf-gcc, if present
> @@ -197,7 +197,6 @@ $(OUTPUT)/test_sockmap: 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
> -$(OUTPUT)/test_netcnt: cgroup_helpers.c
>   $(OUTPUT)/test_sock_fields: cgroup_helpers.c
>   $(OUTPUT)/test_sysctl: cgroup_helpers.c
>   
> diff --git a/tools/testing/selftests/bpf/prog_tests/netcnt.c b/tools/testing/selftests/bpf/prog_tests/netcnt.c
> new file mode 100644
> index 000000000000..063a40d228b6
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/netcnt.c
> @@ -0,0 +1,93 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <sys/sysinfo.h>
> +#include <test_progs.h>
> +#include "netcnt_prog.skel.h"
> +#include "netcnt_common.h"
> +
> +#define CG_NAME "/netcnt"
> +
> +void test_netcnt(void)
> +{
> +	union percpu_net_cnt *percpu_netcnt = NULL;
> +	struct bpf_cgroup_storage_key key;
> +	int map_fd, percpu_map_fd;
> +	struct netcnt_prog *skel;
> +	unsigned long packets;
> +	union net_cnt netcnt;
> +	unsigned long bytes;
> +	int cpu, nproc;
> +	int cg_fd = -1;
> +
> +	skel = netcnt_prog__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "netcnt_prog__open_and_load"))
> +		return;
> +
> +	nproc = get_nprocs_conf();
> +	percpu_netcnt = malloc(sizeof(*percpu_netcnt) * nproc);
> +	if (!ASSERT_OK_PTR(percpu_netcnt, "malloc(percpu_netcnt)"))
> +		goto err;
> +
> +	cg_fd = test__join_cgroup(CG_NAME);
> +	if (!ASSERT_GE(cg_fd, 0, "test__join_cgroup"))
> +		goto err;
> +
> +	skel->links.bpf_nextcnt =
> +		bpf_program__attach_cgroup(skel->progs.bpf_nextcnt, cg_fd);
> +	if (!ASSERT_OK_PTR(skel->links.bpf_nextcnt,
> +			   "attach_cgroup(bpf_nextcnt)"))
> +		goto err;
> +
> +	if (system("which ping6 &>/dev/null") == 0)
> +		assert(!system("ping6 ::1 -c 10000 -f -q > /dev/null"));
> +	else
> +		assert(!system("ping -6 ::1 -c 10000 -f -q > /dev/null"));
> +
> +	map_fd = bpf_map__fd(skel->maps.netcnt);
> +	if (!ASSERT_GE(map_fd, 0, "bpf_map__fd(netcnt)"))
> +		goto err;

For skeleton, map_fd is always valid and you do not need to check it.

> +
> +	percpu_map_fd = bpf_map__fd(skel->maps.percpu_netcnt);
> +	if (!ASSERT_GE(percpu_map_fd, 0, "bpf_map__fd(percpu_netcnt)"))
> +		goto err;

The same for percpu_map_fd, it is always valid and no need to check it.

> +
> +	if (!ASSERT_OK(bpf_map_get_next_key(map_fd, NULL, &key),
> +		       "bpf_map_get_next_key"))
> +		goto err;
> +
> +	if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &netcnt),
> +		       "bpf_map_lookup_elem(netcnt)"))
> +		goto err;
> +
[...]

Re: [PATCH bpf-next] selftests/bpf: move netcnt test under test_progs

[Andrii Nakryiko]

On Wed, Jul 28, 2021 at 8:14 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Rewrite to skel and ASSERT macros as well while we are at it.
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---

Thanks! Bonus points for skeleton and ASSERT_XXX! ;)

In addition to Yonghong's comments, a few more below. Missed assert()s
require a new revision, unfortunately.

>  tools/testing/selftests/bpf/Makefile          |   3 +-
>  .../testing/selftests/bpf/prog_tests/netcnt.c |  93 +++++++++++
>  tools/testing/selftests/bpf/test_netcnt.c     | 148 ------------------

Usually there is .gitignore clean up as well.

>  3 files changed, 94 insertions(+), 150 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/prog_tests/netcnt.c
>  delete mode 100644 tools/testing/selftests/bpf/test_netcnt.c
>

[...]

> +
> +       skel->links.bpf_nextcnt =
> +               bpf_program__attach_cgroup(skel->progs.bpf_nextcnt, cg_fd);
> +       if (!ASSERT_OK_PTR(skel->links.bpf_nextcnt,
> +                          "attach_cgroup(bpf_nextcnt)"))
> +               goto err;
> +
> +       if (system("which ping6 &>/dev/null") == 0)

see 4cda0c82a34b ("selftests/bpf: Use ping6 only if available in
tc_redirect"), we should probably add some system() + ping -6/ping6
wrapper into network_helpers.c and use that in at least all
test_progs' tests.

> +               assert(!system("ping6 ::1 -c 10000 -f -q > /dev/null"));
> +       else
> +               assert(!system("ping -6 ::1 -c 10000 -f -q > /dev/null"));

no assert() please

> +
> +       map_fd = bpf_map__fd(skel->maps.netcnt);
> +       if (!ASSERT_GE(map_fd, 0, "bpf_map__fd(netcnt)"))
> +               goto err;
> +
> +       percpu_map_fd = bpf_map__fd(skel->maps.percpu_netcnt);
> +       if (!ASSERT_GE(percpu_map_fd, 0, "bpf_map__fd(percpu_netcnt)"))
> +               goto err;
> +
> +       if (!ASSERT_OK(bpf_map_get_next_key(map_fd, NULL, &key),
> +                      "bpf_map_get_next_key"))

it's ok to use all 100 characters if that helps keeps simple function
invocations on the single line, so don't hesitate to do that

> +               goto err;
> +
> +       if (!ASSERT_OK(bpf_map_lookup_elem(map_fd, &key, &netcnt),
> +                      "bpf_map_lookup_elem(netcnt)"))
> +               goto err;
> +
> +       if (!ASSERT_OK(bpf_map_lookup_elem(percpu_map_fd, &key,
> +                                          &percpu_netcnt[0]),
> +                      "bpf_map_lookup_elem(percpu_netcnt)"))
> +               goto err;
> +

[...]