perf/core improvements and fixes

Hi Ingo,

Please consider pulling,

- Arnaldo

The following changes since commit e7a7865cc0da306542db0b9205cb0a467f59e33d:

perf symbols: Fix dso__load_sym to put dso (2015-12-10 16:29:32 -0300)

are available in the git repository at:

git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git tags/perf-core-for-mingo

for you to fetch changes up to 93b0ba3c60da89043ce2b9f601cd2b3da408903b:

perf tools: Clear struct machine during machine__init() (2015-12-11 09:32:41 -0300)

----------------------------------------------------------------
perf/core improvements and fixes:

User visible:

- Fix 'perf top' annotation in --stdio (Namhyung Kim)

- Support hw breakpoint events (mem:0xAddress) in the default output mode in 'perf script' (Wang Nan)

Infrastructure:

- Do not hold the hists lock while emitting one specific warning (Namhyung Kim)

- Fetch map names from correct strtab, worked so far because llvm/clang uses just one string table (Wang Nan)

----------------------------------------------------------------
Namhyung Kim (4):
perf top: Do not convert address for perf_top__record_precise_ip() perf top: Access hists->lock only if needed
perf top: Fix annotation on --stdio
perf top: Cleanup condition in perf_top__record_precise_ip()

Wang Nan (5):
tools lib bpf: Check return value of strdup when reading map names tools lib bpf: Fetch map names from correct strtab perf data: Add u32_hex data type
perf script: Add support for PERF_TYPE_BREAKPOINT
perf tools: Clear struct machine during machine__init()

 tools/lib/bpf/libbpf.c            | 24 +++++++++++++-----
 tools/perf/builtin-script.c       | 14 +++++++++++
 tools/perf/builtin-top.c          | 52 +++++++++++++++++----------------------
 tools/perf/util/data-convert-bt.c |  2 ++
 tools/perf/util/machine.c         |  1 +
 5 files changed, 57 insertions(+), 36 deletions(-)

From: Namhyung Kim <[email protected]>

We call map->unmap_ip() before the function and call map->map_ip() inside the function. This is meaningless and look strange since only one of the two checks 'map'. Let's use al->addr directly.

---
 tools/perf/builtin-top.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

index 785aa2dd8f0b..3b0978e5578a 100644
@@ -189,8 +189,6 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 	if (pthread_mutex_trylock(&notes->lock))
 		return;
 
-	ip = he->ms.map->map_ip(he->ms.map, ip);
-
 	if (ui__has_annotation())
 		err = hist_entry__inc_addr_samples(he, counter, ip);
 

@@ -687,14 +685,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
 	struct hist_entry *he = iter->he;
 	struct perf_evsel *evsel = iter->evsel;
 
-	if (sort__has_sym && single) {
-		u64 ip = al->addr;
-
-		if (al->map)
-			ip = al->map->unmap_ip(al->map, ip);
-
-		perf_top__record_precise_ip(top, he, evsel->idx, ip);
-	}
+	if (sort__has_sym && single)
+		perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
 
 	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
 		     !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));

2.1.0

From: Namhyung Kim <[email protected]>

The perf_top__record_precise_ip() releases and regrabs the he->hists->lock because it can sleep if there's an error. But it should be done conditionally as it slows down the fast path.

---
 tools/perf/builtin-top.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

index 3b0978e5578a..586798acf7db 100644
@@ -194,21 +194,23 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 
 	pthread_mutex_unlock(&notes->lock);
 
-	/*
-	 * This function is now called with he->hists->lock held.
-	 * Release it before going to sleep.
-	 */
-	pthread_mutex_unlock(&he->hists->lock);
+	if (unlikely(err)) {
+		/*
+		 * This function is now called with he->hists->lock held.
+		 * Release it before going to sleep.
+		 */
+		pthread_mutex_unlock(&he->hists->lock);
+
+		if (err == -ERANGE && !he->ms.map->erange_warned)
+			ui__warn_map_erange(he->ms.map, sym, ip);
+		else if (err == -ENOMEM) {
+			pr_err("Not enough memory for annotating '%s' symbol!\n",
+			       sym->name);
+			sleep(1);
+		}
 
-	if (err == -ERANGE && !he->ms.map->erange_warned)
-		ui__warn_map_erange(he->ms.map, sym, ip);
-	else if (err == -ENOMEM) {
-		pr_err("Not enough memory for annotating '%s' symbol!\n",
-		       sym->name);
-		sleep(1);
+		pthread_mutex_lock(&he->hists->lock);
 	}
-
-	pthread_mutex_lock(&he->hists->lock);
 }
 
 static void perf_top__show_details(struct perf_top *top)

