From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934662AbbGHODV (ORCPT ); Wed, 8 Jul 2015 10:03:21 -0400 Received: from mail.kernel.org ([198.145.29.136]:53206 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932599AbbGHODT (ORCPT ); Wed, 8 Jul 2015 10:03:19 -0400 Date: Wed, 8 Jul 2015 11:03:09 -0300 From: Arnaldo Carvalho de Melo To: Wang Nan Cc: ast@plumgrid.com, linux-kernel@vger.kernel.org, lizefan@huawei.com, hekuang@huawei.com, xiakaixu@huawei.com, pi3orama@163.com Subject: Re: [PATCH v11 00/39] perf tools: filtering events using eBPF programs - part1 Message-ID: <20150708140309.GA31332@kernel.org> References: <1436361268-234530-1-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436361268-234530-1-git-send-email-wangnan0@huawei.com> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.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