Message ID | 20200429035841.3959159-4-songliubraving@fb.com |
---|---|
State | Changes Requested |
Delegated to: | BPF Maintainers |
Headers | show |
Series | bpf: sharing bpf runtime stats with | expand |
On Tue, Apr 28, 2020 at 08:58:41PM -0700, Song Liu wrote: > + > + skel = test_enable_stats__open_and_load(); > + if (CHECK(!skel, "skel_open_and_load", "skeleton open/load failed\n")) > + return; > + > + stats_fd = bpf_enable_stats(BPF_STATS_RUNTIME_CNT); Just realized that the name is wrong. The stats are enabling run_cnt and run_time_ns. runtime_cnt sounds like 'snark' from 'The Hunting of the Snark' :) May be BPF_STATS_RUN_TIME ? > + if (CHECK(stats_fd < 0, "get_stats_fd", "failed %d\n", errno)) { > + test_enable_stats__destroy(skel); > + return; > + } > + > + err = test_enable_stats__attach(skel); > + if (CHECK(err, "attach_raw_tp", "err %d\n", err)) > + goto cleanup; > + > + /* generate 100 sys_enter */ > + for (i = 0; i < 100; i++) > + usleep(1); > + > + test_enable_stats__detach(skel); > + > + prog_fd = bpf_program__fd(skel->progs.test_enable_stats); > + memset(&info, 0, info_len); > + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len); > + if (CHECK(err, "get_prog_info", > + "failed to get bpf_prog_info for fd %d\n", prog_fd)) > + goto cleanup; > + if (CHECK(info.run_time_ns == 0, "check_stats_enabled", > + "failed to enable run_time_ns stats\n")) > + goto cleanup; > + > + bss_fd = bpf_map__fd(skel->maps.bss); > + err = bpf_map_lookup_elem(bss_fd, &zero, &count); 'count' is a global var. It's accessible directly via skeleton. No need for map_lookup. Even after __detach(skel) the global data is still valid. > + if (CHECK(err, "map_lookup_elem", > + "failed map_lookup_elem for fd %d\n", bss_fd)) > + goto cleanup; > + > + CHECK(info.run_cnt != count, "check_run_cnt_valid", > + "invalid run_cnt stats\n"); what happens if there are other syscalls during for(i<100) loop? The count will still match, right? Then why 100 ? and why usleep() at all? test_enable_stats__attach() will generate at least one syscall. > + > +cleanup: > + test_enable_stats__destroy(skel); > + close(stats_fd); May be close(stats_fd) first. Then test_enable_stats__attach(skel); again. Generate few more syscalls and check that 'count' incrementing, but info.run_cnt doesnt ? That check assumes that sysctl is off. Overkill?
> On Apr 28, 2020, at 9:57 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Apr 28, 2020 at 08:58:41PM -0700, Song Liu wrote: >> + >> + skel = test_enable_stats__open_and_load(); >> + if (CHECK(!skel, "skel_open_and_load", "skeleton open/load failed\n")) >> + return; >> + >> + stats_fd = bpf_enable_stats(BPF_STATS_RUNTIME_CNT); > > Just realized that the name is wrong. > The stats are enabling run_cnt and run_time_ns. > runtime_cnt sounds like 'snark' from 'The Hunting of the Snark' :) > May be BPF_STATS_RUN_TIME ? Will fix. [...] >> + >> + CHECK(info.run_cnt != count, "check_run_cnt_valid", >> + "invalid run_cnt stats\n"); > > what happens if there are other syscalls during for(i<100) loop? > The count will still match, right? > Then why 100 ? and why usleep() at all? > test_enable_stats__attach() will generate at least one syscall. We don't really need usleep. I was thinking if it matches after many calls it really matches... I will remove it. > >> + >> +cleanup: >> + test_enable_stats__destroy(skel); >> + close(stats_fd); > > May be close(stats_fd) first. > Then test_enable_stats__attach(skel); again. > Generate few more syscalls and check that 'count' incrementing, > but info.run_cnt doesnt ? > That check assumes that sysctl is off. Overkill? I thought about this. However, close(stats_fd) cannot guarantee the stats is not enabled by other fd or the sysctl. I think this will generate noise on specific systems. Thanks, Song
diff --git a/tools/testing/selftests/bpf/prog_tests/enable_stats.c b/tools/testing/selftests/bpf/prog_tests/enable_stats.c new file mode 100644 index 000000000000..4930efe4cd2b --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <test_progs.h> +#include <sys/mman.h> +#include "test_enable_stats.skel.h" + +void test_enable_stats(void) +{ + int stats_fd, err, prog_fd, bss_fd, i; + struct test_enable_stats *skel; + struct bpf_prog_info info; + __u32 info_len = sizeof(info); + int duration = 0; + __u32 zero = 0; + __u64 count; + + skel = test_enable_stats__open_and_load(); + if (CHECK(!skel, "skel_open_and_load", "skeleton open/load failed\n")) + return; + + stats_fd = bpf_enable_stats(BPF_STATS_RUNTIME_CNT); + if (CHECK(stats_fd < 0, "get_stats_fd", "failed %d\n", errno)) { + test_enable_stats__destroy(skel); + return; + } + + err = test_enable_stats__attach(skel); + if (CHECK(err, "attach_raw_tp", "err %d\n", err)) + goto cleanup; + + /* generate 100 sys_enter */ + for (i = 0; i < 100; i++) + usleep(1); + + test_enable_stats__detach(skel); + + prog_fd = bpf_program__fd(skel->progs.test_enable_stats); + memset(&info, 0, info_len); + err = bpf_obj_get_info_by_fd(prog_fd, &info, &info_len); + if (CHECK(err, "get_prog_info", + "failed to get bpf_prog_info for fd %d\n", prog_fd)) + goto cleanup; + if (CHECK(info.run_time_ns == 0, "check_stats_enabled", + "failed to enable run_time_ns stats\n")) + goto cleanup; + + bss_fd = bpf_map__fd(skel->maps.bss); + err = bpf_map_lookup_elem(bss_fd, &zero, &count); + if (CHECK(err, "map_lookup_elem", + "failed map_lookup_elem for fd %d\n", bss_fd)) + goto cleanup; + + CHECK(info.run_cnt != count, "check_run_cnt_valid", + "invalid run_cnt stats\n"); + +cleanup: + test_enable_stats__destroy(skel); + close(stats_fd); +} diff --git a/tools/testing/selftests/bpf/progs/test_enable_stats.c b/tools/testing/selftests/bpf/progs/test_enable_stats.c new file mode 100644 index 000000000000..dfd987e4ede4 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_enable_stats.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2020 Facebook + +#include <linux/bpf.h> +#include <stdint.h> +#include <linux/types.h> +#include <bpf/bpf_helpers.h> + +char _license[] SEC("license") = "GPL"; + +static __u64 count; + +SEC("raw_tracepoint/sys_enter") +int test_enable_stats(void *ctx) +{ + count += 1; + return 0; +}
Add test for BPF_ENABLE_STATS, which should enable run_time_ns stats. ~/selftests/bpf# ./test_progs -t enable_stats -v test_enable_stats:PASS:skel_open_and_load 0 nsec test_enable_stats:PASS:get_stats_fd 0 nsec test_enable_stats:PASS:attach_raw_tp 0 nsec test_enable_stats:PASS:get_prog_info 0 nsec test_enable_stats:PASS:check_stats_enabled 0 nsec test_enable_stats:PASS:map_lookup_elem 0 nsec test_enable_stats:PASS:check_run_cnt_valid 0 nsec Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Song Liu <songliubraving@fb.com> --- .../selftests/bpf/prog_tests/enable_stats.c | 58 +++++++++++++++++++ .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++ 2 files changed, 76 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/enable_stats.c create mode 100644 tools/testing/selftests/bpf/progs/test_enable_stats.c