2.1.0

From: Namhyung Kim <[email protected]>

The ui__has_annotation() inside perf_top__record_precise_ip() should be removed since it returns true only for TUI (and when sort key has symbol). However the 'perf top --stdio' also supports annotation for a symbol which was specified by 's' key action.

Actually it already does the necessary checks before calling the function. So it's ok to get rid of the check here.

---
 tools/perf/builtin-top.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

index 586798acf7db..f447e5531f8b 100644
@@ -189,8 +189,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 	if (pthread_mutex_trylock(&notes->lock))
 		return;
 
-	if (ui__has_annotation())
-		err = hist_entry__inc_addr_samples(he, counter, ip);
+	err = hist_entry__inc_addr_samples(he, counter, ip);
 
 	pthread_mutex_unlock(&notes->lock);
 

2.1.0

From: Namhyung Kim <[email protected]>

The 'he' cannot be NULL since it's caller hist_iter__top_callback() is called only if iter->he is not NULL (see hist_entry_iter__add). So setting 'sym' before the condition to simplify the code.

Also make it clearer that the top->symbol_filter_entry check is only meaningful on stdio mode (i.e. when use_browser is 0).

---
 tools/perf/builtin-top.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

index f447e5531f8b..92fe963e43c4 100644
@@ -175,15 +175,14 @@ static void perf_top__record_precise_ip(struct perf_top *top,
 					int counter, u64 ip)
 {
 	struct annotation *notes;
-	struct symbol *sym;
+	struct symbol *sym = he->ms.sym;
 	int err = 0;
 
-	if (he == NULL || he->ms.sym == NULL ||
-	    ((top->sym_filter_entry == NULL ||
-	      top->sym_filter_entry->ms.sym != he->ms.sym) && use_browser != 1))
+	if (sym == NULL || (use_browser == 0 &&
+			    (top->sym_filter_entry == NULL ||
+			     top->sym_filter_entry->ms.sym != sym)))
 		return;
 
-	sym = he->ms.sym;
 	notes = symbol__annotation(sym);
 
 	if (pthread_mutex_trylock(&notes->lock))

2.1.0

From: Wang Nan <[email protected]>

Commit 561bbccac72d08babafaa33fd7fa9100ec4c9fb6 ("tools lib bpf:
Extract and collect map names from BPF object file") forgets checking return value of strdup(). This patch fixes it. It also checks names pointer before strcmp() for safety.

---
 tools/lib/bpf/libbpf.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

index a298614ad091..16485ab05fc1 100644
@@ -527,14 +527,14 @@ bpf_object__init_maps(struct bpf_object *obj, void *data,
 	return 0;
 }
 
-static void
+static int
 bpf_object__init_maps_name(struct bpf_object *obj, int maps_shndx)
 {
 	int i;
 	Elf_Data *symbols = obj->efile.symbols;
 
 	if (!symbols || maps_shndx < 0)
-		return;
+		return -EINVAL;
 
 	for (i = 0; i < symbols->d_size / sizeof(GElf_Sym); i++) {
 		GElf_Sym sym;

@@ -556,9 +556,14 @@ bpf_object__init_maps_name(struct bpf_object *obj, int maps_shndx)
 			continue;
 		}
 		obj->maps[map_idx].name = strdup(map_name);
+		if (!obj->maps[map_idx].name) {
+			pr_warning("failed to alloc map name\n");
+			return -ENOMEM;
+		}
 		pr_debug("map %zu is \"%s\"\n", map_idx,
 			 obj->maps[map_idx].name);
 	}
+	return 0;
 }
 
 static int bpf_object__elf_collect(struct bpf_object *obj)

@@ -663,7 +668,7 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 	}
 
 	if (maps_shndx >= 0)
-		bpf_object__init_maps_name(obj, maps_shndx);
+		err = bpf_object__init_maps_name(obj, maps_shndx);
 out:
 	return err;
 }

