Message-ID: <20190318163848.GE22548@kernel.org>
Date:   Mon, 18 Mar 2019 13:38:48 -0300
From:   Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
To:     Song Liu <songliubraving@...com>
Cc:     bpf@...r.kernel.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        kernel-team@...com, peterz@...radead.org, acme@...hat.com,
        jolsa@...nel.org, namhyung@...nel.org, sdf@...ichev.me
Subject: Re: [PATCH v9 perf,bpf 12/15] perf, bpf: enable annotation of bpf
 program

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@...com>
> ---
>  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?

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.

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

Powered by blists - more mailing lists