From: "Wangnan (F)" <wangnan0@huawei.com>
To: Arnaldo Carvalho de Melo <acme@kernel.org>
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
Date: Tue, 14 Jul 2015 11:58:15 +0800 [thread overview]
Message-ID: <55A488D7.1070507@huawei.com> (raw)
In-Reply-To: <20150713195109.GE2885@kernel.org>
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
next prev parent reply other threads:[~2015-07-14 4:03 UTC|newest]
Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-09 12:35 [GIT PULL 00/39] perf tools: filtering events using eBPF programs Wang Nan
2015-07-09 12:35 ` [PATCH 01/39] bpf tools: Collect symbol table from SHT_SYMTAB section Wang Nan
2015-07-09 12:35 ` [PATCH 02/39] bpf tools: Collect eBPF programs from their own sections Wang Nan
2015-07-09 15:58 ` Arnaldo Carvalho de Melo
2015-07-10 3:07 ` Wangnan (F)
2015-07-13 19:51 ` Arnaldo Carvalho de Melo
2015-07-14 3:58 ` Wangnan (F) [this message]
2015-07-09 12:35 ` [PATCH 03/39] bpf tools: Collect relocation sections from SHT_REL sections Wang Nan
2015-07-09 12:35 ` [PATCH 04/39] bpf tools: Record map accessing instructions for each program Wang Nan
2015-07-09 12:35 ` [PATCH 05/39] bpf tools: Add bpf.c/h for common bpf operations Wang Nan
2015-07-09 12:35 ` [PATCH 06/39] bpf tools: Create eBPF maps defined in an object file Wang Nan
2015-07-13 19:54 ` Arnaldo Carvalho de Melo
2015-07-14 3:59 ` Wangnan (F)
2015-07-09 12:35 ` [PATCH 07/39] bpf tools: Relocate eBPF programs Wang Nan
2015-07-09 12:35 ` [PATCH 08/39] bpf tools: Introduce bpf_load_program() to bpf.c Wang Nan
2015-07-09 12:35 ` [PATCH 09/39] bpf tools: Load eBPF programs in object files into kernel Wang Nan
2015-07-09 12:35 ` [PATCH 10/39] bpf tools: Introduce accessors for struct bpf_program Wang Nan
2015-07-09 12:35 ` [PATCH 11/39] bpf tools: Link all bpf objects onto a list Wang Nan
2015-07-09 12:35 ` [PATCH 12/39] perf tools: Introduce llvm config options Wang Nan
2015-07-21 11:13 ` [PATCH 12/39 update] " Wang Nan
2015-08-08 8:16 ` [tip:perf/core] " tip-bot for Wang Nan
2015-07-09 12:35 ` [PATCH 13/39] perf tools: Call clang to compile C source to object code Wang Nan
2015-07-09 12:35 ` [PATCH 14/39] perf tools: Auto detecting kernel build directory Wang Nan
2015-07-13 21:46 ` Arnaldo Carvalho de Melo
2015-07-14 6:56 ` Wangnan (F)
2015-07-14 7:16 ` Wangnan (F)
2015-07-09 12:35 ` [PATCH 15/39] perf tools: Auto detecting kernel include options Wang Nan
2015-08-08 8:17 ` [tip:perf/core] " tip-bot for Wang Nan
2015-07-09 12:35 ` [PATCH 16/39] perf tests: Add LLVM test for eBPF on-the-fly compiling Wang Nan
2015-07-15 10:54 ` [PATCH updated " Wang Nan
2015-07-22 4:46 ` [PATCH 16/39 RESEND] " Wang Nan
2015-07-09 12:35 ` [PATCH 17/39] perf tools: Make perf depend on libbpf Wang Nan
2015-07-09 12:35 ` [PATCH 18/39] perf record: Enable passing bpf object file to --event Wang Nan
2015-07-09 12:35 ` [PATCH 19/39] perf record: Compile scriptlets if pass '.c' " Wang Nan
2015-07-09 12:35 ` [PATCH 20/39] perf tools: Parse probe points of eBPF programs during preparation Wang Nan
2015-07-09 12:35 ` [PATCH 21/39] perf probe: Attach trace_probe_event with perf_probe_event Wang Nan
2015-07-09 12:35 ` [PATCH 22/39] perf record: Probe at kprobe points Wang Nan
2015-07-09 12:35 ` [PATCH 23/39] perf record: Load all eBPF object into kernel Wang Nan
2015-07-09 12:35 ` [PATCH 24/39] perf tools: Add bpf_fd field to evsel and config it Wang Nan
2015-07-09 12:35 ` [PATCH 25/39] perf tools: Attach eBPF program to perf event Wang Nan
2015-07-09 12:35 ` [PATCH 26/39] perf tools: Suppress probing messages when probing by BPF loading Wang Nan
2015-07-09 12:35 ` [PATCH 27/39] perf record: Add clang options for compiling BPF scripts Wang Nan
2015-07-09 12:35 ` [PATCH 28/39] bpf tools: Load a program with different instances using preprocessor Wang Nan
2015-07-13 5:44 ` Wangnan (F)
2015-07-09 12:35 ` [PATCH 29/39] perf tools: Fix probe-event.h include Wang Nan
2015-07-09 12:35 ` [PATCH 30/39] perf probe: Reset args and nargs for probe_trace_event when failure Wang Nan
2015-07-09 12:35 ` [PATCH 31/39] perf tools: Move linux/filter.h to tools/include Wang Nan
2015-07-09 12:35 ` [PATCH 32/39] perf tools: Add BPF_PROLOGUE config options for further patches Wang Nan
2015-07-09 12:35 ` [PATCH 33/39] perf tools: Introduce arch_get_reg_info() for x86 Wang Nan
2015-07-09 12:35 ` [PATCH 34/39] perf tools: Add prologue for BPF programs for fetching arguments Wang Nan
2015-07-09 12:35 ` [PATCH 35/39] perf tools: Generate prologue for BPF programs Wang Nan
2015-07-09 12:35 ` [PATCH 36/39] perf tools: Use same BPF program if arguments are identical Wang Nan
2015-07-09 12:35 ` [PATCH 37/39] perf record: Support custom vmlinux path Wang Nan
2015-07-09 12:35 ` [PATCH 38/39] perf probe: Init symbol as kprobe if any event is kprobe Wang Nan
2015-07-09 12:35 ` [PATCH 39/39] perf tools: Support attach BPF program on uprobe events Wang Nan
2015-07-14 15:36 ` perf test LLVM was: Re: [GIT PULL 00/39] perf tools: filtering events using eBPF programs Arnaldo Carvalho de Melo
2015-07-15 10:49 ` Wangnan (F)
2015-07-15 11:20 ` Arnaldo Carvalho de Melo
2015-07-17 2:34 ` Wangnan (F)
[not found] ` <CA+JHD93=ZMFP0gB1NqTXGBjTfSyYC-53Uj7r11gQJnKtQRKcDg@mail.gmail.com>
2015-07-21 11:09 ` Wangnan (F)
2015-07-21 11:41 ` Arnaldo Carvalho de Melo
2015-07-22 4:40 ` Wangnan (F)
2015-07-31 15:35 ` perf eBPF patch ordering. was: " Arnaldo Carvalho de Melo
2015-07-31 20:31 ` Arnaldo Carvalho de Melo
2015-08-03 2:37 ` Wangnan (F)
2015-08-03 15:07 ` Arnaldo Carvalho de Melo
2015-08-03 15:19 ` Arnaldo Carvalho de Melo
2015-08-03 15:53 ` pi3orama
2015-08-03 16:11 ` Arnaldo Carvalho de Melo
2015-08-03 19:49 ` Arnaldo Carvalho de Melo
2015-08-04 5:28 ` Wangnan (F)
2015-08-04 10:39 ` Wangnan (F)
2015-08-04 15:55 ` Arnaldo Carvalho de Melo
2015-08-04 16:11 ` Arnaldo Carvalho de Melo
2015-08-04 16:13 ` pi3orama
2015-08-06 1:44 ` Wangnan (F)
2015-08-06 14:46 ` Arnaldo Carvalho de Melo
2015-08-06 15:58 ` Arnaldo Carvalho de Melo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=55A488D7.1070507@huawei.com \
--to=wangnan0@huawei.com \
--cc=acme@kernel.org \
--cc=ast@plumgrid.com \
--cc=hekuang@huawei.com \
--cc=linux-kernel@vger.kernel.org \
--cc=lizefan@huawei.com \
--cc=pi3orama@163.com \
--cc=xiakaixu@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.