@@ -1372,7 +1377,7 @@ bpf_object__get_map_by_name(struct bpf_object *obj, const char *name)
 	struct bpf_map *pos;
 
 	bpf_map__for_each(pos, obj) {
-		if (strcmp(pos->name, name) == 0)
+		if (pos->name && !strcmp(pos->name, name))
 			return pos;
 	}
 	return NULL;

2.1.0

From: Wang Nan <[email protected]>

Namhyung Kim pointed out a potential problem in original code that it fetches names of maps from section header string table, which is used to store section names.

Original code doesn't cause error because of a LLVM behavior that, it combines shstrtab into strtab. For example:

 $ echo 'int func() {return 0;}' | x86_64-oe-linux-clang -x c -o temp.o -c -
 $ readelf -h ./temp.o
 ELF Header:
   Magic:   7f 45 4c 46 02 01 01 03 00 00 00 00 00 00 00 00
   ...
   Section header string table index: 1
 $ readelf -S ./temp.o
 There are 10 section headers, starting at offset 0x288:

Section Headers:

   [Nr] Name              Type             Address           Offset
        Size              EntSize          Flags  Link  Info  Align
   [ 0]                   NULL             0000000000000000  00000000
        0000000000000000  0000000000000000           0     0     0
   [ 1] .strtab           STRTAB           0000000000000000  00000230
        0000000000000051  0000000000000000           0     0     1
        ...
 $ readelf -p .strtab ./temp.o

String dump of section '.strtab':

   [     1]  .text
   [     7]  .comment
   [    10]  .bss
   [    15]  .note.GNU-stack
   [    25]  .rela.eh_frame
   [    34]  func
   [    39]  .strtab
   [    41]  .symtab
   [    49]  .data
   [    4f]  -

 $ readelf -p .shstrtab ./temp.o
 readelf: Warning: Section '.shstrtab' was not dumped because it does not exist!

Where, 'section header string table index' points to '.strtab', and symbol names are also stored there.

However, in case of gcc:

 $ echo 'int func() {return 0;}' | gcc -x c -o temp.o -c -
 $ readelf -p .shstrtab ./temp.o

String dump of section '.shstrtab':

   [     1]  .symtab
   [     9]  .strtab
   [    11]  .shstrtab
   [    1b]  .text
   [    21]  .data
   [    27]  .bss
   [    2c]  .comment
   [    35]  .note.GNU-stack
   [    45]  .rela.eh_frame
 $ readelf -p .strtab ./temp.o

String dump of section '.strtab':
[ 1] func

They are separated sections.

Although original code doesn't cause error, we'd better use canonical method for fetching symbol names to avoid potential behavior changing. This patch learns from readelf's code, fetches string from sh_link of .symbol section.

---
 tools/lib/bpf/libbpf.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

index 16485ab05fc1..8334a5a9d5d7 100644
@@ -195,6 +195,7 @@ struct bpf_object {
 		Elf *elf;
 		GElf_Ehdr ehdr;
 		Elf_Data *symbols;
+		size_t strtabidx;
 		struct {
 			GElf_Shdr shdr;
 			Elf_Data *data;

@@ -547,7 +548,7 @@ bpf_object__init_maps_name(struct bpf_object *obj, int maps_shndx)
 			continue;
 
 		map_name = elf_strptr(obj->efile.elf,
-				      obj->efile.ehdr.e_shstrndx,
+				      obj->efile.strtabidx,
 				      sym.st_name);
 		map_idx = sym.st_value / sizeof(struct bpf_map_def);
 		if (map_idx >= obj->nr_maps) {

@@ -630,8 +631,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 				pr_warning("bpf: multiple SYMTAB in %s\n",
 					   obj->path);
 				err = -LIBBPF_ERRNO__FORMAT;
-			} else
+			} else {
 				obj->efile.symbols = data;
+				obj->efile.strtabidx = sh.sh_link;
+			}
 		} else if ((sh.sh_type == SHT_PROGBITS) &&
 			   (sh.sh_flags & SHF_EXECINSTR) &&
 			   (data->d_size > 0)) {

@@ -667,6 +670,10 @@ static int bpf_object__elf_collect(struct bpf_object *obj)
 			goto out;
 	}
 
