Skip to contentSkip to conversation

perf tools: Allow vmlinux to fallback to kallsyms on NO_LIBELF=1

PATCH In review Namhyung Kim 5 participants 28 messages Nov 7, 2014 – Nov 20, 2014 perf events

lore
Share & tools
lore

Namhyung Kim

author maintainer
#0 lore raw

PATCH 1/3 perf tools: Allow vmlinux to fallback to kallsyms on NO_LIBELF=1

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

  # Overhead  Command  Shared Object      Symbol
  # ........  .......  .................  ........................
  #
      34.51%  swapper  [kernel.kallsyms]  [k] intel_idle
      12.54%  perf     [kernel.kallsyms]  [k] generic_exec_single
      10.11%  swapper  [kernel.kallsyms]  [k] int_sqrt
       9.83%  swapper  [kernel.kallsyms]  [k] hrtimer_interrupt
       7.25%  emacs    [kernel.kallsyms]  [k] __switch_to
       7.06%  sleep    [kernel.kallsyms]  [k] find_next_iomem_res
       7.02%  swapper  [kernel.kallsyms]  [k] run_timer_softirq
       2.55%  swapper  [kernel.kallsyms]  [k] _raw_spin_unlock_irq
       1.90%  swapper  [kernel.kallsyms]  [k] lapic_next_deadline
       1.75%  swapper  [kernel.kallsyms]  [k] native_sched_clock
       1.49%  swapper  [kernel.kallsyms]  [k] __schedule
       1.20%  swapper  [kernel.kallsyms]  [k] read_tsc
       1.10%  sleep    [kernel.kallsyms]  [k] current_kernel_time
       0.99%  swapper  [kernel.kallsyms]  [k] pick_next_task_rt
       0.36%  swapper  [kernel.kallsyms]  [k] group_sched_in
       0.24%  swapper  [kernel.kallsyms]  [k] x86_pmu_commit_txn
       0.06%  swapper  [kernel.kallsyms]  [k] intel_pmu_enable_all
       0.03%  perf     [kernel.kallsyms]  [k] intel_pmu_enable_all
Reported-by Peter Zijlstra <[email protected]>
Cc Adrian Hunter <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/symbol-minimal.c +3 -0
index c9541fea9514..226cf41ed7e6 100644
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 	unsigned char *build_id[BUILD_ID_SIZE];
 	int ret;
 
+	if (dso->kernel)
+		return 0;  /* always use kallsyms */
+
 	ret = fd__is_64_bit(ss->fd);
 	if (ret >= 0)
 		dso->is_64_bit = ret;
Signature · 1 line
2.1.2
author maintainer
#1 lore raw

PATCH 2/3 perf symbol: Implement a very simple ELF symbol parser

It'll be used to show (userspace) symbol names when libelf isn't (or cannot be) linked.

  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  .........................
  #
      37.01%  mem-memcpy  libc-2.17.so       [.] __memcpy_ssse3_back
      24.25%  perf        ld-2.17.so         [.] _dl_relocate_object
      22.16%  perf        [kernel.kallsyms]  [k] kmem_cache_alloc
      14.29%  mem-memset  libc-2.17.so       [.] __memset_sse2
       2.21%  perf        [kernel.kallsyms]  [k] flush_signal_handlers
       0.07%  perf        [kernel.kallsyms]  [k] intel_pmu_enable_all
Cc Adrian Hunter <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 143 ++++++++++++++++++++++++++++++++++++---
 tools/perf/util/symbol-minimal.h |  64 ++++++++++++++++++
 tools/perf/util/symbol.h         |  10 ++-
 3 files changed, 208 insertions(+), 9 deletions(-)
 create mode 100644 tools/perf/util/symbol-minimal.h
tools/perf/util/symbol-minimal.c +135 -8
index 226cf41ed7e6..9da571d83a25 100644
@@ -1,11 +1,13 @@
 #include "symbol.h"
 #include "util.h"
+#include "debug.h"
 
 #include <stdio.h>
 #include <fcntl.h>
 #include <string.h>
 #include <byteswap.h>
 #include <sys/stat.h>
+#include <sys/mman.h>
 
 
 static bool check_need_swap(int file_endian)

@@ -242,22 +244,81 @@ int sysfs__read_build_id(const char *filename, void *build_id, size_t size)
 	return ret;
 }
 
+static const char *symsrc__symname(struct symsrc *ss, Elf_Shdr *strsec, int idx)
+{
+	return ss->ptr + strsec->sh_offset + idx;
+}
+
 int symsrc__init(struct symsrc *ss, struct dso *dso __maybe_unused,
 		 const char *name,
 	         enum dso_binary_type type)
 {
-	int fd = open(name, O_RDONLY);
+	int fd;
+	unsigned i;
+	Elf_Ehdr ehdr;
+	struct stat stbuf;
+	void *ptr;
+
+	fd = open(name, O_RDONLY);
 	if (fd < 0)
 		return -1;
 
 	ss->name = strdup(name);
-	if (!ss->name)
-		goto out_close;
+	if (!ss->name) {
+		close(fd);
+		return -1;
+	}
 
 	ss->fd = fd;
 	ss->type = type;
 
+	/* only consider native case (no swap) */
+	if (read(fd, &ehdr, sizeof(ehdr)) < 0)
+		goto out_close;
+
+	if (memcmp(ehdr.e_ident, ELFMAG, SELFMAG) ||
+	    ehdr.e_ident[EI_VERSION] != EV_CURRENT)
+		goto out_close;
+
+	if (ehdr.e_ident[EI_CLASS] != ELFCLASS)
+		goto out_close;
+
+	if (check_need_swap(ehdr.e_ident[EI_DATA]))
+		goto out_close;
+
+	if (ehdr.e_shentsize != sizeof(Elf_Shdr))
+		goto out_close;
+
+	if (fstat(fd, &stbuf) < 0)
+		goto out_close;
+
+	ptr = mmap(NULL, stbuf.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (ptr == MAP_FAILED)
+		goto out_close;
+
+	ss->ptr = ptr;
+	ss->len = stbuf.st_size;
+	ss->sec = ptr + ehdr.e_shoff;
+
+	ss->symtab = NULL;
+	for (i = 0; i < ehdr.e_shnum; i++) {
+		Elf_Shdr *shdrp = &ss->sec[i];
+		const char *shname;
+
+		shname = symsrc__symname(ss, &ss->sec[ehdr.e_shstrndx],
+					 shdrp->sh_name);
+
+		if (!strcmp(shname, ".text"))
+			ss->adjust_offset = shdrp->sh_addr - shdrp->sh_offset;
+
+		if (shdrp->sh_type == SHT_SYMTAB) {
+			ss->symtab = shdrp;
+			ss->strtab = &ss->sec[shdrp->sh_link];
+		}
+	}
+
 	return 0;
+
 out_close:
 	close(fd);
 	return -1;

@@ -276,6 +337,7 @@ bool symsrc__has_symtab(struct symsrc *ss __maybe_unused)
 
 void symsrc__destroy(struct symsrc *ss)
 {
+	munmap(ss->ptr, ss->len);
 	zfree(&ss->name);
 	close(ss->fd);
 }

@@ -332,7 +394,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 		  symbol_filter_t filter __maybe_unused,
 		  int kmodule __maybe_unused)
 {
-	unsigned char *build_id[BUILD_ID_SIZE];
+	Elf_Ehdr *ehdr = ss->ptr;
+	Elf_Sym *symtab;
+	size_t i, nr_sym;
 	int ret;
 
 	if (dso->kernel)

@@ -342,11 +406,74 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 	if (ret >= 0)
 		dso->is_64_bit = ret;
 
-	if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) > 0) {
-		dso__set_build_id(dso, build_id);
-		return 1;
+	if (dso->has_build_id) {
+		u8 build_id[BUILD_ID_SIZE];
+
+		if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) < 0)
+			return -1;
+
+		if (!dso__build_id_equal(dso, build_id))
+			return -1;
 	}
