| Message ID | 20200429002922.3064669-4-songliubraving@fb.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: sharing bpf runtime stats with | expand |
On 4/28/20 5:29 PM, Song Liu wrote: > 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 > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED > > Signed-off-by: Song Liu <songliubraving@fb.com> > --- > .../selftests/bpf/prog_tests/enable_stats.c | 45 +++++++++++++++++++ > .../selftests/bpf/progs/test_enable_stats.c | 21 +++++++++ > 2 files changed, 66 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 > > 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..987fc743ab75 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c > @@ -0,0 +1,45 @@ > +// 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) > +{ > + struct test_enable_stats *skel; > + struct bpf_prog_info info = {}; > + __u32 info_len = sizeof(info); > + int stats_fd, err, prog_fd; > + int duration = 0; > + > + 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)) > + goto cleanup; > + > + err = test_enable_stats__attach(skel); > + > + if (CHECK(err, "attach_raw_tp", "err %d\n", err)) > + goto cleanup; > + > + usleep(1000); > + > + prog_fd = bpf_program__fd(skel->progs.test_enable_stats); > + 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; > + > + CHECK(info.run_time_ns == 0, "check_stats_enabled", > + "failed to enable run_time_ns stats\n"); > + > +cleanup: > + test_enable_stats__destroy(skel); > + if (stats_fd >= 0) > + 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..aa9626330a9e > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_enable_stats.c > @@ -0,0 +1,21 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2020 Facebook > + > +#include <linux/bpf.h> > +#include <stdint.h> > +#include <bpf/bpf_helpers.h> > + > +char _license[] SEC("license") = "GPL"; > + > +static int val = 1; > + > +SEC("raw_tracepoint/sys_enter") > +int test_enable_stats(void *ctx) > +{ > + __u32 key = 0; > + __u64 *val; The above two declarations (key/val) are not needed, esp. "val" is shadowing. Maybe the maintainer can fix it up before merging if there is no other changes for this patch set. > + > + val += 1; > + > + return 0; > +} >
On Tue, Apr 28, 2020 at 05:33:54PM -0700, Yonghong Song wrote: > > > On 4/28/20 5:29 PM, Song Liu wrote: > > 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 > > Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED ... > > +static int val = 1; > > + > > +SEC("raw_tracepoint/sys_enter") > > +int test_enable_stats(void *ctx) > > +{ > > + __u32 key = 0; > > + __u64 *val; > > The above two declarations (key/val) are not needed, > esp. "val" is shadowing. > Maybe the maintainer can fix it up before merging > if there is no other changes for this patch set. > > > + > > + val += 1; I think 'PASSED' above is quite misleading. How it can pass when it wasn't incremented? The user space test_enable_stats() doesn't check this val. Please fix. usleep(1000); needs an explanation as well. Why 1000 ? It should work with any syscall. like getpid ? and with value 1 ? Since there is bpf_obj_get_info_by_fd() that usleep() is unnecessary. What am I missing?
> On Apr 28, 2020, at 5:43 PM, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Apr 28, 2020 at 05:33:54PM -0700, Yonghong Song wrote: >> >> >> On 4/28/20 5:29 PM, Song Liu wrote: >>> 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 >>> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED > ... >>> +static int val = 1; >>> + >>> +SEC("raw_tracepoint/sys_enter") >>> +int test_enable_stats(void *ctx) >>> +{ >>> + __u32 key = 0; >>> + __u64 *val; >> >> The above two declarations (key/val) are not needed, >> esp. "val" is shadowing. >> Maybe the maintainer can fix it up before merging >> if there is no other changes for this patch set. >> >>> + >>> + val += 1; > > I think 'PASSED' above is quite misleading. > How it can pass when it wasn't incremented? > The user space test_enable_stats() doesn't check this val. > Please fix. > > usleep(1000); needs an explanation as well. > Why 1000 ? It should work with any syscall. like getpid ? > and with value 1 ? > Since there is bpf_obj_get_info_by_fd() that usleep() > is unnecessary. What am I missing? This test currently doesn't test the value. It simply checks run_time_ns is none zero. I guess it is good to actually test the value. Let me add that. Thanks, Son
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..987fc743ab75 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c @@ -0,0 +1,45 @@ +// 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) +{ + struct test_enable_stats *skel; + struct bpf_prog_info info = {}; + __u32 info_len = sizeof(info); + int stats_fd, err, prog_fd; + int duration = 0; + + 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)) + goto cleanup; + + err = test_enable_stats__attach(skel); + + if (CHECK(err, "attach_raw_tp", "err %d\n", err)) + goto cleanup; + + usleep(1000); + + prog_fd = bpf_program__fd(skel->progs.test_enable_stats); + 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; + + CHECK(info.run_time_ns == 0, "check_stats_enabled", + "failed to enable run_time_ns stats\n"); + +cleanup: + test_enable_stats__destroy(skel); + if (stats_fd >= 0) + 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..aa9626330a9e --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_enable_stats.c @@ -0,0 +1,21 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2020 Facebook + +#include <linux/bpf.h> +#include <stdint.h> +#include <bpf/bpf_helpers.h> + +char _license[] SEC("license") = "GPL"; + +static int val = 1; + +SEC("raw_tracepoint/sys_enter") +int test_enable_stats(void *ctx) +{ + __u32 key = 0; + __u64 *val; + + val += 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 Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Song Liu <songliubraving@fb.com> --- .../selftests/bpf/prog_tests/enable_stats.c | 45 +++++++++++++++++++ .../selftests/bpf/progs/test_enable_stats.c | 21 +++++++++ 2 files changed, 66 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