Re: [PATCH v11 00/39] perf tools: filtering events using eBPF programs - part1

[pi3orama <pi3orama@163.com>]

发自我的 iPhone

> 在 2015年7月8日，下午10:03，Arnaldo Carvalho de Melo <acme@kernel.org> 写道：
> 
> Em Wed, Jul 08, 2015 at 01:13:49PM +0000, Wang Nan escreveu:
>> Hi Arnaldo Carvalho de Melo,
> 
> Hi Wang (hope this shorter form is ok on your country, calling me just
> "Arnaldo" is fine in mine :-))
> 
>>   I rearranged the first 39 patches of this patchset according to
>> your comments. After applying all of them you can use a hello world
>> BPF program for testing. They are based on your 'tmp.perf/ebpf', commit
>> 60cd37eb100c4880b28078a47f3062fac7572095.
> 
>>  I hope I can manage a public avaliable git repository for you
>> tomorrow (tomorrow means 24 hours later). What about a repository on
>> github? However I have to do this out of my office because of company's
>> IT policy.
> 
> Why not ask the kernel.org admins for a:
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/wangnan0/linux.git
> 
> Area?
> 

Is that possible for me? Could you please provide further instruction (or web page describe those instructions) so I can follow to form my application?

>> In this v11 you can see following improvements:
>> 
>> Commit messages improvements:
>> 'bpf tools: Collect symbol table from SHT_SYMTAB section'
>> 'bpf tools: Collect relocation sections from SHT_REL sections'
>> 'bpf tools: Record map accessing instructions for each program'
>> 'bpf tools: Relocate eBPF programs'
>> 'bpf tools: Link all bpf objects onto a list'
>> 
>> Decoupling:
>> 'bpf tools: Collect eBPF programs from their own sections'
>> 'bpf tools: Introduce accessors for struct bpf_program'
>> 
>> Renaming: bpf_object__for_each -> bpf_object__for_each_safe
>> 'bpf tools: Link all bpf objects onto a list'
>> 
>> Patch ordering:
>> 'perf tools: Make perf depend on libbpf'
>> 
>> Error message improvement (refer to http://llvm.org/apt):
>> 'perf tools: Call clang to compile C source to object code'
>> 
>> In this v11 part 1 patch set, I haven't follow your comment in
>> 'bpf tools: Introduce accessors for struct bpf_object' that let me
>> update accessors API from returning error code to returning actual
>> value and indicate error using invalid values. I prefer current API
>> because I saw and fixed many bugs related to error code in perf's
>> code (like commit ed30775).
> 
>> Reason of those bugs are misusing of error code: some part of code
>> return negative on error, some part of code return non-zero on error,
>> and developer forgot them. I don't want libbpf to introduce more bugs
>> like them. But if you insist on it, I'll change it.
> 
> If you don't follow the chosen convention, bugs appear.
> 
> And the convention of returning < 0 for errors and >= 0 for success is
> common, just see the libc wrappers for syscalls, see the open, read,
> write man pages, etc, that is an ooooold convention :-)
> 
> And those wrappers struck me as exaggerated, see one of them:
> 
> int bpf_program__get_fd(struct bpf_program *prog, int *pfd)
> {            
>    if (!pfd)
>        return -EINVAL;
> 
>    *pfd = prog->fd;
>    return 0;
> }
> 
> What can go wrong with accessing a struct member? The only think I
> thought about was: hey, the struct pointer needs to be checked against
> NULL, but no, in this case what you thought could go wrong was for the
> library user to pass a NULL pointer as the return place (pfd).
> 

OK, I will change. You won't see it in next version.

However, for the specific function you mentioned, please see patch 39/39 in this patchset. It shows that, accessing a struct member can go wrong in a way unpredictable when start working on this patch.

When I start working on this patch I thought this function may go wrong in only two obvious situations: 1: caller feed a NULL pointer; 2: try to get file descriptor before loading that program. However, when we start working on BPF prologue generator we realize that we must provide a way to enable one program be loaded multiple times with different prologues. Patch 39/39 is the winner among many ideas which provides the desire function while not impact old code (new code for you) too much, which we discussed and tries for weeks. In that patch we allow caller attach a pre-processor to a program, and require them call a new function bpf_program__get_nth_fd() to get the nth descriptor. The semantic of old bpf_program__get_fd() becomes hard to define so we simply disallow them to call it if preprocessing is required. This is how a new source of error raise during development.

Thank you.

