Re: [PATCH v6 bpf-next 3/3] bpf: add selftest for BPF_ENABLE_STATS

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> 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





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux