From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752666AbbGMTvc (ORCPT ); Mon, 13 Jul 2015 15:51:32 -0400 Received: from [198.145.29.136] ([198.145.29.136]:37625 "EHLO mail.kernel.org" rhost-flags-FAIL-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1752561AbbGMTvb (ORCPT ); Mon, 13 Jul 2015 15:51:31 -0400 Date: Mon, 13 Jul 2015 16:51:09 -0300 From: Arnaldo Carvalho de Melo To: "Wangnan (F)" Cc: ast@plumgrid.com, linux-kernel@vger.kernel.org, lizefan@huawei.com, hekuang@huawei.com, xiakaixu@huawei.com, pi3orama@163.com Subject: Re: [PATCH 02/39] bpf tools: Collect eBPF programs from their own sections Message-ID: <20150713195109.GE2885@kernel.org> References: <1436445342-1402-1-git-send-email-wangnan0@huawei.com> <1436445342-1402-3-git-send-email-wangnan0@huawei.com> <20150709155809.GF19430@kernel.org> <559F3709.2080302@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <559F3709.2080302@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 Fri, Jul 10, 2015 at 11:07:53AM +0800, Wangnan (F) escreveu: > On 2015/7/9 23:58, Arnaldo Carvalho de Melo wrote: > >Em Thu, Jul 09, 2015 at 12:35:05PM +0000, Wang Nan escreveu: > >>This patch collects all programs in an object file into an array of > >>'struct bpf_program' for further processing. That structure is for > >>representing each eBPF program. 'bpf_prog' should be a better name, but > >>it has been used by linux/filter.h. Although it is a kernel space name, > >>I still prefer to call it 'bpf_program' to prevent possible confusion. > >> > >>bpf_program__new() creates a new 'struct bpf_program' object. It first > >>init a variable in stack using __bpf_program__new(), then if success, > >>enlarges obj->programs array and copy the new object in. > >> > >>Signed-off-by: Wang Nan > >>Acked-by: Alexei Starovoitov > >>Cc: Brendan Gregg > >>Cc: Daniel Borkmann > >>Cc: David Ahern > >>Cc: He Kuang > >>Cc: Jiri Olsa > >>Cc: Kaixu Xia > >>Cc: Masami Hiramatsu > >>Cc: Namhyung Kim > >>Cc: Paul Mackerras > >>Cc: Peter Zijlstra > >>Cc: Zefan Li > >>Cc: pi3orama@163.com > >>Link: http://lkml.kernel.org/r/1435716878-189507-13-git-send-email-wangnan0@huawei.com > >>Signed-off-by: Arnaldo Carvalho de Melo > >>--- > >> tools/lib/bpf/libbpf.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 117 insertions(+) > >> > >>diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >>index 9b016c0..3b717de 100644 > >>--- a/tools/lib/bpf/libbpf.c > >>+++ b/tools/lib/bpf/libbpf.c > >>@@ -78,12 +78,27 @@ void libbpf_set_print(libbpf_print_fn_t warn, > >> # define LIBBPF_ELF_C_READ_MMAP ELF_C_READ > >> #endif > >>+/* > >>+ * bpf_prog should be a better name but it has been used in > >>+ * linux/filter.h. > >>+ */ > >>+struct bpf_program { > >>+ /* Index in elf obj file, for relocation use. */ > >>+ int idx; > >>+ char *section_name; > >>+ struct bpf_insn *insns; > >>+ size_t insns_cnt; > >>+}; > >>+ > >> struct bpf_object { > >> char license[64]; > >> u32 kern_version; > >> void *maps_buf; > >> size_t maps_buf_sz; > >>+ struct bpf_program *programs; > >>+ size_t nr_programs; > >>+ > >> /* > >> * Information when doing elf related work. Only valid if fd > >> * is valid. > >>@@ -100,6 +115,84 @@ struct bpf_object { > >> }; > >> #define obj_elf_valid(o) ((o)->efile.elf) > >>+static void bpf_program__clear(struct bpf_program *prog) > >>+{ > >>+ if (!prog) > >>+ return; > >>+ > >>+ zfree(&prog->section_name); > >>+ zfree(&prog->insns); > >>+ prog->insns_cnt = 0; > >>+ prog->idx = -1; > >>+} > >So in perf land we use 'bpf_program__exit()' as the counterpart of > >bpf_program__init(), i.e. one just initializes fields, allocating > >memory for 'struct bpf_program' members, but does not allocates the > >struct bpf_program itself, because sometimes we embed it inside other > >structs, or we have it in arrays, as you do. > > > >So, to keep that convention, please rename bpf_program__clear() to > >bpf_program__exit() and the next function, __bpf_program__new() to > >bpf_program__init(), with 'struct bpf_program *prog' as the first > >parameter. > > > >To speed things up, from now on, when I see such stuff, I will do the > >changes, put them in a branch with a commiter note, and wait for your > >Ack (or not, if you disagree with something). > > > >One more comment below. > > > >>+ > >>+static int > >>+__bpf_program__new(void *data, size_t size, char *name, int idx, > >>+ struct bpf_program *prog) > >>+{ > >>+ if (size < sizeof(struct bpf_insn)) { > >>+ pr_warning("corrupted section '%s'\n", name); > >>+ return -EINVAL; > >>+ } > >>+ > >>+ bzero(prog, sizeof(*prog)); > >>+ > >>+ prog->section_name = strdup(name); > >>+ if (!prog->section_name) { > >>+ pr_warning("failed to alloc name for prog %s\n", > >>+ name); > >>+ goto errout; > >>+ } > >>+ > >>+ prog->insns = malloc(size); > >>+ if (!prog->insns) { > >>+ pr_warning("failed to alloc insns for %s\n", name); > >>+ goto errout; > >>+ } > >>+ prog->insns_cnt = size / sizeof(struct bpf_insn); > >>+ memcpy(prog->insns, data, > >>+ prog->insns_cnt * sizeof(struct bpf_insn)); > >>+ prog->idx = idx; > >>+ > >>+ return 0; > >>+errout: > >>+ bpf_program__clear(prog); > >>+ return -ENOMEM; > >>+} > >>+ > >>+static struct bpf_program * > >>+bpf_program__new(struct bpf_object *obj, void *data, size_t size, > >>+ char *name, int idx) > >This, as well, is not a 'bpf_program' method, it is a 'struct > >bpf_object' one, that will manipulate 'struct bpf_object' internal > >state, changing its struct members to get space for an extra bpf_program > >that was initialized on the stack, if the initialization of such > >bpf_program went well, or bail out otherwise. > > > >So I suggest you rename this to: > > > >int bpf_object__add_program(struct bpf_object *obj, void *data, size_t size, char *name, int idx) > > > >And probably move that debug that uses prog->section_name to just after > >the realloc, here in this function. > > > >I will look at the other patches after lunch, thanks for providing the > >git tree, I will try and use it before looking at the patches > >individually, to get a feel of the whole thing. > > I didn't find your code, so I updated my git tree. Please see: > > https://github.com/WangNan0/linux/commit/e5ffa4f070ee36cce5130d08622dc305ad9cdb31 Ok, so used bpf_object__add_program, but you still return a bpf_program pointer, that you do not use for anything, i.e. the failure of bpf_object__add_program is reported only via a NULL return and you then assume this was because ENOMEM was the reason, when there are multiple errors that can cause bpf_object__add_program to fail. Noted that with a comment on that patch, checked that no later patches use that return, etc. - Arnaldo