+	if (!obj->efile.strtabidx || obj->efile.strtabidx >= idx) {
+		pr_warning("Corrupted ELF file: index of strtab invalid\n");
+		return LIBBPF_ERRNO__FORMAT;
+	}
 	if (maps_shndx >= 0)
 		err = bpf_object__init_maps_name(obj, maps_shndx);
 out:

2.1.0

From: Wang Nan <[email protected]>

Add hexadecimal u32 to base data type, which is useful for raw output because raw data is u32 aligned.

---
 tools/perf/util/data-convert-bt.c | 2 ++
 1 file changed, 2 insertions(+)

index 5bfc1198ab46..34cd1e4039d3 100644
@@ -63,6 +63,7 @@ struct ctf_writer {
 			struct bt_ctf_field_type	*s32;
 			struct bt_ctf_field_type	*u32;
 			struct bt_ctf_field_type	*string;
+			struct bt_ctf_field_type	*u32_hex;
 			struct bt_ctf_field_type	*u64_hex;
 		};
 		struct bt_ctf_field_type *array[6];

@@ -982,6 +983,7 @@ do {							\
 	CREATE_INT_TYPE(cw->data.u64, 64, false, false);
 	CREATE_INT_TYPE(cw->data.s32, 32, true,  false);
 	CREATE_INT_TYPE(cw->data.u32, 32, false, false);
+	CREATE_INT_TYPE(cw->data.u32_hex, 32, false, true);
 	CREATE_INT_TYPE(cw->data.u64_hex, 64, false, true);
 
 	cw->data.string  = bt_ctf_field_type_string_create();

2.1.0

From: Wang Nan <[email protected]>

Useful for getting stack traces for hardware breakpoint events.

Test result:

Before this patch:
# ~/perf record -g -e mem:0x600980 ./sample
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.011 MB perf.data (12 samples) ]

# ~/perf script

# ~/perf script -F comm,tid,pid,time,event,ip,sym,dso sample 22520/22520 97457.836294: mem:0x600980:

          5a4ad8 __clear_user (/lib/modules/4.3.0-rc4+/build/vmlinux)
 ...
          3f41ba sys_execve (/lib/modules/4.3.0-rc4+/build/vmlinux)
          979395 return_from_execve (/lib/modules/4.3.0-rc4+/build/vmlinux)
    7f1b59719cf7 [unknown] ([unknown])

sample 22520/22520 97457.836648: mem:0x600980:

             532 main (/home/w00229757/DataBreakpoints/sample)
           21bd5 __libc_start_main (/tmp/oxygen_root-root/lib64/libc-2.18.so)

...

After this patch:
# ~/perf script
sample 22520 97457.836294: mem:0x600980:

                   5a4ad8 __clear_user (/lib/modules/4.3.0-rc4+/build/vmlinux)
 ...
                   3f41ba sys_execve (/lib/modules/4.3.0-rc4+/build/vmlinux)
                   979395 return_from_execve (/lib/modules/4.3.0-rc4+/build/vmlinux)
             7f1b59719cf7 [unknown] ([unknown])

sample 22520 97457.836648: mem:0x600980:

                      532 main (/home/w00229757/DataBreakpoints/sample)
                    21bd5 __libc_start_main (/tmp/oxygen_root-root/lib64/libc-2.18.so)

Committer note:

So, further testing, lets do it for a kernel global variable, tcp_hashinfo:

  # grep -w tcp_hashinfo /proc/kallsyms
  ffffffff8202fc00 B tcp_hashinfo
  #

Note: allow specifying mem:tcp_hashinfo:

  # perf record -g -e mem:0xffffffff81c65ac0 -a
  ^C[ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.790 MB perf.data ]
  #
  # perf evlist
  mem:0xffffffff8202fc00
  # perf evlist -v
  mem:0xffffffff8202fc00: type: 5, size: 112, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|CALLCHAIN|CPU, disabled: 1, inherit: 1, mmap: 1, comm: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1, bp_type: 3, { bp_addr, config1 }: 0xffffffff8202fc00, { bp_len, config2 }: 0x4
  #

