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

[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?
 
>  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).

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