Re: [PATCH bpf-next v2] selftests/bpf: test for bpf_get_file_path() from raw tracepoint

[Wenbo Zhang <ethercflow@gmail.com>]

> Unless there is a real reason for all this complexity (in which case,
> please spell it out in commit or comments), I think this could be so
> much simpler.

Yes, it could be so much simpler than this implement.

> - you don't have to use perf_buffer to pass data back, just use global data;
> - you can add a filter for PID to only capture data triggered by test
> process and avoid the noise;

Yes, I'll use map instead of perf buffer to communicate.

> - why all those set_affinity dances? Is it just because you used
> existing perf_buffer test which did that to specifically test
> perf_buffer delivering data across every CPU core? Seems like your
> test doesn't care about that...

I used fd2path_loadgen to get hundreds of syscalls between multi usleep, which
may cause it's sched to different cores, but as you say, this test
doesn't care about
that... I'll remove them with perf buffer.

> - do we really need a separate binary generating hundreds of syscalls?
> It's hard to synchronize with test and it seems much simpler to just
> trigger necessary syscalls synchronously from the test itself, no?

This is my fault, necessary syscalls synchronously from the test
itself is enough.

> I have a bunch of more minutia nits, but most of them will go away if
> you simplify your testing approach anyway, so I'll postpone them till
> then.

Thanks a lot. I'll modify a simplified version then recommitted.

Andrii Nakryiko <andrii.nakryiko@gmail.com> 于2019年11月2日周六 下午1:49写道：

>
> On Fri, Nov 1, 2019 at 6:08 AM Wenbo Zhang <ethercflow@gmail.com> wrote:
> >
> > trace fstat events by raw tracepoint sys_enter:newfstat, and handle events
> > only produced by fd2path_loadgen, the fd2path_loadgen call fstat on several
> > different types of files to test bpf_get_file_path's feature.
> > ---
>
> Unless there is a real reason for all this complexity (in which case,
> please spell it out in commit or comments), I think this could be so
> much simpler.
>
> - you don't have to use perf_buffer to pass data back, just use global data;
> - you can add a filter for PID to only capture data triggered by test
> process and avoid the noise;
> - why all those set_affinity dances? Is it just because you used
> existing perf_buffer test which did that to specifically test
> perf_buffer delivering data across every CPU core? Seems like your
> test doesn't care about that...
> - do we really need a separate binary generating hundreds of syscalls?
> It's hard to synchronize with test and it seems much simpler to just
> trigger necessary syscalls synchronously from the test itself, no?
>
> I have a bunch of more minutia nits, but most of them will go away if
> you simplify your testing approach anyway, so I'll postpone them till
> then.
>
> >  tools/testing/selftests/bpf/Makefile          |   8 +-
> >  tools/testing/selftests/bpf/fd2path_loadgen.c |  75 ++++++++++
> >  .../selftests/bpf/prog_tests/get_file_path.c  | 130 ++++++++++++++++++
> >  .../selftests/bpf/progs/test_get_file_path.c  |  58 ++++++++
> >  4 files changed, 269 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/fd2path_loadgen.c
> >  create mode 100644 tools/testing/selftests/bpf/prog_tests/get_file_path.c
> >  create mode 100644 tools/testing/selftests/bpf/progs/test_get_file_path.c
> >
>
> [...]