Re: [PATCH 02/39] bpf tools: Collect eBPF programs from their own sections

["Wangnan (F)" <wangnan0@huawei.com>]

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

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.