> On Apr 30, 2020, at 12:02 AM, Yonghong Song <y...@fb.com> wrote:
>
>
>
> On 4/29/20 10:12 PM, Song Liu wrote:
>>> On Apr 29, 2020, at 7:23 PM, Andrii Nakryiko <andrii.nakry...@gmail.com>
>>> wrote:
>>>
>>> On Tue, Apr 28, 2020 at 11:47 PM Song Liu <songliubrav...@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 <songliubrav...@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