-	return 0;
+
+	if (map->type != MAP__FUNCTION)
+		return 0;
+
+	if (!ss->symtab)
+		return 0;
+
+	symtab = ss->ptr + ss->symtab->sh_offset;
+	nr_sym = ss->symtab->sh_size / ss->symtab->sh_entsize;
+
+	pr_debug4("loading symbols from %s\n", ss->name);
+	ret = 0;
+	for (i = 0; i < nr_sym; i++) {
+		struct symbol *s;
+		Elf_Sym *sym = &symtab[i];
+		const char *symname = symsrc__symname(ss, ss->strtab, sym->st_name);
+		unsigned long sym_addr = sym->st_value;
+
+		if (ELF_ST_TYPE(sym->st_info) != STT_FUNC)
+			continue;
+
+		if (sym->st_shndx == SHN_UNDEF)
+			continue;
+
+		if (ehdr->e_machine == EM_ARM) {
+			/* Reject ARM ELF "mapping symbols": these aren't unique
+			 * and don't identify functions, so will confuse the
+			 * profile output: */
+			if (!strcmp(symname, "$a") ||
+			    !strcmp(symname, "$d") ||
+			    !strcmp(symname, "$t"))
+				continue;
+
+			/* On ARM, symbols for thumb functions have 1 added to
+			 * the symbol address as a flag - remove it */
+			if (sym->st_value & 1)
+				sym_addr--;
+		}
+		sym_addr -= ss->adjust_offset;
+
+		pr_debug4("sym: %s (%lx+%lu)\n", symname, sym_addr, sym->st_size);
+		s = symbol__new(sym_addr, sym->st_size,
+				ELF_ST_BIND(sym->st_info), symname);
+		if (!s)
+			break;
+
+		if (filter && filter(map, s))
+			symbol__delete(s);
+		else {
+			symbols__insert(&dso->symbols[map->type], s);
+			ret++;
+		}
+	}
+
+	if (ret > 0) {
+		symbols__fixup_duplicate(&dso->symbols[map->type]);
+		symbols__fixup_end(&dso->symbols[map->type]);
+	}
+	return ret;
 }
 
 int file__read_maps(int fd __maybe_unused, bool exe __maybe_unused,
tools/perf/util/symbol-minimal.h new file +64 -0

64 lines not on this page · raw message

tools/perf/util/symbol.h +9 -1
index ded3ca7266de..1c1dc5b9b3f8 100644
@@ -18,8 +18,9 @@
 #ifdef HAVE_LIBELF_SUPPORT
 #include <libelf.h>
 #include <gelf.h>
+#else
+#include "symbol-minimal.h"
 #endif
-#include <elf.h>
 
 #include "dso.h"
 

@@ -229,6 +230,13 @@ struct symsrc {
 
 	bool adjust_symbols;
 	bool is_64_bit;
+#else
+	void *ptr;
+	size_t len;
+	Elf_Shdr *sec;
+	Elf_Shdr *symtab;
+	Elf_Shdr *strtab;
+	long adjust_offset;
 #endif
 };
Signature · 1 line
2.1.2
maintainer
#2 lore raw

Re: [PATCH 2/3] perf symbol: Implement a very simple ELF symbol parser

Em Fri, Nov 07, 2014 at 02:20:05PM +0900, Namhyung Kim escreveu:

Quoted 2 lines

It'll be used to show (userspace) symbol names when libelf isn't (or cannot be) linked.

Does this deals with prelink, etc?

- Arnaldo

Quoted 321 lines
  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  .........................
  #
      37.01%  mem-memcpy  libc-2.17.so       [.] __memcpy_ssse3_back
      24.25%  perf        ld-2.17.so         [.] _dl_relocate_object
      22.16%  perf        [kernel.kallsyms]  [k] kmem_cache_alloc
      14.29%  mem-memset  libc-2.17.so       [.] __memset_sse2
       2.21%  perf        [kernel.kallsyms]  [k] flush_signal_handlers
       0.07%  perf        [kernel.kallsyms]  [k] intel_pmu_enable_all
Cc Adrian Hunter <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 143 ++++++++++++++++++++++++++++++++++++---
 tools/perf/util/symbol-minimal.h |  64 ++++++++++++++++++
 tools/perf/util/symbol.h         |  10 ++-
 3 files changed, 208 insertions(+), 9 deletions(-)
 create mode 100644 tools/perf/util/symbol-minimal.h
tools/perf/util/symbol-minimal.c +135 -8
index 226cf41ed7e6..9da571d83a25 100644
@@ -1,11 +1,13 @@
 #include "symbol.h"
 #include "util.h"
+#include "debug.h"
 
 #include <stdio.h>
 #include <fcntl.h>
 #include <string.h>
 #include <byteswap.h>
 #include <sys/stat.h>
+#include <sys/mman.h>
 
 
 static bool check_need_swap(int file_endian)

@@ -242,22 +244,81 @@ int sysfs__read_build_id(const char *filename, void *build_id, size_t size)
 	return ret;
 }
 
+static const char *symsrc__symname(struct symsrc *ss, Elf_Shdr *strsec, int idx)
+{
+	return ss->ptr + strsec->sh_offset + idx;
+}
+
 int symsrc__init(struct symsrc *ss, struct dso *dso __maybe_unused,
 		 const char *name,
 	         enum dso_binary_type type)
 {
-	int fd = open(name, O_RDONLY);
+	int fd;
+	unsigned i;
+	Elf_Ehdr ehdr;
+	struct stat stbuf;
+	void *ptr;
+
+	fd = open(name, O_RDONLY);
 	if (fd < 0)
 		return -1;
 
 	ss->name = strdup(name);
-	if (!ss->name)
-		goto out_close;
+	if (!ss->name) {
+		close(fd);
+		return -1;
+	}
 
 	ss->fd = fd;
 	ss->type = type;
 
+	/* only consider native case (no swap) */
+	if (read(fd, &ehdr, sizeof(ehdr)) < 0)
+		goto out_close;
+
+	if (memcmp(ehdr.e_ident, ELFMAG, SELFMAG) ||
+	    ehdr.e_ident[EI_VERSION] != EV_CURRENT)
+		goto out_close;
+
+	if (ehdr.e_ident[EI_CLASS] != ELFCLASS)
+		goto out_close;
+
+	if (check_need_swap(ehdr.e_ident[EI_DATA]))
+		goto out_close;
+
+	if (ehdr.e_shentsize != sizeof(Elf_Shdr))
+		goto out_close;
+
+	if (fstat(fd, &stbuf) < 0)
+		goto out_close;
+
+	ptr = mmap(NULL, stbuf.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
+	if (ptr == MAP_FAILED)
+		goto out_close;
+
+	ss->ptr = ptr;
+	ss->len = stbuf.st_size;
+	ss->sec = ptr + ehdr.e_shoff;
+
+	ss->symtab = NULL;
+	for (i = 0; i < ehdr.e_shnum; i++) {
+		Elf_Shdr *shdrp = &ss->sec[i];
+		const char *shname;
+
+		shname = symsrc__symname(ss, &ss->sec[ehdr.e_shstrndx],
+					 shdrp->sh_name);
+
+		if (!strcmp(shname, ".text"))
+			ss->adjust_offset = shdrp->sh_addr - shdrp->sh_offset;
+
+		if (shdrp->sh_type == SHT_SYMTAB) {
+			ss->symtab = shdrp;
+			ss->strtab = &ss->sec[shdrp->sh_link];
+		}
+	}
+
 	return 0;
+
 out_close:
 	close(fd);
 	return -1;

@@ -276,6 +337,7 @@ bool symsrc__has_symtab(struct symsrc *ss __maybe_unused)
 
 void symsrc__destroy(struct symsrc *ss)
 {
+	munmap(ss->ptr, ss->len);
 	zfree(&ss->name);
 	close(ss->fd);
 }

@@ -332,7 +394,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 		  symbol_filter_t filter __maybe_unused,
 		  int kmodule __maybe_unused)
 {
-	unsigned char *build_id[BUILD_ID_SIZE];
+	Elf_Ehdr *ehdr = ss->ptr;
+	Elf_Sym *symtab;
+	size_t i, nr_sym;
 	int ret;
 
 	if (dso->kernel)

@@ -342,11 +406,74 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 	if (ret >= 0)
 		dso->is_64_bit = ret;
 
-	if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) > 0) {
-		dso__set_build_id(dso, build_id);
-		return 1;
+	if (dso->has_build_id) {
+		u8 build_id[BUILD_ID_SIZE];
+
+		if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) < 0)
+			return -1;
+
+		if (!dso__build_id_equal(dso, build_id))
+			return -1;
 	}
