diff mbox series

[bpf-next,1/4] selftests/bpf: convert get_current_cgroup_id_user to test_progs

Message ID 20240731-convert_cgroup_tests-v1-1-14cbc51b6947@bootlin.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series selftests/bpf: convert three other cgroup tests to test_progs | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-27 fail Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-PR fail PR summary
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 42 this patch: 42
netdev/build_tools success Errors and warnings before: 10 this patch: 10
netdev/cc_maintainers success CCed 17 of 17 maintainers
netdev/build_clang success Errors and warnings before: 44 this patch: 44
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 49 this patch: 49
netdev/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Alexis Lothoré (eBPF Foundation) July 31, 2024, 10:38 a.m. UTC 
get_current_cgroup_id_user allows testing for bpf_get_current_cgroup_id()
bpf API but is not integrated into test_progs, and so is not tested
automatically in CI.

Convert it to the test_progs framework to allow running it automatically.
The most notable differences with the old test are the following:
- the new test relies on autoattach instead of manually hooking/enabling
  the targeted tracepoint through perf_event, which reduces quite a lot the
  test code size
- sleep duration passed to nanosleep syscall has been reduced to its
  minimum to not impact overall CI duration (we only care about the syscall
  being properly triggered, not about the passed duration)

Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>
---
The new test_progs part has been tested in a local qemu environment:

  ./test_progs -a cgroup_get_current_cgroup_id
  47      cgroup_get_current_cgroup_id:OK
  Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
---
 tools/testing/selftests/bpf/.gitignore             |   1 -
 tools/testing/selftests/bpf/Makefile               |   3 +-
 tools/testing/selftests/bpf/get_cgroup_id_user.c   | 151 ---------------------
 .../bpf/prog_tests/cgroup_get_current_cgroup_id.c  |  58 ++++++++
 4 files changed, 59 insertions(+), 154 deletions(-)

Comments

Alan Maguire July 31, 2024, 5:23 p.m. UTC | #1 
On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> get_current_cgroup_id_user allows testing for bpf_get_current_cgroup_id()
> bpf API but is not integrated into test_progs, and so is not tested
> automatically in CI.
> 
> Convert it to the test_progs framework to allow running it automatically.
> The most notable differences with the old test are the following:
> - the new test relies on autoattach instead of manually hooking/enabling
>   the targeted tracepoint through perf_event, which reduces quite a lot the
>   test code size
> - sleep duration passed to nanosleep syscall has been reduced to its
>   minimum to not impact overall CI duration (we only care about the syscall
>   being properly triggered, not about the passed duration)
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

Nice work! Two small suggestions below..

