> On Apr 28, 2020, at 5:43 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> 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