-	return 0;
+
+	if (map->type != MAP__FUNCTION)
+		return 0;
+
+	if (!ss->symtab)
+		return 0;
+
+	symtab = ss->ptr + ss->symtab->sh_offset;
+	nr_sym = ss->symtab->sh_size / ss->symtab->sh_entsize;
+
+	pr_debug4("loading symbols from %s\n", ss->name);
+	ret = 0;
+	for (i = 0; i < nr_sym; i++) {
+		struct symbol *s;
+		Elf_Sym *sym = &symtab[i];
+		const char *symname = symsrc__symname(ss, ss->strtab, sym->st_name);
+		unsigned long sym_addr = sym->st_value;
+
+		if (ELF_ST_TYPE(sym->st_info) != STT_FUNC)
+			continue;
+
+		if (sym->st_shndx == SHN_UNDEF)
+			continue;
+
+		if (ehdr->e_machine == EM_ARM) {
+			/* Reject ARM ELF "mapping symbols": these aren't unique
+			 * and don't identify functions, so will confuse the
+			 * profile output: */
+			if (!strcmp(symname, "$a") ||
+			    !strcmp(symname, "$d") ||
+			    !strcmp(symname, "$t"))
+				continue;
+
+			/* On ARM, symbols for thumb functions have 1 added to
+			 * the symbol address as a flag - remove it */
+			if (sym->st_value & 1)
+				sym_addr--;
+		}
+		sym_addr -= ss->adjust_offset;
+
+		pr_debug4("sym: %s (%lx+%lu)\n", symname, sym_addr, sym->st_size);
+		s = symbol__new(sym_addr, sym->st_size,
+				ELF_ST_BIND(sym->st_info), symname);
+		if (!s)
+			break;
+
+		if (filter && filter(map, s))
+			symbol__delete(s);
+		else {
+			symbols__insert(&dso->symbols[map->type], s);
+			ret++;
+		}
+	}
+
+	if (ret > 0) {
+		symbols__fixup_duplicate(&dso->symbols[map->type]);
+		symbols__fixup_end(&dso->symbols[map->type]);
+	}
+	return ret;
 }
 
 int file__read_maps(int fd __maybe_unused, bool exe __maybe_unused,
tools/perf/util/symbol-minimal.h new file +64 -0
new file mode 100644
index 000000000000..7b8b38426359
@@ -0,0 +1,64 @@
+#ifndef __PERF_SYMBOL_MINIMAL_H
+#define __PERF_SYMBOL_MINIMAL_H
+
+#include <elf.h>
+#include <linux/bitops.h>
+
+#if BITS_PER_LONG == 32
+
+# define ELFCLASS	ELFCLASS32
+
+# define Elf_Half	Elf32_Half
+# define Elf_Word	Elf32_Word
+# define Elf_Sword	Elf32_Sword
+# define Elf_Xword	Elf32_Xword
+# define Elf_Sxword	Elf32_Sxword
+# define Elf_Addr	Elf32_Addr
+# define Elf_Off	Elf32_Off
+# define Elf_Section	Elf32_Section
+# define Elf_Versym	Elf32_Versym
+
+# define Elf_Ehdr	Elf32_Ehdr
+# define Elf_Phdr	Elf32_Phdr
+# define Elf_Shdr	Elf32_Shdr
+# define Elf_Nhdr	Elf32_Nhdr
+# define Elf_Sym	Elf32_Sym
+# define Elf_Syminfo	Elf32_Syminfo
+# define Elf_Rel	Elf32_Rel
+# define Elf_Rela	Elf32_Rela
+# define Elf_Dyn	Elf32_Dyn
+
+# define ELF_ST_BIND(val)		ELF32_ST_BIND(val)
+# define ELF_ST_TYPE(val)		ELF32_ST_TYPE(val)
+# define ELF_ST_INFO(bind, type)	ELF32_ST_INFO(bind, type)
+
+#else /* BITS_PER_LONG == 64 */
+
+# define ELFCLASS	ELFCLASS64
+
+# define Elf_Half	Elf64_Half
+# define Elf_Word	Elf64_Word
+# define Elf_Sword	Elf64_Sword
+# define Elf_Xword	Elf64_Xword
+# define Elf_Sxword	Elf64_Sxword
+# define Elf_Addr	Elf64_Addr
+# define Elf_Off	Elf64_Off
+# define Elf_Section	Elf64_Section
+# define Elf_Versym	Elf64_Versym
+
+# define Elf_Ehdr	Elf64_Ehdr
+# define Elf_Phdr	Elf64_Phdr
+# define Elf_Shdr	Elf64_Shdr
+# define Elf_Nhdr	Elf64_Nhdr
+# define Elf_Sym	Elf64_Sym
+# define Elf_Syminfo	Elf64_Syminfo
+# define Elf_Rel	Elf64_Rel
+# define Elf_Rela	Elf64_Rela
+# define Elf_Dyn	Elf64_Dyn
+
+# define ELF_ST_BIND(val)		ELF64_ST_BIND(val)
+# define ELF_ST_TYPE(val)		ELF64_ST_TYPE(val)
+# define ELF_ST_INFO(bind, type)	ELF64_ST_INFO(bind, type)
+
+#endif /* BITS_PER_LONG == 64 */
+#endif /* __PERF_SYMBOL_MINIMAL_H */
tools/perf/util/symbol.h +9 -1
index ded3ca7266de..1c1dc5b9b3f8 100644
@@ -18,8 +18,9 @@
 #ifdef HAVE_LIBELF_SUPPORT
 #include <libelf.h>
 #include <gelf.h>
+#else
+#include "symbol-minimal.h"
 #endif
-#include <elf.h>
 
 #include "dso.h"
 

@@ -229,6 +230,13 @@ struct symsrc {
 
 	bool adjust_symbols;
 	bool is_64_bit;
+#else
+	void *ptr;
+	size_t len;
+	Elf_Shdr *sec;
+	Elf_Shdr *symtab;
+	Elf_Shdr *strtab;
+	long adjust_offset;
 #endif
 };
Signature · 1 line
2.1.2
author maintainer
#3 lore raw

Re: [PATCH 2/3] perf symbol: Implement a very simple ELF symbol parser

Hi Arnaldo,

Quoted 5 lines · Arnaldo Carvalho de Melo

Em Fri, Nov 07, 2014 at 02:20:05PM +0900, Namhyung Kim escreveu:

Quoted 2 lines

It'll be used to show (userspace) symbol names when libelf isn't (or cannot be) linked.

Does this deals with prelink, etc?

I believe so. :)

