Re: [PATCH v9 perf,bpf 12/15] perf, bpf: enable annotation of bpf program

From: Song Liu
Date: Tue Mar 19 2019 - 02:06:13 EST

Next message: Kuninori Morimoto: "Re: [PATCH v3 0/3] rsnd: dts: change to use extended audio dmac register"
Previous message: Pankaj Gupta: "[PATCH] virtio_console: initialize vtermno value for ports"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH v9 perf,bpf 12/15] perf, bpf: enable annotation of bpf program"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v9 perf,bpf 12/15] perf, bpf: enable annotation of bpf program"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Sorry for the late response.

> On Mar 18, 2019, at 9:38 AM, Arnaldo Carvalho de Melo <arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Mon, Mar 11, 2019 at 10:30:48PM -0700, Song Liu escreveu:
>> This patch enables the annotation of bpf program.
>>
>> A new dso type DSO_BINARY_TYPE__BPF_PROG_INFO is introduced to for BPF
>> programs. In symbol__disassemble(), DSO_BINARY_TYPE__BPF_PROG_INFO dso
>> calls into a new function symbol__disassemble_bpf(), where annotation
>> line information is filled based bpf_prog_info and btf saved in given
>> perf_env.
>>
>> symbol__disassemble_bpf() uses libbfd to disassemble bpf programs.
>>
>> Signed-off-by: Song Liu <songliubraving@xxxxxx>
>> ---
>> tools/build/Makefile.feature | 6 +-
>> tools/perf/Makefile.config | 4 +
>
> I see the changes to these two files, but I can't see the feature test
> that it triggers, forgot to do a git-add? Or is it in an upcoming patch
> in this series?

test-disassembler-four-args.c is an existing test used by bpftool.
It was added in fb982666e380c1632a74495b68b3c33a66e76430 .

>
> After applying this test it "works":
>
> make: Entering directory '/home/acme/git/perf/tools/perf'
> BUILD: Doing 'make -j8' parallel build
>
> Auto-detecting system features:
> ... dwarf: [ on ]
> ... dwarf_getlocations: [ on ]
> <SNIP>
> ... bpf: [ on ]
> ... libaio: [ on ]
> ... disassembler-four-args: [ on ]
>
> Because you added it to FEATURE_TESTS_BASIC, and this means that if
> tools/build/feature/test-all.c builds, then what is in
> FEATURE_TESTS_BASIC is set to 'on', but you didn't add anything to
> tools/build/feature/test-all.c, so it works because all the other
> features are present.
>
> Take a look at:
>
> 2a07d814747b ("tools build feature: Check if libaio is available")
>
> To see what needs to be done.

I used different versions of gcc to verify that current version of the
patch works for both test-succeed and test-fail cases. I guess something
like the following is needed? But the test does work without it.

Thanks,
Song

diff --git i/tools/build/feature/test-all.c w/tools/build/feature/test-all.c
index e903b86b742f..7853e6d91090 100644
--- i/tools/build/feature/test-all.c
+++ w/tools/build/feature/test-all.c
@@ -178,6 +178,10 @@
# include "test-reallocarray.c"
#undef main

+#define main main_test_disassembler_four_args
+# include "test-disassembler-four-args.c"
+#undef main
+
int main(int argc, char *argv[])
{
main_test_libpython();
@@ -219,6 +223,7 @@ int main(int argc, char *argv[])
main_test_setns();
main_test_libaio();
main_test_reallocarray();
+ main_test_disassembler_four_args();

return 0;
}

