Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751154AbbGIP6Z (ORCPT ); Thu, 9 Jul 2015 11:58:25 -0400 Received: from mail.kernel.org ([198.145.29.136]:54553 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751125AbbGIP6Q (ORCPT ); Thu, 9 Jul 2015 11:58:16 -0400 Date: Thu, 9 Jul 2015 12:58: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 02/39] bpf tools: Collect eBPF programs from their own sections Message-ID: <20150709155809.GF19430@kernel.org> References: <1436445342-1402-1-git-send-email-wangnan0@huawei.com> <1436445342-1402-3-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436445342-1402-3-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 Content-Length: 7373 Lines: 241 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. Ah, I also noticed that you provided way more comments in other patches, that really helps, keep it up! Thanks, - Arnaldo > +{ > + struct bpf_program prog, *progs; > + int nr_progs, err; > + > + err = __bpf_program__new(data, size, name, idx, &prog); > + if (err) > + return NULL; > + > + progs = obj->programs; > + nr_progs = obj->nr_programs; > + > + progs = realloc(progs, sizeof(progs[0]) * (nr_progs + 1)); > + if (!progs) { > + /* > + * In this case the original obj->programs > + * is still valid, so don't need special treat for > + * bpf_close_object(). > + */ > + pr_warning("failed to alloc a new program '%s'\n", > + name); > + bpf_program__clear(&prog); > + return NULL; > + } > + > + obj->programs = progs; > + obj->nr_programs = nr_progs + 1; > + progs[nr_progs] = prog; > + return &progs[nr_progs]; > +} > + > static struct bpf_object *bpf_object__new(const char *path, > void *obj_buf, > size_t obj_buf_sz) > @@ -342,6 +435,21 @@ static int bpf_object__elf_collect(struct bpf_object *obj) > err = -EEXIST; > } else > obj->efile.symbols = data; > + } else if ((sh.sh_type == SHT_PROGBITS) && > + (sh.sh_flags & SHF_EXECINSTR) && > + (data->d_size > 0)) { > + struct bpf_program *prog; > + > + prog = bpf_program__new(obj, data->d_buf, > + data->d_size, name, > + idx); > + if (!prog) { > + pr_warning("failed to alloc program %s (%s)", > + name, obj->path); > + err = -ENOMEM; > + } else > + pr_debug("found program %s\n", > + prog->section_name); > } > if (err) > goto out; > @@ -415,11 +523,20 @@ struct bpf_object *bpf_object__open_buffer(void *obj_buf, > > void bpf_object__close(struct bpf_object *obj) > { > + size_t i; > + > if (!obj) > return; > > bpf_object__elf_finish(obj); > > zfree(&obj->maps_buf); > + > + if (obj->programs && obj->nr_programs) { > + for (i = 0; i < obj->nr_programs; i++) > + bpf_program__clear(&obj->programs[i]); > + } > + zfree(&obj->programs); > + > free(obj); > } > -- > 1.8.3.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/