Quoted 5 lines
Quoted 5 lines
 
  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  .........................
  #
      37.01%  mem-memcpy  libc-2.17.so       [.] __memcpy_ssse3_back
  namhyung@sejong:linux$ readelf -WS /lib64/libc-2.17.so | grep prelink
    [41] .gnu.prelink_undo PROGBITS        0000000000000000 200368 000c30 01      0   0  8
  namhyung@sejong:linux$ nm /lib64/libc-2.17.so | grep __memcpy_ssse3_back
  0000003153f46f40 t __memcpy_ssse3_back

Thanks,
Namhyung

Quoted 5 lines
Quoted 5 lines
      24.25%  perf        ld-2.17.so         [.] _dl_relocate_object
      22.16%  perf        [kernel.kallsyms]  [k] kmem_cache_alloc
      14.29%  mem-memset  libc-2.17.so       [.] __memset_sse2
       2.21%  perf        [kernel.kallsyms]  [k] flush_signal_handlers
       0.07%  perf        [kernel.kallsyms]  [k] intel_pmu_enable_all
maintainer
#4 lore raw

Re: [PATCH 2/3] perf symbol: Implement a very simple ELF symbol parser

Em Mon, Nov 10, 2014 at 03:36:48PM +0900, Namhyung Kim escreveu:

Quoted 17 lines

Hi Arnaldo,

Quoted 5 lines · Arnaldo Carvalho de Melo

Em Fri, Nov 07, 2014 at 02:20:05PM +0900, Namhyung Kim escreveu:

Quoted 2 lines

It'll be used to show (userspace) symbol names when libelf isn't (or cannot be) linked.

Does this deals with prelink, etc?

I believe so. :)

Quoted 5 lines
Quoted 5 lines
 
  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  .........................
  #
      37.01%  mem-memcpy  libc-2.17.so       [.] __memcpy_ssse3_back
Quoted 5 lines
  namhyung@sejong:linux$ readelf -WS /lib64/libc-2.17.so | grep prelink
    [41] .gnu.prelink_undo PROGBITS        0000000000000000 200368 000c30 01      0   0  8
  namhyung@sejong:linux$ nm /lib64/libc-2.17.so | grep __memcpy_ssse3_back
  0000003153f46f40 t __memcpy_ssse3_back

Right, in this case most of the samples seems to map to what is expected for that workload, and the binary was prelinked. Good.

What about binaries that are not prelinked? IIRC there is code in the full blown ELF symbol-elf.c file to detect that and act accordingly, from a _very_ quick look I didn't saw it in this minimalistic ELF symtab reader, hence my question.

- Arnaldo

Quoted 10 lines

Thanks,
Namhyung

Quoted 5 lines
Quoted 5 lines
      24.25%  perf        ld-2.17.so         [.] _dl_relocate_object
      22.16%  perf        [kernel.kallsyms]  [k] kmem_cache_alloc
      14.29%  mem-memset  libc-2.17.so       [.] __memset_sse2
       2.21%  perf        [kernel.kallsyms]  [k] flush_signal_handlers
       0.07%  perf        [kernel.kallsyms]  [k] intel_pmu_enable_all
author maintainer
#5 lore raw

Re: [PATCH 2/3] perf symbol: Implement a very simple ELF symbol parser

Quoted 24 lines · Arnaldo Carvalho de Melo

Em Mon, Nov 10, 2014 at 03:36:48PM +0900, Namhyung Kim escreveu:

Quoted 17 lines

Hi Arnaldo,

Quoted 5 lines · Arnaldo Carvalho de Melo

Em Fri, Nov 07, 2014 at 02:20:05PM +0900, Namhyung Kim escreveu:

Quoted 2 lines

It'll be used to show (userspace) symbol names when libelf isn't (or cannot be) linked.

Does this deals with prelink, etc?

I believe so. :)

Quoted 5 lines
Quoted 5 lines
 
  # Overhead  Command     Shared Object      Symbol
  # ........  ..........  .................  .........................
  #
      37.01%  mem-memcpy  libc-2.17.so       [.] __memcpy_ssse3_back
Quoted 5 lines
  namhyung@sejong:linux$ readelf -WS /lib64/libc-2.17.so | grep prelink
    [41] .gnu.prelink_undo PROGBITS        0000000000000000 200368 000c30 01      0   0  8
  namhyung@sejong:linux$ nm /lib64/libc-2.17.so | grep __memcpy_ssse3_back
  0000003153f46f40 t __memcpy_ssse3_back
Quoted 7 lines

Right, in this case most of the samples seems to map to what is expected for that workload, and the binary was prelinked. Good.

What about binaries that are not prelinked? IIRC there is code in the full blown ELF symbol-elf.c file to detect that and act accordingly, from a _very_ quick look I didn't saw it in this minimalistic ELF symtab reader, hence my question.

AFAIK we deal with prelink'ed binary and normal binary as same way - we only cares about the file offsets.

  $ perf top --stdio
     PerfTop:     110 irqs/sec  kernel:70.0%  exact:  0.0% [4000Hz cycles],  (all, 12 CPUs)
  --------------------------------------------------------------------------------------------
     9.42%  perf           [.] map__process_kallsym_symbol       
     8.91%  [kernel]       [k] kallsyms_expand_symbol.constprop.1
     6.80%  [kernel]       [k] memcpy                            
     6.08%  [kernel]       [k] vsnprintf                         
     ...

Thanks,
Namhyung

author maintainer
#6 lore raw

PATCH 3/3 perf tools: Clean up libelf feature support code

In reply to Namhyung Kim

Current EXTLIBS contains -lelf by default and removes it when libelf is not detected. This is little bit confusing since we can now build perf without libelf so there's no need to handle it differently than other libraries.

Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/Makefile.perf   | 2 --
 tools/perf/config/Makefile | 5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)
tools/perf/Makefile.perf +0 -2
index 31a76ee92c93..06cccdceb0ae 100644
@@ -495,8 +495,6 @@ ifneq ($(OUTPUT),)
 endif
 
 ifdef NO_LIBELF
-EXTLIBS := $(filter-out -lelf,$(EXTLIBS))
-
 # Remove ELF/DWARF dependent codes
 LIB_OBJS := $(filter-out $(OUTPUT)util/symbol-elf.o,$(LIB_OBJS))
 LIB_OBJS := $(filter-out $(OUTPUT)util/dwarf-aux.o,$(LIB_OBJS))
tools/perf/config/Makefile +3 -2
index 79f906c7124e..5d4b039fe1ed 100644
@@ -150,7 +150,7 @@ CFLAGS += -std=gnu99
 # adding assembler files missing the .GNU-stack linker note.
 LDFLAGS += -Wl,-z,noexecstack
 
-EXTLIBS = -lelf -lpthread -lrt -lm -ldl
+EXTLIBS = -lpthread -lrt -lm -ldl
 
 ifneq ($(OUTPUT),)
   OUTPUT_FEATURES = $(OUTPUT)config/feature-checks/

@@ -354,6 +354,7 @@ endif # NO_LIBELF
 
 ifndef NO_LIBELF
   CFLAGS += -DHAVE_LIBELF_SUPPORT
+  EXTLIBS += -lelf
 
   ifeq ($(feature-libelf-mmap), 1)
     CFLAGS += -DHAVE_LIBELF_MMAP_SUPPORT

@@ -373,7 +374,7 @@ ifndef NO_LIBELF
     else
       CFLAGS += -DHAVE_DWARF_SUPPORT $(LIBDW_CFLAGS)
       LDFLAGS += $(LIBDW_LDFLAGS)
-      EXTLIBS += -lelf -ldw
+      EXTLIBS += -ldw
     endif # PERF_HAVE_DWARF_REGS
   endif # NO_DWARF
 endif # NO_LIBELF
Signature · 1 line
2.1.2
merge bot
#7 lore raw

[tip:perf/core] perf tools: Clean up libelf feature support code