> ---
> The new test_progs part has been tested in a local qemu environment:
> 
>   ./test_progs -a cgroup_get_current_cgroup_id
>   47      cgroup_get_current_cgroup_id:OK
>   Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> ---
>  tools/testing/selftests/bpf/.gitignore             |   1 -
>  tools/testing/selftests/bpf/Makefile               |   3 +-
>  tools/testing/selftests/bpf/get_cgroup_id_user.c   | 151 ---------------------
>  .../bpf/prog_tests/cgroup_get_current_cgroup_id.c  |  58 ++++++++
>  4 files changed, 59 insertions(+), 154 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
> index 4e4aae8aa7ec..108eb10626b9 100644
> --- a/tools/testing/selftests/bpf/.gitignore
> +++ b/tools/testing/selftests/bpf/.gitignore
> @@ -20,7 +20,6 @@ test_sock
>  urandom_read
>  test_sockmap
>  test_lirc_mode2_user
> -get_cgroup_id_user
>  test_skb_cgroup_id_user
>  test_cgroup_storage
>  test_flow_dissector
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 1d7a62e7deff..8d8483f81009 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -68,7 +68,7 @@ endif
>  # 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_dev_cgroup \
> -	test_sock test_sockmap get_cgroup_id_user \
> +	test_sock test_sockmap \
>  	test_cgroup_storage \
>  	test_tcpnotify_user test_sysctl \
>  	test_progs-no_alu32
> @@ -297,7 +297,6 @@ $(OUTPUT)/test_skb_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_sock: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
> -$(OUTPUT)/get_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_cgroup_storage: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
>  $(OUTPUT)/test_sysctl: $(CGROUP_HELPERS) $(TESTING_HELPERS)
> diff --git a/tools/testing/selftests/bpf/get_cgroup_id_user.c b/tools/testing/selftests/bpf/get_cgroup_id_user.c
> deleted file mode 100644
> index aefd83ebdcd7..000000000000
> --- a/tools/testing/selftests/bpf/get_cgroup_id_user.c
> +++ /dev/null
> @@ -1,151 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -// Copyright (c) 2018 Facebook
> -
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
> -#include <errno.h>
> -#include <fcntl.h>
> -#include <syscall.h>
> -#include <unistd.h>
> -#include <linux/perf_event.h>
> -#include <sys/ioctl.h>
> -#include <sys/time.h>
> -#include <sys/types.h>
> -#include <sys/stat.h>
> -
> -#include <linux/bpf.h>
> -#include <bpf/bpf.h>
> -#include <bpf/libbpf.h>
> -
> -#include "cgroup_helpers.h"
> -#include "testing_helpers.h"
> -
> -#define CHECK(condition, tag, format...) ({		\
> -	int __ret = !!(condition);			\
> -	if (__ret) {					\
> -		printf("%s:FAIL:%s ", __func__, tag);	\
> -		printf(format);				\
> -	} else {					\
> -		printf("%s:PASS:%s\n", __func__, tag);	\
> -	}						\
> -	__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)
> -		return -1;
> -	return bpf_map__fd(map);
> -}
> -
> -#define TEST_CGROUP "/test-bpf-get-cgroup-id/"
> -
> -int main(int argc, char **argv)
> -{
> -	const char *probe_name = "syscalls/sys_enter_nanosleep";
> -	const char *file = "get_cgroup_id_kern.bpf.o";
> -	int err, bytes, efd, prog_fd, pmu_fd;
> -	int cgroup_fd, cgidmap_fd, pidmap_fd;
> -	struct perf_event_attr attr = {};
> -	struct bpf_object *obj;
> -	__u64 kcgid = 0, ucgid;
> -	__u32 key = 0, pid;
> -	int exit_code = 1;
> -	char buf[256];
> -	const struct timespec req = {
> -		.tv_sec = 1,
> -		.tv_nsec = 0,
> -	};
> -
> -	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
> -	if (CHECK(cgroup_fd < 0, "cgroup_setup_and_join", "err %d errno %d\n", cgroup_fd, errno))
> -		return 1;
> -
> -	/* Use libbpf 1.0 API mode */
> -	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
> -
> -	err = bpf_prog_test_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
> -	if (CHECK(err, "bpf_prog_test_load", "err %d errno %d\n", err, errno))
> -		goto cleanup_cgroup_env;
> -
> -	cgidmap_fd = bpf_find_map(__func__, obj, "cg_ids");
> -	if (CHECK(cgidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
> -		  cgidmap_fd, errno))
> -		goto close_prog;
> -
> -	pidmap_fd = bpf_find_map(__func__, obj, "pidmap");
> -	if (CHECK(pidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
> -		  pidmap_fd, errno))
> -		goto close_prog;
> -
> -	pid = getpid();
> -	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
> -
> -	if (access("/sys/kernel/tracing/trace", F_OK) == 0) {
> -		snprintf(buf, sizeof(buf),
> -			 "/sys/kernel/tracing/events/%s/id", probe_name);
> -	} else {
> -		snprintf(buf, sizeof(buf),
> -			 "/sys/kernel/debug/tracing/events/%s/id", probe_name);
> -	}
> -	efd = open(buf, O_RDONLY, 0);
> -	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
> -		goto close_prog;
> -	bytes = read(efd, buf, sizeof(buf));
> -	close(efd);
> -	if (CHECK(bytes <= 0 || bytes >= sizeof(buf), "read",
> -		  "bytes %d errno %d\n", bytes, errno))
> -		goto close_prog;
> -
> -	attr.config = strtol(buf, NULL, 0);
> -	attr.type = PERF_TYPE_TRACEPOINT;
> -	attr.sample_type = PERF_SAMPLE_RAW;
> -	attr.sample_period = 1;
> -	attr.wakeup_events = 1;
> -
> -	/* attach to this pid so the all bpf invocations will be in the
> -	 * cgroup associated with this pid.
> -	 */
> -	pmu_fd = syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
> -	if (CHECK(pmu_fd < 0, "perf_event_open", "err %d errno %d\n", pmu_fd,
> -		  errno))
> -		goto close_prog;
> -
> -	err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
> -	if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n", err,
> -		  errno))
> -		goto close_pmu;
> -
> -	err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
> -	if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n", err,
> -		  errno))
> -		goto close_pmu;
> -
> -	/* trigger some syscalls */
> -	syscall(__NR_nanosleep, &req, NULL);
> -
> -	err = bpf_map_lookup_elem(cgidmap_fd, &key, &kcgid);
> -	if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
> -		goto close_pmu;
> -
> -	ucgid = get_cgroup_id(TEST_CGROUP);
> -	if (CHECK(kcgid != ucgid, "compare_cgroup_id",
> -		  "kern cgid %llx user cgid %llx", kcgid, ucgid))
> -		goto close_pmu;
> -
> -	exit_code = 0;
> -	printf("%s:PASS\n", argv[0]);
> -
> -close_pmu:
> -	close(pmu_fd);
> -close_prog:
> -	bpf_object__close(obj);
> -cleanup_cgroup_env:
> -	cleanup_cgroup_environment();
> -	return exit_code;
> -}
> diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c b/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c
> new file mode 100644
> index 000000000000..dd8835a63a00
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c
> @@ -0,0 +1,58 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <sys/stat.h>
> +#include <sys/sysmacros.h>
> +#include "test_progs.h"
> +#include "cgroup_helpers.h"
> +#include "get_cgroup_id_kern.skel.h"
> +
> +#define TEST_CGROUP "/test-bpf-get-cgroup-id/"
> +
> +void test_cgroup_get_current_cgroup_id(void)
> +{
> +	struct get_cgroup_id_kern *skel;
> +	const struct timespec req = {
> +		.tv_sec = 0,
> +		.tv_nsec = 1,
> +	};
> +	__u64 kcgid, ucgid;
> +	int cgroup_fd;
> +	int key = 0;
> +	int pid;
> +
> +	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
> +	if (!ASSERT_OK_FD(cgroup_fd, "cgroup switch"))
> +		return;
> +
> +	skel = get_cgroup_id_kern__open_and_load();
> +	if (!ASSERT_OK_PTR(skel, "load program"))
> +		goto cleanup_cgroup;
> +
> +	if (!ASSERT_OK(get_cgroup_id_kern__attach(skel), "attach bpf program"))
> +		goto cleanup_progs;
> +
> +	pid = getpid();
> +	if (!ASSERT_OK(bpf_map__update_elem(skel->maps.pidmap, &key,
> +					    sizeof(key), &pid, sizeof(pid), 0),
> +		       "write pid"))
> +		goto cleanup_progs;
> +

I think it would be worth using a global variable in the BPF program
my_pid, and setting skel->bss->my_pid here as other more up-to-date
tests do (example progs/test_usdt.c, prog_tests/usdt.c). No need for a
separate map anymore.

> +	/* trigger the syscall on which is attached the tested prog */
> +	if (!ASSERT_OK(syscall(__NR_nanosleep, &req, NULL), "nanosleep"))
> +		goto cleanup_progs;
> +
> +	if (!ASSERT_OK(bpf_map__lookup_elem(skel->maps.cg_ids, &key,
> +					    sizeof(key), &kcgid, sizeof(kcgid),
> +					    0),
> +		       "read bpf cgroup id"))
> +		goto cleanup_progs;
> +

ditto here, cg_ids could be a global var cg_id that the bpf prog sets
and we check here via skel->bss->cg_id.

> +	ucgid = get_cgroup_id(TEST_CGROUP);
> +
> +	ASSERT_EQ(kcgid, ucgid, "compare cgroup ids");
> +
> +cleanup_progs:
> +	get_cgroup_id_kern__destroy(skel);
> +cleanup_cgroup:
> +	cleanup_cgroup_environment();
> +}
>
Alexis Lothoré (eBPF Foundation) July 31, 2024, 6:53 p.m. UTC | #2 
Hello Alan,