Then, after this patch:

  # perf script
  swapper 0 [000] 171036.986988: mem:0xffffffff8202fc00:
    8a0fb5 __inet_lookup_established (/lib/modules/4.3.0+/build/vmlinux)
    8bc09d tcp_v4_early_demux (/lib/modules/4.3.0+/build/vmlinux)
    896def ip_rcv_finish (/lib/modules/4.3.0+/build/vmlinux)
    8976c2 ip_rcv (/lib/modules/4.3.0+/build/vmlinux)
    855eba __netif_receive_skb_core (/lib/modules/4.3.0+/build/vmlinux)
    8565d8 __netif_receive_skb (/lib/modules/4.3.0+/build/vmlinux)
    8572a8 process_backlog (/lib/modules/4.3.0+/build/vmlinux)
    856b11 net_rx_action (/lib/modules/4.3.0+/build/vmlinux)
    2a284b __do_softirq (/lib/modules/4.3.0+/build/vmlinux)
    2a2ba3 irq_exit (/lib/modules/4.3.0+/build/vmlinux)
    96b7a4 do_IRQ (/lib/modules/4.3.0+/build/vmlinux)
    969807 ret_from_intr (/lib/modules/4.3.0+/build/vmlinux)
    804c27 cpuidle_enter (/lib/modules/4.3.0+/build/vmlinux)
    2ded22 call_cpuidle (/lib/modules/4.3.0+/build/vmlinux)
    2defb6 cpu_startup_entry (/lib/modules/4.3.0+/build/vmlinux)
    95d5bc rest_init (/lib/modules/4.3.0+/build/vmlinux)
   1163ffa start_kernel ([kernel.vmlinux].init.text)
   11634d7 x86_64_start_reservations ([kernel.vmlinux].init.text)
   1163623 x86_64_start_kernel ([kernel.vmlinux].init.text)

---
 tools/perf/builtin-script.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

index 3c3f8d0e3064..d259e9aa3a71 100644
@@ -130,6 +130,18 @@ static struct {
 
 		.invalid_fields = PERF_OUTPUT_TRACE,
 	},
+
+	[PERF_TYPE_BREAKPOINT] = {
+		.user_set = false,
+
+		.fields = PERF_OUTPUT_COMM | PERF_OUTPUT_TID |
+			      PERF_OUTPUT_CPU | PERF_OUTPUT_TIME |
+			      PERF_OUTPUT_EVNAME | PERF_OUTPUT_IP |
+			      PERF_OUTPUT_SYM | PERF_OUTPUT_DSO |
+			      PERF_OUTPUT_PERIOD,
+
+		.invalid_fields = PERF_OUTPUT_TRACE,
+	},
 };
 
 static bool output_set_by_user(void)

@@ -1129,6 +1141,8 @@ static int parse_output_fields(const struct option *opt __maybe_unused,
 			type = PERF_TYPE_TRACEPOINT;
 		else if (!strcmp(str, "raw"))
 			type = PERF_TYPE_RAW;
+		else if (!strcmp(str, "break"))
+			type = PERF_TYPE_BREAKPOINT;
 		else {
 			fprintf(stderr, "Invalid event type in field string.\n");
 			rc = -EINVAL;

2.1.0

From: Wang Nan <[email protected]>

There are so many test cases use stack allocated 'struct machine'. Including:
test__hists_link
test__hists_filter
test__mmap_thread_lookup
test__thread_mg_share
test__hists_output
test__hists_cumulate

Also, in non-test code (for example, machine__new_host()) there are code use 'malloc()' to alloc struct machine.

These are dangerous operations, cause some tests fail or hung in machines__exit(). For example, in

machines__exit ->
machine__destroy_kernel_maps ->

     map_groups__remove ->
       maps__remove ->
         pthread_rwlock_wrlock

a incorrectly initialized lock causes unintended behavior.

This patch memset(0) that structure in machine__init() to ensure all fields in 'struct machine' are initialized to zero.

---
 tools/perf/util/machine.c | 1 +
 1 file changed, 1 insertion(+)

index f5882b8c8db9..1407d5107480 100644
@@ -25,6 +25,7 @@ static void dsos__init(struct dsos *dsos)
 
 int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
 {
+	memset(machine, 0, sizeof(*machine));
 	map_groups__init(&machine->kmaps, machine);
 	RB_CLEAR_NODE(&machine->rb_node);
 	dsos__init(&machine->dsos);

2.1.0

Pulled, thanks a lot Arnaldo!

Ingo