perf tools: Clean up libelf feature support code
Commit-ID:  5e2d4d0e88dac4003cf96aca00d63aff2314391e
Gitweb:     http://git.kernel.org/tip/5e2d4d0e88dac4003cf96aca00d63aff2314391e
Author:     Namhyung Kim <[email protected]>
AuthorDate: Fri, 7 Nov 2014 14:20:06 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 19 Nov 2014 12:33:46 -0300

perf tools: Clean up libelf feature support code

Current EXTLIBS contains -lelf by default and removes it when libelf is not detected.

This is little bit confusing since we can now build perf without libelf so there's no need to handle it differently than other libraries.

Signed-off-by Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc David Ahern <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Peter Zijlstra <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/Makefile.perf   | 2 --
 tools/perf/config/Makefile | 5 +++--
 2 files changed, 3 insertions(+), 4 deletions(-)
tools/perf/Makefile.perf +0 -2
index aecf61dc..478efa9 100644
@@ -497,8 +497,6 @@ ifneq ($(OUTPUT),)
 endif
 
 ifdef NO_LIBELF
-EXTLIBS := $(filter-out -lelf,$(EXTLIBS))
-
 # Remove ELF/DWARF dependent codes
 LIB_OBJS := $(filter-out $(OUTPUT)util/symbol-elf.o,$(LIB_OBJS))
 LIB_OBJS := $(filter-out $(OUTPUT)util/dwarf-aux.o,$(LIB_OBJS))
tools/perf/config/Makefile +3 -2
index 79f906c..5d4b039 100644
@@ -150,7 +150,7 @@ CFLAGS += -std=gnu99
 # adding assembler files missing the .GNU-stack linker note.
 LDFLAGS += -Wl,-z,noexecstack
 
-EXTLIBS = -lelf -lpthread -lrt -lm -ldl
+EXTLIBS = -lpthread -lrt -lm -ldl
 
 ifneq ($(OUTPUT),)
   OUTPUT_FEATURES = $(OUTPUT)config/feature-checks/

@@ -354,6 +354,7 @@ endif # NO_LIBELF
 
 ifndef NO_LIBELF
   CFLAGS += -DHAVE_LIBELF_SUPPORT
+  EXTLIBS += -lelf
 
   ifeq ($(feature-libelf-mmap), 1)
     CFLAGS += -DHAVE_LIBELF_MMAP_SUPPORT

@@ -373,7 +374,7 @@ ifndef NO_LIBELF
     else
       CFLAGS += -DHAVE_DWARF_SUPPORT $(LIBDW_CFLAGS)
       LDFLAGS += $(LIBDW_LDFLAGS)
-      EXTLIBS += -lelf -ldw
+      EXTLIBS += -ldw
     endif # PERF_HAVE_DWARF_REGS
   endif # NO_DWARF
 endif # NO_LIBELF
maintainer
#8 lore raw
In reply to Namhyung Kim
Quoted 43 lines · Namhyung Kim

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

  # Overhead  Command  Shared Object      Symbol
  # ........  .......  .................  ........................
  #
      34.51%  swapper  [kernel.kallsyms]  [k] intel_idle
      12.54%  perf     [kernel.kallsyms]  [k] generic_exec_single
      10.11%  swapper  [kernel.kallsyms]  [k] int_sqrt
       9.83%  swapper  [kernel.kallsyms]  [k] hrtimer_interrupt
       7.25%  emacs    [kernel.kallsyms]  [k] __switch_to
       7.06%  sleep    [kernel.kallsyms]  [k] find_next_iomem_res
       7.02%  swapper  [kernel.kallsyms]  [k] run_timer_softirq
       2.55%  swapper  [kernel.kallsyms]  [k] _raw_spin_unlock_irq
       1.90%  swapper  [kernel.kallsyms]  [k] lapic_next_deadline
       1.75%  swapper  [kernel.kallsyms]  [k] native_sched_clock
       1.49%  swapper  [kernel.kallsyms]  [k] __schedule
       1.20%  swapper  [kernel.kallsyms]  [k] read_tsc
       1.10%  sleep    [kernel.kallsyms]  [k] current_kernel_time
       0.99%  swapper  [kernel.kallsyms]  [k] pick_next_task_rt
       0.36%  swapper  [kernel.kallsyms]  [k] group_sched_in
       0.24%  swapper  [kernel.kallsyms]  [k] x86_pmu_commit_txn
       0.06%  swapper  [kernel.kallsyms]  [k] intel_pmu_enable_all
       0.03%  perf     [kernel.kallsyms]  [k] intel_pmu_enable_all
Reported-by Peter Zijlstra <[email protected]>
Cc Adrian Hunter <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/symbol-minimal.c +0 -0
index c9541fea9514..226cf41ed7e6 100644

