lkml.org 
[lkml]   [2015]   [Jul]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 02/39] bpf tools: Collect eBPF programs from their own sections


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 <wangnan0@huawei.com>
>>>> Acked-by: Alexei Starovoitov <ast@plumgrid.com>
>>>> Cc: Brendan Gregg <brendan.d.gregg@gmail.com>
>>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>>> Cc: David Ahern <dsahern@gmail.com>
>>>> Cc: He Kuang <hekuang@huawei.com>
>>>> Cc: Jiri Olsa <jolsa@kernel.org>
>>>> Cc: Kaixu Xia <xiakaixu@huawei.com>
>>>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>>>> Cc: Namhyung Kim <namhyung@kernel.org>
>>>> Cc: Paul Mackerras <paulus@samba.org>
>>>> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
>>>> Cc: Zefan Li <lizefan@huawei.com>
>>>> 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 <acme@redhat.com>
>>>> ---
>>>> 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




\
 
 \ /
  Last update: 2015-07-14 06:21    [W:0.108 / U:0.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site