On 7/31/24 19:23, Alan Maguire wrote:
> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:

[...]

>> +	pid = getpid();
>> +	if (!ASSERT_OK(bpf_map__update_elem(skel->maps.pidmap, &key,
>> +					    sizeof(key), &pid, sizeof(pid), 0),
>> +		       "write pid"))
>> +		goto cleanup_progs;
>> +
> 
> I think it would be worth using a global variable in the BPF program
> my_pid, and setting skel->bss->my_pid here as other more up-to-date
> tests do (example progs/test_usdt.c, prog_tests/usdt.c). No need for a
> separate map anymore.

That sounds like a good improvement, thanks for the hint and the example :) I'll
spin a new revision with this, and make sure to use it in my next test
conversion patches too when relevant.

TBH I am not familiar with global variables usage in ebpf/libbpf, so it is not
clear for me when I should prefer it over classic maps. From some quick search I
feel like it should be the default choice when needing basic controls
knobs/feedback on a bpf program from userspace ? Or maybe it should be used even
more broadly by default ?

>> +	/* trigger the syscall on which is attached the tested prog */
>> +	if (!ASSERT_OK(syscall(__NR_nanosleep, &req, NULL), "nanosleep"))
>> +		goto cleanup_progs;
>> +
>> +	if (!ASSERT_OK(bpf_map__lookup_elem(skel->maps.cg_ids, &key,
>> +					    sizeof(key), &kcgid, sizeof(kcgid),
>> +					    0),
>> +		       "read bpf cgroup id"))
>> +		goto cleanup_progs;
>> +
> 
> ditto here, cg_ids could be a global var cg_id that the bpf prog sets
> and we check here via skel->bss->cg_id.

