| Message ID | 20200429064543.634465-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 11:47 PM Song Liu <songliubraving@fb.com> 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 > 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 | 46 +++++++++++++++++++ > .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++++ > 2 files changed, 64 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..cb5e34dcfd42 > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c > @@ -0,0 +1,46 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#include <test_progs.h> > +#include <sys/mman.h> is this header used for anything? > +#include "test_enable_stats.skel.h" > + > +void test_enable_stats(void) > +{ [...] > + > +char _license[] SEC("license") = "GPL"; > + > +static __u64 count; this is actually very unreliable, because compiler might decide to just remove this variable. It should be either `static volatile`, or better use zero-initialized global variable: __u64 count = 0; > + > +SEC("raw_tracepoint/sys_enter") > +int test_enable_stats(void *ctx) > +{ > + count += 1; > + return 0; > +} > -- > 2.24.1 >
> On Apr 29, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > On Tue, Apr 28, 2020 at 11:47 PM Song Liu <songliubraving@fb.com> 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 >> 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 | 46 +++++++++++++++++++ >> .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++++ >> 2 files changed, 64 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..cb5e34dcfd42 >> --- /dev/null >> +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c >> @@ -0,0 +1,46 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +#include <test_progs.h> >> +#include <sys/mman.h> > > is this header used for anything? Not really, will remove it. > >> +#include "test_enable_stats.skel.h" >> + >> +void test_enable_stats(void) >> +{ > > [...] > >> + >> +char _license[] SEC("license") = "GPL"; >> + >> +static __u64 count; > > this is actually very unreliable, because compiler might decide to > just remove this variable. It should be either `static volatile`, or > better use zero-initialized global variable: > > __u64 count = 0; Why would compile remove it? Is it because "static" or "no initialized? Would "__u64 count;" work? For "__u64 count = 0;", checkpatch.pl generates an error: ERROR: do not initialise globals to 0 #92: FILE: tools/testing/selftests/bpf/progs/test_enable_stats.c:11: +__u64 count = 0; Thanks, Song
On Wed, Apr 29, 2020 at 10:12 PM Song Liu <songliubraving@fb.com> wrote: > > > > > On Apr 29, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > > > > On Tue, Apr 28, 2020 at 11:47 PM Song Liu <songliubraving@fb.com> 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 > >> 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 | 46 +++++++++++++++++++ > >> .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++++ > >> 2 files changed, 64 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..cb5e34dcfd42 > >> --- /dev/null > >> +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c > >> @@ -0,0 +1,46 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +#include <test_progs.h> > >> +#include <sys/mman.h> > > > > is this header used for anything? > > Not really, will remove it. > > > > >> +#include "test_enable_stats.skel.h" > >> + > >> +void test_enable_stats(void) > >> +{ > > > > [...] > > > >> + > >> +char _license[] SEC("license") = "GPL"; > >> + > >> +static __u64 count; > > > > this is actually very unreliable, because compiler might decide to > > just remove this variable. It should be either `static volatile`, or > > better use zero-initialized global variable: > > > > __u64 count = 0; > > Why would compile remove it? Is it because "static" or "no initialized? because static, which makes compiler assume that no one else can access it (which is not true for BPF programs). > Would "__u64 count;" work? unfortunately, no, libbpf enforces that all global variables are initialized (uninitialized global variables go into special COM section, libbpf doesn't support it). > > For "__u64 count = 0;", checkpatch.pl generates an error: > > ERROR: do not initialise globals to 0 > #92: FILE: tools/testing/selftests/bpf/progs/test_enable_stats.c:11: > +__u64 count = 0; ignore checkpatch.pl in this case? > > Thanks, > Song
On 4/29/20 10:12 PM, Song Liu wrote: > > >> On Apr 29, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >> >> On Tue, Apr 28, 2020 at 11:47 PM Song Liu <songliubraving@fb.com> 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 >>> 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 | 46 +++++++++++++++++++ >>> .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++++ >>> 2 files changed, 64 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..cb5e34dcfd42 >>> --- /dev/null >>> +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c >>> @@ -0,0 +1,46 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +#include <test_progs.h> >>> +#include <sys/mman.h> >> >> is this header used for anything? > > Not really, will remove it. > >> >>> +#include "test_enable_stats.skel.h" >>> + >>> +void test_enable_stats(void) >>> +{ >> >> [...] >> >>> + >>> +char _license[] SEC("license") = "GPL"; >>> + >>> +static __u64 count; >> >> this is actually very unreliable, because compiler might decide to >> just remove this variable. It should be either `static volatile`, or >> better use zero-initialized global variable: >> >> __u64 count = 0; > > Why would compile remove it? Is it because "static" or "no initialized? > Would "__u64 count;" work? It is because of "static". This static variable has file scope and the compiler COULD remove count+=1 since it does not have any other effect other than incrementting itself and nobody uses it. > > For "__u64 count = 0;", checkpatch.pl generates an error: > > ERROR: do not initialise globals to 0 > #92: FILE: tools/testing/selftests/bpf/progs/test_enable_stats.c:11: > +__u64 count = 0; I think this is okay. For llvm10, you have to use `__u64 count = 0`. For llvm11, you can use "__u64 count", the compiler changed global "common" variable treatment default from as a "common" var to as a "bss" var. In selftest, we have numerous cases for `__u64 count = 0` style definitions and I recommend to use it as well since probably quite some people uses llvm10 to compile/run selftests. > > Thanks, > Song >
> On Apr 30, 2020, at 12:02 AM, Yonghong Song <yhs@fb.com> wrote: > > > > On 4/29/20 10:12 PM, Song Liu wrote: >>> On Apr 29, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: >>> >>> On Tue, Apr 28, 2020 at 11:47 PM Song Liu <songliubraving@fb.com> 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 >>>> 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 | 46 +++++++++++++++++++ >>>> .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++++ >>>> 2 files changed, 64 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..cb5e34dcfd42 >>>> --- /dev/null >>>> +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c >>>> @@ -0,0 +1,46 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +#include <test_progs.h> >>>> +#include <sys/mman.h> >>> >>> is this header used for anything? >> Not really, will remove it. >>> >>>> +#include "test_enable_stats.skel.h" >>>> + >>>> +void test_enable_stats(void) >>>> +{ >>> >>> [...] >>> >>>> + >>>> +char _license[] SEC("license") = "GPL"; >>>> + >>>> +static __u64 count; >>> >>> this is actually very unreliable, because compiler might decide to >>> just remove this variable. It should be either `static volatile`, or >>> better use zero-initialized global variable: >>> >>> __u64 count = 0; >> Why would compile remove it? Is it because "static" or "no initialized? >> Would "__u64 count;" work? > > It is because of "static". This static variable has file scope and the > compiler COULD remove count+=1 since it does not have any other effect > other than incrementting itself and nobody uses it. > >> For "__u64 count = 0;", checkpatch.pl generates an error: >> ERROR: do not initialise globals to 0 >> #92: FILE: tools/testing/selftests/bpf/progs/test_enable_stats.c:11: >> +__u64 count = 0; > > I think this is okay. > > For llvm10, you have to use `__u64 count = 0`. > For llvm11, you can use "__u64 count", the compiler changed global "common" variable treatment default from as a "common" var > to as a "bss" var. > > In selftest, we have numerous cases for `__u64 count = 0` style > definitions and I recommend to use it as well since probably > quite some people uses llvm10 to compile/run selftests. Thanks for the explanation. Will send fixed version shortly. 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..cb5e34dcfd42 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/enable_stats.c @@ -0,0 +1,46 @@ +// 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; + int stats_fd, err, prog_fd; + struct bpf_prog_info info; + __u32 info_len = sizeof(info); + 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_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; + + 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; + + CHECK(info.run_cnt != skel->bss->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: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 | 46 +++++++++++++++++++ .../selftests/bpf/progs/test_enable_stats.c | 18 ++++++++ 2 files changed, 64 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