tools/perf/util/symbol-minimal.c int dso__load_sym(struct dso *dso, struct map *map __maybe_unused, · line 343

Quoted 9 lines
tools/perf/util/symbol-minimal.c +3 -0
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 	unsigned char *build_id[BUILD_ID_SIZE];
 	int ret;
 
+	if (dso->kernel)
+		return 0;  /* always use kallsyms */
+
 	ret = fd__is_64_bit(ss->fd);
 	if (ret >= 0)
 		dso->is_64_bit = ret;

Why does this live in the minimal implementation; should we not always discard ELF files with 0 symbols?

Suppose I have a vmlinux but removed all symbols from it; I want it to fall back to kallsyms too.

author maintainer
#9 lore raw

Hi Peter,

2014-11-07 (금), 09:27 +0100, Peter Zijlstra:

Quoted 57 lines · Namhyung Kim
Quoted 43 lines

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

  # Overhead  Command  Shared Object      Symbol
  # ........  .......  .................  ........................
  #
      34.51%  swapper  [kernel.kallsyms]  [k] intel_idle
      12.54%  perf     [kernel.kallsyms]  [k] generic_exec_single
      10.11%  swapper  [kernel.kallsyms]  [k] int_sqrt
       9.83%  swapper  [kernel.kallsyms]  [k] hrtimer_interrupt
       7.25%  emacs    [kernel.kallsyms]  [k] __switch_to
       7.06%  sleep    [kernel.kallsyms]  [k] find_next_iomem_res
       7.02%  swapper  [kernel.kallsyms]  [k] run_timer_softirq
       2.55%  swapper  [kernel.kallsyms]  [k] _raw_spin_unlock_irq
       1.90%  swapper  [kernel.kallsyms]  [k] lapic_next_deadline
       1.75%  swapper  [kernel.kallsyms]  [k] native_sched_clock
       1.49%  swapper  [kernel.kallsyms]  [k] __schedule
       1.20%  swapper  [kernel.kallsyms]  [k] read_tsc
       1.10%  sleep    [kernel.kallsyms]  [k] current_kernel_time
       0.99%  swapper  [kernel.kallsyms]  [k] pick_next_task_rt
       0.36%  swapper  [kernel.kallsyms]  [k] group_sched_in
       0.24%  swapper  [kernel.kallsyms]  [k] x86_pmu_commit_txn
       0.06%  swapper  [kernel.kallsyms]  [k] intel_pmu_enable_all
       0.03%  perf     [kernel.kallsyms]  [k] intel_pmu_enable_all
Reported-by Peter Zijlstra <[email protected]>
Cc Adrian Hunter <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/symbol-minimal.c +0 -0
index c9541fea9514..226cf41ed7e6 100644

tools/perf/util/symbol-minimal.c int dso__load_sym(struct dso *dso, struct map *map __maybe_unused, · line 343

Quoted 9 lines
tools/perf/util/symbol-minimal.c +3 -0
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 	unsigned char *build_id[BUILD_ID_SIZE];
 	int ret;
 
+	if (dso->kernel)
+		return 0;  /* always use kallsyms */
+
 	ret = fd__is_64_bit(ss->fd);
 	if (ret >= 0)
 		dso->is_64_bit = ret;

Why does this live in the minimal implementation; should we not always discard ELF files with 0 symbols?

Quoted 2 lines

Suppose I have a vmlinux but removed all symbols from it; I want it to fall back to kallsyms too.

I'm not sure I understood what you said correctly. With this change, dso__load_kernel_sym() always ends up calling dso__load_kallsyms() since dso__load_vmlinux() will always return 0;

So I think you'll fall back to kallsyms even though you have a vmlinux with symbol. This makes dso__load_sym() in the patch 2/3 simpler IMHO.

Thanks,
Namhyung

maintainer
#10 lore raw
Quoted 6 lines · Namhyung Kim
Quoted 5 lines

Why does this live in the minimal implementation; should we not always discard ELF files with 0 symbols?

Suppose I have a vmlinux but removed all symbols from it; I want it to fall back to kallsyms too.

Quoted 6 lines

I'm not sure I understood what you said correctly. With this change, dso__load_kernel_sym() always ends up calling dso__load_kallsyms() since dso__load_vmlinux() will always return 0;

So I think you'll fall back to kallsyms even though you have a vmlinux with symbol. This makes dso__load_sym() in the patch 2/3 simpler IMHO.

But why have it specific to the minimal elf thing? Why not discard any DSO with 0 symbols and try the next option to acquire symbols?

author maintainer
#11 lore raw

Hi Peter,

Quoted 12 lines · Peter Zijlstra
Quoted 10 lines · Namhyung Kim
Quoted 5 lines

Why does this live in the minimal implementation; should we not always discard ELF files with 0 symbols?

Suppose I have a vmlinux but removed all symbols from it; I want it to fall back to kallsyms too.

I'm not sure I understood what you said correctly. With this change, dso__load_kernel_sym() always ends up calling dso__load_kallsyms() since dso__load_vmlinux() will always return 0;

Quoted 5 lines
Quoted 2 lines

So I think you'll fall back to kallsyms even though you have a vmlinux with symbol. This makes dso__load_sym() in the patch 2/3 simpler IMHO.

But why have it specific to the minimal elf thing? Why not discard any DSO with 0 symbols and try the next option to acquire symbols?

Hmm.. I don't think it's specific to the minimal elf parser. The return value of dso__load_sym() is a number of symbols found so when it sees a dso with 0 symbols it'll fall back to the next option IMHO (not tested). Did you see a problem with the current code?

Thanks,
Namhyung

maintainer
#12 lore raw
Quoted 4 lines · Namhyung Kim

Hmm.. I don't think it's specific to the minimal elf parser. The return value of dso__load_sym() is a number of symbols found so when it sees a dso with 0 symbols it'll fall back to the next option IMHO (not tested). Did you see a problem with the current code?

So your patch:

+++ b/tools/perf/util/symbol-minimal.c
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,

        unsigned char *build_id[BUILD_ID_SIZE];
        int ret;

+ if (dso->kernel)
+ return 0; /* always use kallsyms */
+

changes the symbol-minimal.c file to add this exception. That is very much specific to the minimal elf parser, or am I just seeing things?

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

author maintainer
#13 lore raw

Hi Peter,

Quoted 17 lines · Peter Zijlstra
Quoted 4 lines · Namhyung Kim

Hmm.. I don't think it's specific to the minimal elf parser. The return value of dso__load_sym() is a number of symbols found so when it sees a dso with 0 symbols it'll fall back to the next option IMHO (not tested). Did you see a problem with the current code?

So your patch:

+++ b/tools/perf/util/symbol-minimal.c

tools/perf/util/symbol-minimal.c +3 -0
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
        unsigned char *build_id[BUILD_ID_SIZE];
        int ret;
 
+       if (dso->kernel)
+               return 0;  /* always use kallsyms */
+
Quoted 2 lines

changes the symbol-minimal.c file to add this exception. That is very much specific to the minimal elf parser, or am I just seeing things?

What minimal parser does here is just skip kernel dsos (vmlinux) since it didn't deal with all the details of parsing vmlinux currently.

Quoted 3 lines

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

The util/symbol.c doesn't need this because it can handle vmlinux reliably. So after reading symbol table, it'll use the dso if it actually contains symbols or fallback to next dso if it has 0 symbols. IOW it already disregards all dsos with 0 symbols in.

Signature · 2 lines
Thanks,
Namhyung
maintainer
#14 lore raw
Quoted 7 lines · Namhyung Kim
Quoted 2 lines

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

The util/symbol.c doesn't need this because it can handle vmlinux reliably. So after reading symbol table, it'll use the dso if it actually contains symbols or fallback to next dso if it has 0 symbols. IOW it already disregards all dsos with 0 symbols in.

Well, it could not could it.. it readily proceeded with 0 symbols in my case. It did _not_ fallback to kallsyms.

maintainer
#15 lore raw

Em Tue, Nov 11, 2014 at 11:27:45AM +0100, Peter Zijlstra escreveu:

Quoted 11 lines · Namhyung Kim
Quoted 7 lines
Quoted 2 lines

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

The util/symbol.c doesn't need this because it can handle vmlinux reliably. So after reading symbol table, it'll use the dso if it actually contains symbols or fallback to next dso if it has 0 symbols. IOW it already disregards all dsos with 0 symbols in.

Well, it could not could it.. it readily proceeded with 0 symbols in my case. It did _not_ fallback to kallsyms.

Right, testing a fix for the problem I described on my last message.

- Arnaldo

maintainer
#16 lore raw

Em Tue, Nov 11, 2014 at 10:03:26AM -0300, Arnaldo Carvalho de Melo escreveu:

Quoted 10 lines

Em Tue, Nov 11, 2014 at 11:27:45AM +0100, Peter Zijlstra escreveu:

Quoted 8 lines · Namhyung Kim
Quoted 7 lines
Quoted 2 lines

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

The util/symbol.c doesn't need this because it can handle vmlinux reliably. So after reading symbol table, it'll use the dso if it actually contains symbols or fallback to next dso if it has 0 symbols. IOW it already disregards all dsos with 0 symbols in.

Quoted 4 lines
Quoted 2 lines

Well, it could not could it.. it readily proceeded with 0 symbols in my case. It did _not_ fallback to kallsyms.

Right, testing a fix for the problem I described on my last message.

Can you try the patch below?

- Arnaldo

tools/perf/util/symbol-minimal.c +1 -1
index c9541fea9514..b55f96d49503 100644
@@ -341,7 +341,7 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 
 	if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) > 0) {
 		dso__set_build_id(dso, build_id);
-		return 1;
+		return 0;
 	}
 	return 0;
 }
maintainer
#17 lore raw
Quoted 8 lines · Arnaldo Carvalho de Melo

Can you try the patch below?

tools/perf/util/symbol-minimal.c +0 -0
index c9541fea9514..b55f96d49503 100644

tools/perf/util/symbol-minimal.c int dso__load_sym(struct dso *dso, struct map *map __maybe_unused, · line 347

Quoted 8 lines
tools/perf/util/symbol-minimal.c +1 -1
@@ -341,7 +341,7 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 
 	if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) > 0) {
 		dso__set_build_id(dso, build_id);
-		return 1;
+		return 0;
 	}
 	return 0;
 }

removing libelf-dev and recompiling to verify its broken again, then I made this change, recompiled and tried again and that seems to work.

Control flow seems redundant though, might as well remove the entire return stmt.

Still wondering why the generic code accepts DSOs without symbols though. But whatever ;-)

merge bot
#18 lore raw

[tip:perf/core] perf symbols: Fallback to kallsyms when using the minimal 'ELF' loader

