Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751633AbbGNEDK (ORCPT ); Tue, 14 Jul 2015 00:03:10 -0400 Received: from szxga03-in.huawei.com ([119.145.14.66]:8391 "EHLO szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751557AbbGNEDH (ORCPT ); Tue, 14 Jul 2015 00:03:07 -0400 Message-ID: <55A488D7.1070507@huawei.com> Date: Tue, 14 Jul 2015 11:58:15 +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> <559F3709.2080302@huawei.com> <20150713195109.GE2885@kernel.org> In-Reply-To: <20150713195109.GE2885@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 X-Mirapoint-Virus-RAPID-Raw: score=unknown(0), refid=str=0001.0A020205.55A489EE.0183,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0, ip=0.0.0.0, so=2013-05-26 15:14:31, dmn=2013-03-21 17:37:32 X-Mirapoint-Loop-Id: 9c84e9560b719a842c11c3568e552aa8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6645 Lines: 175 On 2015/7/14 3:51, Arnaldo Carvalho de Melo wrote: > 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. I saw your modification ann it looks good to me. I'll collect it into my patchset. Thank you. > - Arnaldo -- 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/