From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752067AbbGJDQ5 (ORCPT ); Thu, 9 Jul 2015 23:16:57 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:36959 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751293AbbGJDQt (ORCPT ); Thu, 9 Jul 2015 23:16:49 -0400 Message-ID: <559F3709.2080302@huawei.com> Date: Fri, 10 Jul 2015 11:07:53 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Arnaldo Carvalho de Melo CC: , , , , , Subject: Re: [PATCH 02/39] bpf tools: Collect eBPF programs from their own sections References: <1436445342-1402-1-git-send-email-wangnan0@huawei.com> <1436445342-1402-3-git-send-email-wangnan0@huawei.com> <20150709155809.GF19430@kernel.org> In-Reply-To: <20150709155809.GF19430@kernel.org> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 And I also resolved a confliction in the next patch. I think you can comment on current patchset since the modification is too small to send again. And following is new pull request: The following changes since commit 3381a29cbec5447086c0f726ee9a88c02e60becc: bpf tools: Collect map definitions from 'maps' section (2015-07-07 13:41:45 -0300) are available in the git repository at: https://github.com/WangNan0/linux.git perf/ebpf-for-acme for you to fetch changes up to ecc453e27f60ff59e0c2d1cdf64fb595b72d2f68: perf tools: Support attach BPF program on uprobe events (2015-07-10 02:52:57 +0000) Thank you.