ACK, I'll update this too.

Thanks,

Alexis
Alan Maguire Aug. 1, 2024, 8:17 a.m. UTC | #3 
On 31/07/2024 19:53, Alexis Lothoré wrote:
> Hello Alan,
> 
> On 7/31/24 19:23, Alan Maguire wrote:
>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
> 
> [...]
> 
>>> +	pid = getpid();
>>> +	if (!ASSERT_OK(bpf_map__update_elem(skel->maps.pidmap, &key,
>>> +					    sizeof(key), &pid, sizeof(pid), 0),
>>> +		       "write pid"))
>>> +		goto cleanup_progs;
>>> +
>>
>> I think it would be worth using a global variable in the BPF program
>> my_pid, and setting skel->bss->my_pid here as other more up-to-date
>> tests do (example progs/test_usdt.c, prog_tests/usdt.c). No need for a
>> separate map anymore.
> 
> That sounds like a good improvement, thanks for the hint and the example :) I'll
> spin a new revision with this, and make sure to use it in my next test
> conversion patches too when relevant.
> 
> TBH I am not familiar with global variables usage in ebpf/libbpf, so it is not
> clear for me when I should prefer it over classic maps. From some quick search I
> feel like it should be the default choice when needing basic controls
> knobs/feedback on a bpf program from userspace ? Or maybe it should be used even
> more broadly by default ?
> 

Yeah, it's certainly what I use by default, unless I need multiple
instances of an object. Under the hood, the BPF skeleton creates
single-element array maps for .bss, .data and .rodata sections which
contain all the initialized, uninitialized and constant globals in the
BPF object and mmaps() them so you can read/update the values in
userspace via skel->bss/skel->data without needing a map-related syscalls.