>
> Either way, its interesting to break this patch in two, one adding the
> feature test and another to use it, i.e. the second patch out of this
> should have the commit log message in this patch.
>
> I've applied the patches up to before this one and will continue
> processing other patches while you address this.
>
> Thanks,
>
> - Arnaldo
>
>> tools/perf/util/annotate.c | 150 ++++++++++++++++++++++++++++++++++-
>> tools/perf/util/dso.c | 1 +
>> tools/perf/util/dso.h | 32 +++++---
>> tools/perf/util/symbol.c | 1 +
>> 6 files changed, 180 insertions(+), 14 deletions(-)
>>
>> diff --git a/tools/build/Makefile.feature b/tools/build/Makefile.feature
>> index 61e46d54a67c..8d3864b061f3 100644
>> --- a/tools/build/Makefile.feature
>> +++ b/tools/build/Makefile.feature
>> @@ -66,7 +66,8 @@ FEATURE_TESTS_BASIC := \
>> sched_getcpu \
>> sdt \
>> setns \
>> - libaio
>> + libaio \
>> + disassembler-four-args
>>
>> # FEATURE_TESTS_BASIC + FEATURE_TESTS_EXTRA is the complete list
>> # of all feature tests
>> @@ -118,7 +119,8 @@ FEATURE_DISPLAY ?= \
>> lzma \
>> get_cpuid \
>> bpf \
>> - libaio
>> + libaio \
>> + disassembler-four-args
>>
>> # Set FEATURE_CHECK_(C|LD)FLAGS-all for all FEATURE_TESTS features.
>> # If in the future we need per-feature checks/flags for features not
>> diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
>> index df4ad45599ca..c51b59e43dcc 100644
>> --- a/tools/perf/Makefile.config
>> +++ b/tools/perf/Makefile.config
>> @@ -808,6 +808,10 @@ ifdef HAVE_KVM_STAT_SUPPORT
>> CFLAGS += -DHAVE_KVM_STAT_SUPPORT
>> endif
>>
>> +ifeq ($(feature-disassembler-four-args), 1)
>> + CFLAGS += -DDISASM_FOUR_ARGS_SIGNATURE
>> +endif
>> +
>> ifeq (${IS_64_BIT}, 1)
>> ifndef NO_PERF_READ_VDSO32
>> $(call feature_check,compile-32)
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 5f6dbbf5d749..e492b19a157c 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -10,6 +10,10 @@
>> #include <errno.h>
>> #include <inttypes.h>
>> #include <libgen.h>
>> +#include <bpf/bpf.h>
>> +#include <bpf/btf.h>
>> +#include <bpf/libbpf.h>
>> +#include <linux/btf.h>
>> #include "util.h"
>> #include "ui/ui.h"
>> #include "sort.h"
>> @@ -24,6 +28,7 @@
>> #include "annotate.h"
>> #include "evsel.h"
>> #include "evlist.h"
>> +#include "bpf-event.h"
>> #include "block-range.h"
>> #include "string2.h"
>> #include "arch/common.h"
>> @@ -31,6 +36,9 @@
>> #include <pthread.h>
>> #include <linux/bitops.h>
>> #include <linux/kernel.h>
>> +#include <bfd.h>
>> +#include <dis-asm.h>
>> +#include <bpf/libbpf.h>
>>
>> /* FIXME: For the HE_COLORSET */
>> #include "ui/browser.h"
>> @@ -1674,6 +1682,144 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil
>> return 0;
>> }
>>
>> +static int symbol__disassemble_bpf(struct symbol *sym,
>> + struct annotate_args *args)
>> +{
>> + struct annotation *notes = symbol__annotation(sym);
>> + struct annotation_options *opts = args->options;
>> + struct bpf_prog_info_linear *info_linear;
>> + struct bpf_prog_linfo *prog_linfo = NULL;
>> + struct bpf_prog_info_node *info_node;
>> + int len = sym->end - sym->start;
>> + disassembler_ftype disassemble;
>> + struct map *map = args->ms.map;
>> + struct disassemble_info info;
>> + struct dso *dso = map->dso;
>> + int pc = 0, count, sub_id;
>> + struct btf *btf = NULL;
>> + char tpath[PATH_MAX];
>> + size_t buf_size;
>> + int nr_skip = 0;
>> + int ret = -1;
>> + char *buf;
>> + bfd *bfdf;
>> + FILE *s;
>> +
>> + if (dso->binary_type != DSO_BINARY_TYPE__BPF_PROG_INFO)
>> + return -1;
>> +
>> + pr_debug("%s: handling sym %s addr %lx len %lx\n", __func__,
>> + sym->name, sym->start, sym->end - sym->start);
>> +
>> + memset(tpath, 0, sizeof(tpath));
>> + perf_exe(tpath, sizeof(tpath));
>> +
>> + bfdf = bfd_openr(tpath, NULL);
>> + assert(bfdf);
>> + assert(bfd_check_format(bfdf, bfd_object));
>> +
>> + s = open_memstream(&buf, &buf_size);
>> + if (!s)
>> + goto out;
>> + init_disassemble_info(&info, s,
>> + (fprintf_ftype) fprintf);
>> +
>> + info.arch = bfd_get_arch(bfdf);
>> + info.mach = bfd_get_mach(bfdf);
>> +
>> + info_node = perf_env__find_bpf_prog_info(dso->bpf_prog.env,
>> + dso->bpf_prog.id);
>> + if (!info_node)
>> + goto out;
>> + info_linear = info_node->info_linear;
>> + sub_id = dso->bpf_prog.sub_id;
>> +
>> + info.buffer = (void *)(info_linear->info.jited_prog_insns);
>> + info.buffer_length = info_linear->info.jited_prog_len;
>> +
>> + if (info_linear->info.nr_line_info)
>> + prog_linfo = bpf_prog_linfo__new(&info_linear->info);
>> +
>> + if (info_linear->info.btf_id) {
>> + struct btf_node *node;
>> +
>> + node = perf_env__find_btf(dso->bpf_prog.env,
>> + info_linear->info.btf_id);
>> + if (node)
>> + btf = btf__new((__u8 *)(node->data),
>> + node->data_size);
>> + }
>> +
>> + disassemble_init_for_target(&info);
>> +
>> +#ifdef DISASM_FOUR_ARGS_SIGNATURE
>> + disassemble = disassembler(info.arch,
>> + bfd_big_endian(bfdf),
>> + info.mach,
>> + bfdf);
>> +#else
>> + disassemble = disassembler(bfdf);
>> +#endif
>> + assert(disassemble);
>> +
>> + fflush(s);
>> + do {
>> + const struct bpf_line_info *linfo = NULL;
>> + struct disasm_line *dl;
>> + size_t prev_buf_size;
>> + const char *srcline;
>> + u64 addr;
>> +
>> + addr = pc + ((u64 *)(info_linear->info.jited_ksyms))[sub_id];
>> + count = disassemble(pc, &info);
>> +
>> + if (prog_linfo)
>> + linfo = bpf_prog_linfo__lfind_addr_func(prog_linfo,
>> + addr, sub_id,
>> + nr_skip);
>> +
>> + if (linfo && btf) {
>> + srcline = btf__name_by_offset(btf, linfo->line_off);
>> + nr_skip++;
>> + } else
>> + srcline = NULL;
>> +
>> + fprintf(s, "\n");
>> + prev_buf_size = buf_size;
>> + fflush(s);
>> +
>> + if (!opts->hide_src_code && srcline) {
>> + args->offset = -1;
>> + args->line = strdup(srcline);
>> + args->line_nr = 0;
>> + args->ms.sym = sym;
>> + dl = disasm_line__new(args);
>> + if (dl) {
>> + annotation_line__add(&dl->al,
>> + &notes->src->source);
>> + }
>> + }
>> +
>> + args->offset = pc;
>> + args->line = buf + prev_buf_size;
>> + args->line_nr = 0;
>> + args->ms.sym = sym;
>> + dl = disasm_line__new(args);
>> + if (dl)
>> + annotation_line__add(&dl->al, &notes->src->source);
>> +
>> + pc += count;
>> + } while (count > 0 && pc < len);
>> +
>> + ret = 0;
>> +out:
>> + free(prog_linfo);
>> + free(btf);
>> + fclose(s);
>> + bfd_close(bfdf);
>> + return ret;
>> +}
>> +
>> static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
>> {
>> struct annotation_options *opts = args->options;
>> @@ -1701,7 +1847,9 @@ static int symbol__disassemble(struct symbol *sym, struct annotate_args *args)
>> pr_debug("annotating [%p] %30s : [%p] %30s\n",
>> dso, dso->long_name, sym, sym->name);
>>
>> - if (dso__is_kcore(dso)) {
>> + if (dso->binary_type == DSO_BINARY_TYPE__BPF_PROG_INFO) {
>> + return symbol__disassemble_bpf(sym, args);
>> + } else if (dso__is_kcore(dso)) {
>> kce.kcore_filename = symfs_filename;
>> kce.addr = map__rip_2objdump(map, sym->start);
>> kce.offs = sym->start;
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index ba58ba603b69..58547c468c65 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -184,6 +184,7 @@ int dso__read_binary_type_filename(const struct dso *dso,
>> case DSO_BINARY_TYPE__KALLSYMS:
>> case DSO_BINARY_TYPE__GUEST_KALLSYMS:
>> case DSO_BINARY_TYPE__JAVA_JIT:
>> + case DSO_BINARY_TYPE__BPF_PROG_INFO:
>> case DSO_BINARY_TYPE__NOT_FOUND:
>> ret = -1;
>> break;
>> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
>> index bb417c54c25a..c274b5aa839d 100644
>> --- a/tools/perf/util/dso.h
>> +++ b/tools/perf/util/dso.h
>> @@ -14,6 +14,7 @@
>>
>> struct machine;
>> struct map;
>> +struct perf_env;
>>
>> enum dso_binary_type {
>> DSO_BINARY_TYPE__KALLSYMS = 0,
>> @@ -35,6 +36,7 @@ enum dso_binary_type {
>> DSO_BINARY_TYPE__KCORE,
>> DSO_BINARY_TYPE__GUEST_KCORE,
>> DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO,
>> + DSO_BINARY_TYPE__BPF_PROG_INFO,
>> DSO_BINARY_TYPE__NOT_FOUND,
>> };
>>
>> @@ -178,17 +180,25 @@ struct dso {
>> struct auxtrace_cache *auxtrace_cache;
>> int comp;
>>
>> - /* dso data file */
>> - struct {
>> - struct rb_root cache;
>> - int fd;
>> - int status;
>> - u32 status_seen;
>> - size_t file_size;
>> - struct list_head open_entry;
>> - u64 debug_frame_offset;
>> - u64 eh_frame_hdr_offset;
>> - } data;
>> + union {
>> + /* dso data file */
>> + struct {
>> + struct rb_root cache;
>> + int fd;
>> + int status;
>> + u32 status_seen;
>> + size_t file_size;
>> + struct list_head open_entry;
>> + u64 debug_frame_offset;
>> + u64 eh_frame_hdr_offset;
>> + } data;
>> + /* bpf prog information */
>> + struct {
>> + u32 id;
>> + u32 sub_id;
>> + struct perf_env *env;
>> + } bpf_prog;
>> + };
>>
>> union { /* Tool specific area */
>> void *priv;
>> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
>> index 758bf5f74e6e..4e2e304d4037 100644
>> --- a/tools/perf/util/symbol.c
>> +++ b/tools/perf/util/symbol.c
>> @@ -1451,6 +1451,7 @@ static bool dso__is_compatible_symtab_type(struct dso *dso, bool kmod,
>> case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:
>> return true;
>>
>> + case DSO_BINARY_TYPE__BPF_PROG_INFO:
>> case DSO_BINARY_TYPE__NOT_FOUND:
>> default:
>> return false;
>> --
>> 2.17.1
>
> --
>
> - Arnaldo

Next message: Kuninori Morimoto: "Re: [PATCH v3 0/3] rsnd: dts: change to use extended audio dmac register"
Previous message: Pankaj Gupta: "[PATCH] virtio_console: initialize vtermno value for ports"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH v9 perf,bpf 12/15] perf, bpf: enable annotation of bpf program"
Next in thread: Arnaldo Carvalho de Melo: "Re: [PATCH v9 perf,bpf 12/15] perf, bpf: enable annotation of bpf program"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]