> On Apr 28, 2020, at 9:57 PM, Alexei Starovoitov <alexei.starovoitov@xxxxxxxxx> wrote: > > On Tue, Apr 28, 2020 at 08:58:41PM -0700, Song Liu wrote: >> + >> + 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); > > Just realized that the name is wrong. > The stats are enabling run_cnt and run_time_ns. > runtime_cnt sounds like 'snark' from 'The Hunting of the Snark' :) > May be BPF_STATS_RUN_TIME ? Will fix. [...] >> + >> + CHECK(info.run_cnt != count, "check_run_cnt_valid", >> + "invalid run_cnt stats\n"); > > what happens if there are other syscalls during for(i<100) loop? > The count will still match, right? > Then why 100 ? and why usleep() at all? > test_enable_stats__attach() will generate at least one syscall. We don't really need usleep. I was thinking if it matches after many calls it really matches... I will remove it. > >> + >> +cleanup: >> + test_enable_stats__destroy(skel); >> + close(stats_fd); > > May be close(stats_fd) first. > Then test_enable_stats__attach(skel); again. > Generate few more syscalls and check that 'count' incrementing, > but info.run_cnt doesnt ? > That check assumes that sysctl is off. Overkill? I thought about this. However, close(stats_fd) cannot guarantee the stats is not enabled by other fd or the sysctl. I think this will generate noise on specific systems. Thanks, Song