>>> +	/* trigger the syscall on which is attached the tested prog */
>>> +	if (!ASSERT_OK(syscall(__NR_nanosleep, &req, NULL), "nanosleep"))
>>> +		goto cleanup_progs;
>>> +
>>> +	if (!ASSERT_OK(bpf_map__lookup_elem(skel->maps.cg_ids, &key,
>>> +					    sizeof(key), &kcgid, sizeof(kcgid),
>>> +					    0),
>>> +		       "read bpf cgroup id"))
>>> +		goto cleanup_progs;
>>> +
>>
>> ditto here, cg_ids could be a global var cg_id that the bpf prog sets
>> and we check here via skel->bss->cg_id.
> 
> ACK, I'll update this too.
>

Great, thank you!

Alan
Alexis Lothoré (eBPF Foundation) Aug. 1, 2024, 8:57 a.m. UTC | #4 
Hello Alan,

On 8/1/24 10:17, Alan Maguire wrote:
> On 31/07/2024 19:53, Alexis Lothoré wrote:
>> Hello Alan,
>>
>> On 7/31/24 19:23, Alan Maguire wrote:
>>> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>>
>> [...]
>>
>>>> +	pid = getpid();
>>>> +	if (!ASSERT_OK(bpf_map__update_elem(skel->maps.pidmap, &key,
>>>> +					    sizeof(key), &pid, sizeof(pid), 0),
>>>> +		       "write pid"))
>>>> +		goto cleanup_progs;
>>>> +
>>>
>>> I think it would be worth using a global variable in the BPF program
>>> my_pid, and setting skel->bss->my_pid here as other more up-to-date
>>> tests do (example progs/test_usdt.c, prog_tests/usdt.c). No need for a
>>> separate map anymore.
>>
>> That sounds like a good improvement, thanks for the hint and the example :) I'll
>> spin a new revision with this, and make sure to use it in my next test
>> conversion patches too when relevant.
>>
>> TBH I am not familiar with global variables usage in ebpf/libbpf, so it is not
>> clear for me when I should prefer it over classic maps. From some quick search I
>> feel like it should be the default choice when needing basic controls
>> knobs/feedback on a bpf program from userspace ? Or maybe it should be used even
>> more broadly by default ?
>>
> 
> Yeah, it's certainly what I use by default, unless I need multiple
> instances of an object. Under the hood, the BPF skeleton creates
> single-element array maps for .bss, .data and .rodata sections which
> contain all the initialized, uninitialized and constant globals in the
> BPF object and mmaps() them so you can read/update the values in
> userspace via skel->bss/skel->data without needing a map-related syscalls.

Thanks a lot for the additional details, much appreciated :)
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index 4e4aae8aa7ec..108eb10626b9 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -20,7 +20,6 @@  test_sock
 urandom_read
 test_sockmap
 test_lirc_mode2_user
-get_cgroup_id_user
 test_skb_cgroup_id_user
 test_cgroup_storage
 test_flow_dissector
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 1d7a62e7deff..8d8483f81009 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -68,7 +68,7 @@  endif
 # 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_dev_cgroup \
-	test_sock test_sockmap get_cgroup_id_user \
+	test_sock test_sockmap \
 	test_cgroup_storage \
 	test_tcpnotify_user test_sysctl \
 	test_progs-no_alu32
@@ -297,7 +297,6 @@  $(OUTPUT)/test_skb_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_sock: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_tcpnotify_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(TRACE_HELPERS)
-$(OUTPUT)/get_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_cgroup_storage: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_sock_fields: $(CGROUP_HELPERS) $(TESTING_HELPERS)
 $(OUTPUT)/test_sysctl: $(CGROUP_HELPERS) $(TESTING_HELPERS)
diff --git a/tools/testing/selftests/bpf/get_cgroup_id_user.c b/tools/testing/selftests/bpf/get_cgroup_id_user.c
deleted file mode 100644
index aefd83ebdcd7..000000000000
--- a/tools/testing/selftests/bpf/get_cgroup_id_user.c
+++ /dev/null
@@ -1,151 +0,0 @@ 
-// SPDX-License-Identifier: GPL-2.0
-// Copyright (c) 2018 Facebook
-
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <syscall.h>
-#include <unistd.h>
-#include <linux/perf_event.h>
-#include <sys/ioctl.h>
-#include <sys/time.h>
-#include <sys/types.h>
-#include <sys/stat.h>
-
-#include <linux/bpf.h>
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "cgroup_helpers.h"
-#include "testing_helpers.h"
-
-#define CHECK(condition, tag, format...) ({		\
-	int __ret = !!(condition);			\
-	if (__ret) {					\
-		printf("%s:FAIL:%s ", __func__, tag);	\
-		printf(format);				\
-	} else {					\
-		printf("%s:PASS:%s\n", __func__, tag);	\
-	}						\
-	__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)
-		return -1;
-	return bpf_map__fd(map);
-}
-
-#define TEST_CGROUP "/test-bpf-get-cgroup-id/"
-
-int main(int argc, char **argv)
-{
-	const char *probe_name = "syscalls/sys_enter_nanosleep";
-	const char *file = "get_cgroup_id_kern.bpf.o";
-	int err, bytes, efd, prog_fd, pmu_fd;
-	int cgroup_fd, cgidmap_fd, pidmap_fd;
-	struct perf_event_attr attr = {};
-	struct bpf_object *obj;
-	__u64 kcgid = 0, ucgid;
-	__u32 key = 0, pid;
-	int exit_code = 1;
-	char buf[256];
-	const struct timespec req = {
-		.tv_sec = 1,
-		.tv_nsec = 0,
-	};
-
-	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
-	if (CHECK(cgroup_fd < 0, "cgroup_setup_and_join", "err %d errno %d\n", cgroup_fd, errno))
-		return 1;
-
-	/* Use libbpf 1.0 API mode */
-	libbpf_set_strict_mode(LIBBPF_STRICT_ALL);
-
-	err = bpf_prog_test_load(file, BPF_PROG_TYPE_TRACEPOINT, &obj, &prog_fd);
-	if (CHECK(err, "bpf_prog_test_load", "err %d errno %d\n", err, errno))
-		goto cleanup_cgroup_env;
-
-	cgidmap_fd = bpf_find_map(__func__, obj, "cg_ids");
-	if (CHECK(cgidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
-		  cgidmap_fd, errno))
-		goto close_prog;
-
-	pidmap_fd = bpf_find_map(__func__, obj, "pidmap");
-	if (CHECK(pidmap_fd < 0, "bpf_find_map", "err %d errno %d\n",
-		  pidmap_fd, errno))
-		goto close_prog;
-
-	pid = getpid();
-	bpf_map_update_elem(pidmap_fd, &key, &pid, 0);
-
-	if (access("/sys/kernel/tracing/trace", F_OK) == 0) {
-		snprintf(buf, sizeof(buf),
-			 "/sys/kernel/tracing/events/%s/id", probe_name);
-	} else {
-		snprintf(buf, sizeof(buf),
-			 "/sys/kernel/debug/tracing/events/%s/id", probe_name);
-	}
-	efd = open(buf, O_RDONLY, 0);
-	if (CHECK(efd < 0, "open", "err %d errno %d\n", efd, errno))
-		goto close_prog;
-	bytes = read(efd, buf, sizeof(buf));
-	close(efd);
-	if (CHECK(bytes <= 0 || bytes >= sizeof(buf), "read",
-		  "bytes %d errno %d\n", bytes, errno))
-		goto close_prog;
-
-	attr.config = strtol(buf, NULL, 0);
-	attr.type = PERF_TYPE_TRACEPOINT;
-	attr.sample_type = PERF_SAMPLE_RAW;
-	attr.sample_period = 1;
-	attr.wakeup_events = 1;
-
-	/* attach to this pid so the all bpf invocations will be in the
-	 * cgroup associated with this pid.
-	 */
-	pmu_fd = syscall(__NR_perf_event_open, &attr, getpid(), -1, -1, 0);
-	if (CHECK(pmu_fd < 0, "perf_event_open", "err %d errno %d\n", pmu_fd,
-		  errno))
-		goto close_prog;
-
-	err = ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
-	if (CHECK(err, "perf_event_ioc_enable", "err %d errno %d\n", err,
-		  errno))
-		goto close_pmu;
-
-	err = ioctl(pmu_fd, PERF_EVENT_IOC_SET_BPF, prog_fd);
-	if (CHECK(err, "perf_event_ioc_set_bpf", "err %d errno %d\n", err,
-		  errno))
-		goto close_pmu;
-
-	/* trigger some syscalls */
-	syscall(__NR_nanosleep, &req, NULL);
-
-	err = bpf_map_lookup_elem(cgidmap_fd, &key, &kcgid);
-	if (CHECK(err, "bpf_map_lookup_elem", "err %d errno %d\n", err, errno))
-		goto close_pmu;
-
-	ucgid = get_cgroup_id(TEST_CGROUP);
-	if (CHECK(kcgid != ucgid, "compare_cgroup_id",
-		  "kern cgid %llx user cgid %llx", kcgid, ucgid))
-		goto close_pmu;
-
-	exit_code = 0;
-	printf("%s:PASS\n", argv[0]);
-
-close_pmu:
-	close(pmu_fd);
-close_prog:
-	bpf_object__close(obj);
-cleanup_cgroup_env:
-	cleanup_cgroup_environment();
-	return exit_code;
-}
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c b/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c
new file mode 100644
index 000000000000..dd8835a63a00
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_get_current_cgroup_id.c
@@ -0,0 +1,58 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+#include <sys/stat.h>
+#include <sys/sysmacros.h>
+#include "test_progs.h"
+#include "cgroup_helpers.h"
+#include "get_cgroup_id_kern.skel.h"
+
+#define TEST_CGROUP "/test-bpf-get-cgroup-id/"
+
+void test_cgroup_get_current_cgroup_id(void)
+{
+	struct get_cgroup_id_kern *skel;
+	const struct timespec req = {
+		.tv_sec = 0,
+		.tv_nsec = 1,
+	};
+	__u64 kcgid, ucgid;
+	int cgroup_fd;
+	int key = 0;
+	int pid;
+
+	cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
+	if (!ASSERT_OK_FD(cgroup_fd, "cgroup switch"))
+		return;
+
+	skel = get_cgroup_id_kern__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "load program"))
+		goto cleanup_cgroup;
+
+	if (!ASSERT_OK(get_cgroup_id_kern__attach(skel), "attach bpf program"))
+		goto cleanup_progs;
+
+	pid = getpid();
+	if (!ASSERT_OK(bpf_map__update_elem(skel->maps.pidmap, &key,
+					    sizeof(key), &pid, sizeof(pid), 0),
+		       "write pid"))
+		goto cleanup_progs;
+
+	/* trigger the syscall on which is attached the tested prog */
+	if (!ASSERT_OK(syscall(__NR_nanosleep, &req, NULL), "nanosleep"))
+		goto cleanup_progs;
+
+	if (!ASSERT_OK(bpf_map__lookup_elem(skel->maps.cg_ids, &key,
+					    sizeof(key), &kcgid, sizeof(kcgid),
+					    0),
+		       "read bpf cgroup id"))
+		goto cleanup_progs;
+
+	ucgid = get_cgroup_id(TEST_CGROUP);
+
+	ASSERT_EQ(kcgid, ucgid, "compare cgroup ids");
+
+cleanup_progs:
+	get_cgroup_id_kern__destroy(skel);
+cleanup_cgroup:
+	cleanup_cgroup_environment();
+}