> So, yes, I still think this is way exaggerated, if you insist that the
> struct must be opaque and thus we need accessors, I think that having:
> 
> int bpf_program__fd(struct bpf_program *prog)
> {
>    return prog->fd;
> }
> 
> Is way more sane, yes, I would trow away those extra four characters
> (get_).
> 
> Heck, in this case there is not even a possible problem where we would
> want to return something negative instead of doing what was requested.
> 
> If you find any other part in tools/perf/ (or anywhere else) that
> doesn't follows the convention it states it conforms to, please file a
> bug or submit a patch, like you did in the case you mentioned (ed30775),
> it would be a bug and has to be fixed.
> 
> - Arnaldo
> 
>> Wang Nan (39):
>>  bpf: Use correct #ifdef controller for trace_call_bpf()
>>  tracing, perf: Implement BPF programs attached to uprobes
>>  bpf tools: Introduce 'bpf' library and add bpf feature check
>>  bpf tools: Allow caller to set printing function
>>  bpf tools: Open eBPF object file and do basic validation
>>  bpf tools: Read eBPF object from buffer
>>  bpf tools: Check endianness and make libbpf fail early
>>  bpf tools: Iterate over ELF sections to collect information
>>  bpf tools: Collect version and license from ELF sections
>>  bpf tools: Collect map definitions from 'maps' section
>>  bpf tools: Collect symbol table from SHT_SYMTAB section
>>  bpf tools: Collect eBPF programs from their own sections
>>  bpf tools: Collect relocation sections from SHT_REL sections
>>  bpf tools: Record map accessing instructions for each program
>>  bpf tools: Add bpf.c/h for common bpf operations
>>  bpf tools: Create eBPF maps defined in an object file
>>  bpf tools: Relocate eBPF programs
>>  bpf tools: Introduce bpf_load_program() to bpf.c
>>  bpf tools: Load eBPF programs in object files into kernel
>>  bpf tools: Introduce accessors for struct bpf_program
>>  bpf tools: Introduce accessors for struct bpf_object
>>  bpf tools: Link all bpf objects onto a list
>>  perf tools: Introduce llvm config options
>>  perf tools: Call clang to compile C source to object code
>>  perf tools: Auto detecting kernel build directory
>>  perf tools: Auto detecting kernel include options
>>  perf tests: Add LLVM test for eBPF on-the-fly compiling
>>  perf tools: Make perf depend on libbpf
>>  perf record: Enable passing bpf object file to --event
>>  perf record: Compile scriptlets if pass '.c' to --event
>>  perf tools: Parse probe points of eBPF programs during preparation
>>  perf probe: Attach trace_probe_event with perf_probe_event
>>  perf record: Probe at kprobe points
>>  perf record: Load all eBPF object into kernel
>>  perf tools: Add bpf_fd field to evsel and config it
>>  perf tools: Attach eBPF program to perf event
>>  perf tools: Suppress probing messages when probing by BPF loading
>>  perf record: Add clang options for compiling BPF scripts
>>  bpf tools: Load a program with different instance using preprocessor
>> 
>> include/linux/trace_events.h    |    7 +-
>> kernel/events/core.c            |    4 +-
>> kernel/trace/Kconfig            |    2 +-
>> kernel/trace/trace_uprobe.c     |    5 +
>> tools/build/Makefile.feature    |    6 +-
>> tools/build/feature/Makefile    |    6 +-
>> tools/build/feature/test-bpf.c  |   18 +
>> tools/lib/bpf/.gitignore        |    2 +
>> tools/lib/bpf/Build             |    1 +
>> tools/lib/bpf/Makefile          |  195 +++++++
>> tools/lib/bpf/bpf.c             |   85 +++
>> tools/lib/bpf/bpf.h             |   23 +
>> tools/lib/bpf/libbpf.c          | 1184 +++++++++++++++++++++++++++++++++++++++
>> tools/lib/bpf/libbpf.h          |  107 ++++
>> tools/perf/MANIFEST             |    3 +
>> tools/perf/Makefile.perf        |   19 +-
>> tools/perf/builtin-probe.c      |    4 +-
>> tools/perf/builtin-record.c     |   43 +-
>> tools/perf/config/Makefile      |   19 +-
>> tools/perf/tests/Build          |    1 +
>> tools/perf/tests/builtin-test.c |    4 +
>> tools/perf/tests/llvm.c         |   85 +++
>> tools/perf/tests/make           |    4 +-
>> tools/perf/tests/tests.h        |    1 +
>> tools/perf/util/Build           |    2 +
>> tools/perf/util/bpf-loader.c    |  310 ++++++++++
>> tools/perf/util/bpf-loader.h    |   46 ++
>> tools/perf/util/config.c        |    4 +
>> tools/perf/util/debug.c         |    5 +
>> tools/perf/util/debug.h         |    1 +
>> tools/perf/util/evlist.c        |   41 ++
>> tools/perf/util/evlist.h        |    1 +
>> tools/perf/util/evsel.c         |   17 +
>> tools/perf/util/evsel.h         |    1 +
>> tools/perf/util/llvm-utils.c    |  373 ++++++++++++
>> tools/perf/util/llvm-utils.h    |   39 ++
>> tools/perf/util/parse-events.c  |   16 +
>> tools/perf/util/parse-events.h  |    2 +
>> tools/perf/util/parse-events.l  |    6 +
>> tools/perf/util/parse-events.y  |   29 +-
>> tools/perf/util/probe-event.c   |   82 +--
>> tools/perf/util/probe-event.h   |    7 +-
>> 42 files changed, 2759 insertions(+), 51 deletions(-)
>> create mode 100644 tools/build/feature/test-bpf.c
>> create mode 100644 tools/lib/bpf/.gitignore
>> create mode 100644 tools/lib/bpf/Build
>> create mode 100644 tools/lib/bpf/Makefile
>> create mode 100644 tools/lib/bpf/bpf.c
>> create mode 100644 tools/lib/bpf/bpf.h
>> create mode 100644 tools/lib/bpf/libbpf.c
>> create mode 100644 tools/lib/bpf/libbpf.h
>> create mode 100644 tools/perf/tests/llvm.c
>> create mode 100644 tools/perf/util/bpf-loader.c
>> create mode 100644 tools/perf/util/bpf-loader.h
>> create mode 100644 tools/perf/util/llvm-utils.c
>> create mode 100644 tools/perf/util/llvm-utils.h
>> 
>> -- 
>> 1.8.3.4