In reply to Arnaldo Carvalho de Melo
perf symbols: Fallback to kallsyms when using the minimal 'ELF' loader
Commit-ID:  162bcc17bb876772793ca070ebd6488cfdae09bf
Gitweb:     http://git.kernel.org/tip/162bcc17bb876772793ca070ebd6488cfdae09bf
Author:     Arnaldo Carvalho de Melo <[email protected]>
AuthorDate: Tue, 11 Nov 2014 11:25:28 -0300
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Wed, 19 Nov 2014 12:33:46 -0300

perf symbols: Fallback to kallsyms when using the minimal 'ELF' loader

The minimal ELF loader should not return 1 when it manages to read the vmlinux build-id, it should instead return 0, meaning that it hasn't loaded any symbols, since it doesn't parses ELF at all.

That way, the main symbol.c routines will understand that it is necessary to continue looking for a file with symbols, and when no libelf is linked, that means it will eventually try kallsyms.

Reported-by Peter Zijlstra <[email protected]>
Tested-by Peter Zijlstra <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc David Ahern <[email protected]>
Cc Don Zickus <[email protected]>
Cc Frederic Weisbecker <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Stephane Eranian <[email protected]>
Link http://lkml.kernel.org/r/[email protected] open here
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 1 -
 1 file changed, 1 deletion(-)
tools/perf/util/symbol-minimal.c +0 -1
index c9541fe..fa585c6 100644
@@ -341,7 +341,6 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 
 	if (filename__read_build_id(ss->name, build_id, BUILD_ID_SIZE) > 0) {
 		dso__set_build_id(dso, build_id);
-		return 1;
 	}
 	return 0;
 }
maintainer
#19 lore raw
In reply to Namhyung Kim

Em Tue, Nov 11, 2014 at 01:24:38PM +0900, Namhyung Kim escreveu:

Quoted 28 lines

Hi Peter,

Quoted 17 lines · Peter Zijlstra
Quoted 4 lines · Namhyung Kim

Hmm.. I don't think it's specific to the minimal elf parser. The return value of dso__load_sym() is a number of symbols found so when it sees a dso with 0 symbols it'll fall back to the next option IMHO (not tested). Did you see a problem with the current code?

So your patch:

+++ b/tools/perf/util/symbol-minimal.c

(quoted diff) +3 -0
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
        unsigned char *build_id[BUILD_ID_SIZE];
        int ret;
 
+       if (dso->kernel)
+               return 0;  /* always use kallsyms */
+
Quoted 2 lines

changes the symbol-minimal.c file to add this exception. That is very much specific to the minimal elf parser, or am I just seeing things?

What minimal parser does here is just skip kernel dsos (vmlinux) since it didn't deal with all the details of parsing vmlinux currently.

Quoted 1 line
Quoted 5 lines
Quoted 2 lines

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

The util/symbol.c doesn't need this because it can handle vmlinux reliably. So after reading symbol table, it'll use the dso if it

symbol.c should not be able to handle anything, since the actual ELF loader is in symbol-elf.c or simbol-minimal.c, no?

I.e.:

	symbol.c::dso__load()
		symbol.c::dso__load_kernel_sym()
			symbol.c::dso__load_vmlinux()

And then it heads into one of the ELF loaders, right now, without your patch, I see, when the minimal loader is used:

		symbol-minimal.c::dso__load_sym()

reads the build-id and if it works, returns 1, which is a bug, it should return 0 in this case, possibly -1 if it doesn't read the build-id :-\

I.e. it should return 0, because that way it signals: "I can't read it, thus no symbols were loaded, symbol.c: please go on looking for them somewhere else, kallsyms perhaps?".

And this is the bug, probably. Now to look at your patches to see if this is touched somehow...

Quoted 2 lines

actually contains symbols or fallback to next dso if it has 0 symbols. IOW it already disregards all dsos with 0 symbols in.

See above.

Quoted 3 lines
Signature · 2 lines
Thanks,
Namhyung
author maintainer
#20 lore raw

Hi Arnaldo,

Sorry for late reply. I was offline last week.

Quoted 29 lines · Arnaldo Carvalho de Melo

Em Tue, Nov 11, 2014 at 01:24:38PM +0900, Namhyung Kim escreveu:

Quoted 28 lines

Hi Peter,

Quoted 17 lines · Peter Zijlstra
Quoted 4 lines · Namhyung Kim

Hmm.. I don't think it's specific to the minimal elf parser. The return value of dso__load_sym() is a number of symbols found so when it sees a dso with 0 symbols it'll fall back to the next option IMHO (not tested). Did you see a problem with the current code?

So your patch:

+++ b/tools/perf/util/symbol-minimal.c

(quoted diff) +3 -0
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
        unsigned char *build_id[BUILD_ID_SIZE];
        int ret;
 
+       if (dso->kernel)
+               return 0;  /* always use kallsyms */
+
Quoted 2 lines

changes the symbol-minimal.c file to add this exception. That is very much specific to the minimal elf parser, or am I just seeing things?

What minimal parser does here is just skip kernel dsos (vmlinux) since it didn't deal with all the details of parsing vmlinux currently.

Quoted 1 line
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

What I was saying, why not have a util/symbol.c change that disregards all DSOs with 0 symbols in.

The util/symbol.c doesn't need this because it can handle vmlinux reliably. So after reading symbol table, it'll use the dso if it

symbol.c should not be able to handle anything, since the actual ELF loader is in symbol-elf.c or simbol-minimal.c, no?

Ah right, I was confused with symbol.c and symbol-elf.c - so I believe the generic logic in symbol.c already handles DSOs with 0 symbol. The problem was symbol-minimal.c didn't return 0 for the DSOs.

Quoted 10 lines

I.e.:

	symbol.c::dso__load()
		symbol.c::dso__load_kernel_sym()
			symbol.c::dso__load_vmlinux()

And then it heads into one of the ELF loaders, right now, without your patch, I see, when the minimal loader is used:

Quoted 8 lines
		symbol-minimal.c::dso__load_sym()

reads the build-id and if it works, returns 1, which is a bug, it should return 0 in this case, possibly -1 if it doesn't read the build-id :-\

I.e. it should return 0, because that way it signals: "I can't read it, thus no symbols were loaded, symbol.c: please go on looking for them somewhere else, kallsyms perhaps?".

Right. It's a bug for vmlinux. But by returning 0, it'll iterate over the candidate binaries in dso__load(). I'm afraid that this *might* overwrite the build-id of the DSO with different one. So I think it also needs to add a check for valid build-id.

I'll update this patch with above.

Thanks,
Namhyung

Quoted 12 lines

And this is the bug, probably. Now to look at your patches to see if this is touched somehow...

Quoted 2 lines

actually contains symbols or fallback to next dso if it has 0 symbols. IOW it already disregards all dsos with 0 symbols in.

See above.

Quoted 3 lines
Signature · 2 lines
Thanks,
Namhyung
#21 lore raw
In reply to Namhyung Kim
Quoted 2 lines · Namhyung Kim

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

Do you know the recent change (commit id) that broke it?

David

author maintainer
#22 lore raw

Hi David,

Quoted 5 lines · David Ahern
Quoted 2 lines · Namhyung Kim

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

Do you know the recent change (commit id) that broke it?

Nope. I tried to find it but failed - older versions seems also have the problem and some of them could not run perf record. Anyway it looks like depending some external condition and broken for a while. :(

Thanks,
Namhyung

#23 lore raw
Quoted 4 lines · Namhyung Kim

Hi David,

Quoted 1 line · David Ahern

On 11/6/14, 10:20 PM, Namhyung Kim wrote:

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

Do you know the recent change (commit id) that broke it?

Nope. I tried to find it but failed - older versions seems also have the problem and some of them could not run perf record. Anyway it looks like depending some external condition and broken for a while. :(

OK. I have a segfault issue when analyzing a mips file offbox and it is related to kernel symbol processing. I was not able to find past version of perf that worked - though limited by the file version checks added around v3.6.

David

maintainer
#24 lore raw
In reply to Namhyung Kim

Em Fri, Nov 07, 2014 at 02:20:04PM +0900, Namhyung Kim escreveu:

Quoted 8 lines

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

Ok, but since you added that minimal ELF symtab loading, isn't better to try that first, i.e. if we find a vmlinux file with a build-id and with symbols in it...

If that isn't the case, i.e. no vmlinux was found, then we fallback to kallsyms, to check if that is available.

I.e. with your new minimalistic ELF symtab loader if we have a suitable vmlinux but no kallsyms, we end up resolving no symbols even having that nice vmlinux :-)

Ah, I applied the feature test one (2/3, I think)

- Arnaldo

Quoted 45 lines
  # Overhead  Command  Shared Object      Symbol
  # ........  .......  .................  ........................
  #
      34.51%  swapper  [kernel.kallsyms]  [k] intel_idle
      12.54%  perf     [kernel.kallsyms]  [k] generic_exec_single
      10.11%  swapper  [kernel.kallsyms]  [k] int_sqrt
       9.83%  swapper  [kernel.kallsyms]  [k] hrtimer_interrupt
       7.25%  emacs    [kernel.kallsyms]  [k] __switch_to
       7.06%  sleep    [kernel.kallsyms]  [k] find_next_iomem_res
       7.02%  swapper  [kernel.kallsyms]  [k] run_timer_softirq
       2.55%  swapper  [kernel.kallsyms]  [k] _raw_spin_unlock_irq
       1.90%  swapper  [kernel.kallsyms]  [k] lapic_next_deadline
       1.75%  swapper  [kernel.kallsyms]  [k] native_sched_clock
       1.49%  swapper  [kernel.kallsyms]  [k] __schedule
       1.20%  swapper  [kernel.kallsyms]  [k] read_tsc
       1.10%  sleep    [kernel.kallsyms]  [k] current_kernel_time
       0.99%  swapper  [kernel.kallsyms]  [k] pick_next_task_rt
       0.36%  swapper  [kernel.kallsyms]  [k] group_sched_in
       0.24%  swapper  [kernel.kallsyms]  [k] x86_pmu_commit_txn
       0.06%  swapper  [kernel.kallsyms]  [k] intel_pmu_enable_all
       0.03%  perf     [kernel.kallsyms]  [k] intel_pmu_enable_all
Reported-by Peter Zijlstra <[email protected]>
Cc Adrian Hunter <[email protected]>
Signed-off-by Namhyung Kim <[email protected]>
---
 tools/perf/util/symbol-minimal.c | 3 +++
 1 file changed, 3 insertions(+)
tools/perf/util/symbol-minimal.c +3 -0
index c9541fea9514..226cf41ed7e6 100644
@@ -335,6 +335,9 @@ int dso__load_sym(struct dso *dso, struct map *map __maybe_unused,
 	unsigned char *build_id[BUILD_ID_SIZE];
 	int ret;
 
+	if (dso->kernel)
+		return 0;  /* always use kallsyms */
+
 	ret = fd__is_64_bit(ss->fd);
 	if (ret >= 0)
 		dso->is_64_bit = ret;
Signature · 1 line
2.1.2
author maintainer
#25 lore raw
Quoted 14 lines · Arnaldo Carvalho de Melo

Em Fri, Nov 07, 2014 at 02:20:04PM +0900, Namhyung Kim escreveu:

Quoted 8 lines

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

Ok, but since you added that minimal ELF symtab loading, isn't better to try that first, i.e. if we find a vmlinux file with a build-id and with symbols in it...

Quoted 6 lines

If that isn't the case, i.e. no vmlinux was found, then we fallback to kallsyms, to check if that is available.

I.e. with your new minimalistic ELF symtab loader if we have a suitable vmlinux but no kallsyms, we end up resolving no symbols even having that nice vmlinux :-)

Yeah, maybe. But it'd add a substantial complexity also and I tried to make it simple and small only to show usual userspace symbols.

I think that about a half of the complexity of the dso__load_sym() in symbol-elf.c came from the kernel (and module) support. And expecting kallsyms on the system is not a high barrier IMHO. So I decided to just fall back to kallsyms for kernel dsos. Mayby we can add the kernel support incrementally later.

Thanks,
Namhyung

maintainer
#26 lore raw

Em Mon, Nov 10, 2014 at 03:53:06PM +0900, Namhyung Kim escreveu:

Quoted 22 lines · Arnaldo Carvalho de Melo
Quoted 14 lines

Em Fri, Nov 07, 2014 at 02:20:04PM +0900, Namhyung Kim escreveu:

Quoted 8 lines

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

Ok, but since you added that minimal ELF symtab loading, isn't better to try that first, i.e. if we find a vmlinux file with a build-id and with symbols in it...

Quoted 6 lines

If that isn't the case, i.e. no vmlinux was found, then we fallback to kallsyms, to check if that is available.

I.e. with your new minimalistic ELF symtab loader if we have a suitable vmlinux but no kallsyms, we end up resolving no symbols even having that nice vmlinux :-)

Quoted 5 lines

Yeah, maybe. But it'd add a substantial complexity also and I tried to make it simple and small only to show usual userspace symbols.

I think that about a half of the complexity of the dso__load_sym() in symbol-elf.c came from the kernel (and module) support. And expecting

Right, I agree that thing grew too complex, reducing that complexity is something we should do when we have the chance. I.e. perhaps we could separate kernel/modules handling out of it, even if just at source code level at first...

Quoted 3 lines

kallsyms on the system is not a high barrier IMHO. So I decided to just fall back to kallsyms for kernel dsos. Mayby we can add the kernel support incrementally later.

The suggestion came because you did that minimalistic reader, which if can be used for vmlinux files, would be great, as resolving vmlinux samples is such a major usecase :-)

- Arnaldo

author maintainer
#27 lore raw
Quoted 26 lines · Arnaldo Carvalho de Melo

Em Mon, Nov 10, 2014 at 03:53:06PM +0900, Namhyung Kim escreveu:

Quoted 25 lines · Arnaldo Carvalho de Melo
Quoted 14 lines

Em Fri, Nov 07, 2014 at 02:20:04PM +0900, Namhyung Kim escreveu:

Quoted 8 lines

When libelf is not used, perf cannot not show symbol names since it doesn't access the ELF symbol table. But kernel is different as it can fallback to kallsyms.

It worked w/o libelf, but recent change to use vmlinux for kernel symbols break it.

With this change, it now can show kernel symbols again:

Ok, but since you added that minimal ELF symtab loading, isn't better to try that first, i.e. if we find a vmlinux file with a build-id and with symbols in it...

Quoted 6 lines

If that isn't the case, i.e. no vmlinux was found, then we fallback to kallsyms, to check if that is available.

I.e. with your new minimalistic ELF symtab loader if we have a suitable vmlinux but no kallsyms, we end up resolving no symbols even having that nice vmlinux :-)

Yeah, maybe. But it'd add a substantial complexity also and I tried to make it simple and small only to show usual userspace symbols.

Quoted 7 lines
Quoted 2 lines

I think that about a half of the complexity of the dso__load_sym() in symbol-elf.c came from the kernel (and module) support. And expecting

Right, I agree that thing grew too complex, reducing that complexity is something we should do when we have the chance. I.e. perhaps we could separate kernel/modules handling out of it, even if just at source code level at first...

Hmm.. okay. I'll try.

Thanks,
Namhyung

Quoted 8 lines
Quoted 4 lines

kallsyms on the system is not a high barrier IMHO. So I decided to just fall back to kallsyms for kernel dsos. Mayby we can add the kernel support incrementally later.

The suggestion came because you did that minimalistic reader, which if can be used for vmlinux files, would be great, as resolving vmlinux samples is such a major usecase :-)