perf tool: Assorted fixes

PATCH Superseded Jiri Olsa 3 participants 13 messages Feb 7, 2018 – Feb 8, 2018 perf events

lore

Share & tools

lore

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #0 lore raw

v2 cover perf tool: Assorted fixes

hi,
sending assorted general fixes that queued
up in my other branches.

v2 changes:
- rebased on current perf/core
- detailed changelog for patch 1
- using zfree instead of free in patch 2
- using machine__set_mmap_name function name

Also available in here:
https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
perf/fixes

thanks,
jirka

---
Jiri Olsa (10):
tools lib symbol: Skip non-address kallsyms line
perf tools: Free root_dir in machine__init error path
perf tools: Move kernel mmap name into struct machine
perf tools: Don't search for active kernel start in __machine__create_kernel_maps perf tools: Generalize machine__set_kernel_mmap function
perf tools: Use machine__set_kernel_mmap instead of map_groups__fixup_end perf tools: Rename __map_groups__fixup_end to map_groups__fixup_end perf tools: Remove machine__load_kallsyms function
perf tools: Do not create kernel maps in sample__resolve
perf tools: Check if we read regular file in dso__load

 tools/lib/symbol/kallsyms.c         |   4 ++++
 tools/perf/tests/vmlinux-kallsyms.c |   2 +-
 tools/perf/util/build-id.c          |  10 +++------
 tools/perf/util/event.c             |  16 +-------------
 tools/perf/util/machine.c           | 147 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------------------------------------------------------------------
 tools/perf/util/machine.h           |   6 +-----
 tools/perf/util/symbol-elf.c        |   2 +-
 tools/perf/util/symbol.c            |  20 ++++++-----------
 tools/perf/util/symbol.h            |   2 +-
 9 files changed, 86 insertions(+), 123 deletions(-)

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #1 lore raw

PATCH v2 1/10 tools lib symbol: Skip non-address kallsyms line

Adding check on failed attempt to parse the address and skip the line parsing early in that case.

The address can be replaced with '(null)' string in case user don't have enough permissions, like:

  $ cat /proc/kallsyms
      (null) A irq_stack_union
      (null) A __per_cpu_start
      ...

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/symbol/kallsyms.c | 4 ++++
 1 file changed, 4 insertions(+)

tools/lib/symbol/kallsyms.c +4 -0

index 914cb8e3d40b..689b6a130dd7 100644
@@ -38,6 +38,10 @@ int kallsyms__parse(const char *filename, void *arg,
 
 		len = hex2u64(line, &start);
 
+		/* Skip the line if we failed to parse the address. */
+		if (!len)
+			continue;
+
 		len++;
 		if (len + 2 >= line_len)
 			continue;

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #2 lore raw

PATCH v2 2/10 perf tools: Free root_dir in machine__init error path

In reply to Jiri Olsa

Freeing root_dir in machine__init error path.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

tools/perf/util/machine.c +7 -1

index b05a67464c03..c976384f9022 100644
@@ -50,6 +50,8 @@ static void machine__threads_init(struct machine *machine)
 
 int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
 {
+	int err = -ENOMEM;
+
 	memset(machine, 0, sizeof(*machine));
 	map_groups__init(&machine->kmaps, machine);
 	RB_CLEAR_NODE(&machine->rb_node);

@@ -79,7 +81,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
 		char comm[64];
 
 		if (thread == NULL)
-			return -ENOMEM;
+			goto out;
 
 		snprintf(comm, sizeof(comm), "[guest/%d]", pid);
 		thread__set_comm(thread, comm, 0);

@@ -87,7 +89,11 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid)
 	}
 
 	machine->current_tid = NULL;
+	err = 0;
 
+out:
+	if (err)
+		zfree(&machine->root_dir);
 	return 0;
 }

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:50 UTC #3 lore raw

PATCH v2 3/10 perf tools: Move kernel mmap name into struct machine

In reply to Jiri Olsa

It simplifies and centralizes the code. The kernel mmap name is set for machine type, which we know from the beginning, so there's no reason to generate it every time we need it.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/build-id.c | 10 +++----
 tools/perf/util/event.c    |  5 +---
 tools/perf/util/machine.c  | 67 +++++++++++++++++++++++-----------------------
 tools/perf/util/machine.h  |  3 +--
 tools/perf/util/symbol.c   |  3 +--
 5 files changed, 39 insertions(+), 49 deletions(-)

tools/perf/util/build-id.c +3 -7

index 7f8553630c4d..537eadd81914 100644
@@ -316,7 +316,6 @@ static int machine__write_buildid_table(struct machine *machine,
 					struct feat_fd *fd)
 {
 	int err = 0;
-	char nm[PATH_MAX];
 	struct dso *pos;
 	u16 kmisc = PERF_RECORD_MISC_KERNEL,
 	    umisc = PERF_RECORD_MISC_USER;

@@ -338,9 +337,8 @@ static int machine__write_buildid_table(struct machine *machine,
 			name = pos->short_name;
 			name_len = pos->short_name_len;
 		} else if (dso__is_kcore(pos)) {
-			machine__mmap_name(machine, nm, sizeof(nm));
-			name = nm;
-			name_len = strlen(nm);
+			name = machine->mmap_name;
+			name_len = strlen(name);
 		} else {
 			name = pos->long_name;
 			name_len = pos->long_name_len;

@@ -813,12 +811,10 @@ static int dso__cache_build_id(struct dso *dso, struct machine *machine)
 	bool is_kallsyms = dso__is_kallsyms(dso);
 	bool is_vdso = dso__is_vdso(dso);
 	const char *name = dso->long_name;
-	char nm[PATH_MAX];
 
 	if (dso__is_kcore(dso)) {
 		is_kallsyms = true;
-		machine__mmap_name(machine, nm, sizeof(nm));
-		name = nm;
+		name = machine->mmap_name;
 	}
 	return build_id_cache__add_b(dso->build_id, sizeof(dso->build_id), name,
 				     dso->nsinfo, is_kallsyms, is_vdso);

tools/perf/util/event.c +1 -4

index 44e603c27944..4644e751a3e3 100644
@@ -894,8 +894,6 @@ int perf_event__synthesize_kernel_mmap(struct perf_tool *tool,
 				       struct machine *machine)
 {
 	size_t size;
-	const char *mmap_name;
-	char name_buff[PATH_MAX];
 	struct map *map = machine__kernel_map(machine);
 	struct kmap *kmap;
 	int err;

@@ -918,7 +916,6 @@ int perf_event__synthesize_kernel_mmap(struct perf_tool *tool,
 		return -1;
 	}
 
-	mmap_name = machine__mmap_name(machine, name_buff, sizeof(name_buff));
 	if (machine__is_host(machine)) {
 		/*
 		 * kernel uses PERF_RECORD_MISC_USER for user space maps,

@@ -931,7 +928,7 @@ int perf_event__synthesize_kernel_mmap(struct perf_tool *tool,
 
 	kmap = map__kmap(map);
 	size = snprintf(event->mmap.filename, sizeof(event->mmap.filename),
-			"%s%s", mmap_name, kmap->ref_reloc_sym->name) + 1;
+			"%s%s", machine->mmap_name, kmap->ref_reloc_sym->name) + 1;
 	size = PERF_ALIGN(size, sizeof(u64));
 	event->mmap.header.type = PERF_RECORD_MMAP;
 	event->mmap.header.size = (sizeof(event->mmap) -

tools/perf/util/machine.c +33 ~~-34~~

140 lines not on this page · raw message

tools/perf/util/machine.h +1 -2

index 5ce860b64c74..cb0a20f3a96b 100644
@@ -43,6 +43,7 @@ struct machine {
 	bool		  comm_exec;
 	bool		  kptr_restrict_warned;
 	char		  *root_dir;
+	char		  *mmap_name;
 	struct threads    threads[THREADS__TABLE_SIZE];
 	struct vdso_info  *vdso_info;
 	struct perf_env   *env;

@@ -142,8 +143,6 @@ struct machine *machines__find(struct machines *machines, pid_t pid);
 struct machine *machines__findnew(struct machines *machines, pid_t pid);
 
 void machines__set_id_hdr_size(struct machines *machines, u16 id_hdr_size);
-char *machine__mmap_name(struct machine *machine, char *bf, size_t size);
-
 void machines__set_comm_exec(struct machines *machines, bool comm_exec);
 
 struct machine *machine__new_host(void);

tools/perf/util/symbol.c +1 -2

index cc065d4bfafc..dcb81176669a 100644
@@ -1960,8 +1960,7 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
 		pr_debug("Using %s for symbols\n", kallsyms_filename);
 	if (err > 0 && !dso__is_kcore(dso)) {
 		dso->binary_type = DSO_BINARY_TYPE__GUEST_KALLSYMS;
-		machine__mmap_name(machine, path, sizeof(path));
-		dso__set_long_name(dso, strdup(path), true);
+		dso__set_long_name(dso, machine->mmap_name, false);
 		map__fixup_start(map);
 		map__fixup_end(map);
 	}

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:50 UTC #4 lore raw

PATCH v2 4/10 perf tools: Don't search for active kernel start in __machine__create_kernel_maps

In reply to Jiri Olsa

We should not search for kernel start address in __machine__create_kernel_maps function, because it's being used in 'report' code path, where we are interested in kernel MMAP data address instead of in current kernel address

The __machine__create_kernel_maps serves purely for creating the machines kernel maps and setting up the kmap group. The report code path then sets the address based on the data from kernel MMAP event in machine__set_kernel_mmap function.

The kallsyms search address logic is used for test code, that calls machine__create_kernel_maps to get current maps and calls machine__get_running_kernel_start to get kernel starting address.

Also making __machine__create_kernel_maps static, because there's no external user.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 6 ++----
 tools/perf/util/machine.h | 1 -
 2 files changed, 2 insertions(+), 5 deletions(-)

tools/perf/util/machine.c +2 -4

index b1f1961b13f4..da85666f3a02 100644
@@ -856,14 +856,12 @@ static int machine__get_running_kernel_start(struct machine *machine,
 	return 0;
 }
 
-int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
+static int
+__machine__create_kernel_maps(struct machine *machine, struct dso *kernel)
 {
 	int type;
 	u64 start = 0;
 
-	if (machine__get_running_kernel_start(machine, NULL, &start))
-		return -1;
-
 	/* In case of renewal the kernel map, destroy previous one */
 	machine__destroy_kernel_maps(machine);

tools/perf/util/machine.h +0 -1

index cb0a20f3a96b..50d587d34459 100644
@@ -238,7 +238,6 @@ size_t machines__fprintf_dsos_buildid(struct machines *machines, FILE *fp,
 				     bool (skip)(struct dso *dso, int parm), int parm);
 
 void machine__destroy_kernel_maps(struct machine *machine);
-int __machine__create_kernel_maps(struct machine *machine, struct dso *kernel);
 int machine__create_kernel_maps(struct machine *machine);
 
 int machines__create_kernel_maps(struct machines *machines, pid_t pid);

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #5 lore raw

PATCH v2 5/10 perf tools: Generalize machine__set_kernel_mmap function

In reply to Jiri Olsa

So it could be called without event object, just with start and end values. It will be used in following patch.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

tools/perf/util/machine.c +7 -6

index da85666f3a02..7563b750137d 100644
@@ -1260,15 +1260,15 @@ int machine__create_kernel_maps(struct machine *machine)
 	return 0;
 }
 
-static void machine__set_kernel_mmap_len(struct machine *machine,
-					 union perf_event *event)
+static void machine__set_kernel_mmap(struct machine *machine,
+				     u64 start, u64 end)
 {
 	int i;
 
 	for (i = 0; i < MAP__NR_TYPES; i++) {
-		machine->vmlinux_maps[i]->start = event->mmap.start;
-		machine->vmlinux_maps[i]->end   = (event->mmap.start +
-						   event->mmap.len);
+		machine->vmlinux_maps[i]->start = start;
+		machine->vmlinux_maps[i]->end   = end;
+
 		/*
 		 * Be a bit paranoid here, some perf.data file came with
 		 * a zero sized synthesized MMAP event for the kernel.

@@ -1373,7 +1373,8 @@ static int machine__process_kernel_mmap_event(struct machine *machine,
 		if (strstr(kernel->long_name, "vmlinux"))
 			dso__set_short_name(kernel, "[kernel.vmlinux]", false);
 
-		machine__set_kernel_mmap_len(machine, event);
+		machine__set_kernel_mmap(machine, event->mmap.start,
+					 event->mmap.start + event->mmap.len);
 
 		/*
 		 * Avoid using a zero address (kptr_restrict) for the ref reloc

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #6 lore raw

PATCH v2 6/10 perf tools: Use machine__set_kernel_mmap instead of map_groups__fixup_end

In reply to Jiri Olsa

The machine__set_kernel_mmap does the same job as map_groups__fixup_end when used on kernel maps within machine__create_kernel_maps call.

This way we can also remove map_groups__fixup_end function, because there's no user to it. Also moving machine__set_kernel_mmap up in code, so we don't need forward declaration.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 49 ++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

tools/perf/util/machine.c +19 ~~-30~~

index 7563b750137d..49ebd451151a 100644
@@ -1028,13 +1028,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
 	return ret;
 }
 
-static void map_groups__fixup_end(struct map_groups *mg)
-{
-	int i;
-	for (i = 0; i < MAP__NR_TYPES; ++i)
-		__map_groups__fixup_end(mg, i);
-}
-
 static char *get_kernel_version(const char *root_dir)
 {
 	char version[PATH_MAX];

@@ -1220,6 +1213,24 @@ static int machine__create_modules(struct machine *machine)
 	return 0;
 }
 
+static void machine__set_kernel_mmap(struct machine *machine,
+				     u64 start, u64 end)
+{
+	int i;
+
+	for (i = 0; i < MAP__NR_TYPES; i++) {
+		machine->vmlinux_maps[i]->start = start;
+		machine->vmlinux_maps[i]->end   = end;
+
+		/*
+		 * Be a bit paranoid here, some perf.data file came with
+		 * a zero sized synthesized MMAP event for the kernel.
+		 */
+		if (machine->vmlinux_maps[i]->end == 0)
+			machine->vmlinux_maps[i]->end = ~0ULL;
+	}
+}
+
 int machine__create_kernel_maps(struct machine *machine)
 {
 	struct dso *kernel = machine__get_kernel(machine);

@@ -1244,11 +1255,6 @@ int machine__create_kernel_maps(struct machine *machine)
 				 "continuing anyway...\n", machine->pid);
 	}
 
-	/*
-	 * Now that we have all the maps created, just set the ->end of them:
-	 */
-	map_groups__fixup_end(&machine->kmaps);
-
 	if (!machine__get_running_kernel_start(machine, &name, &addr)) {
 		if (name &&
 		    maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, name, addr)) {

@@ -1257,27 +1263,10 @@ int machine__create_kernel_maps(struct machine *machine)
 		}
 	}
 
+	machine__set_kernel_mmap(machine, addr, 0);
 	return 0;
 }
 
-static void machine__set_kernel_mmap(struct machine *machine,
-				     u64 start, u64 end)
-{
-	int i;
-
-	for (i = 0; i < MAP__NR_TYPES; i++) {
-		machine->vmlinux_maps[i]->start = start;
-		machine->vmlinux_maps[i]->end   = end;
-
-		/*
-		 * Be a bit paranoid here, some perf.data file came with
-		 * a zero sized synthesized MMAP event for the kernel.
-		 */
-		if (machine->vmlinux_maps[i]->end == 0)
-			machine->vmlinux_maps[i]->end = ~0ULL;
-	}
-}
-
 static bool machine__uses_kcore(struct machine *machine)
 {
 	struct dso *dso;

Signature · 1 line

2.13.6

Namhyung Kim

maintainer

2018-02-08 00:34 UTC #7 lore raw

Re: [PATCH 06/10] perf tools: Use machine__set_kernel_mmap instead of map_groups__fixup_end

Hi Jiri,

Quoted 2 lines · Jiri Olsa

The machine__set_kernel_mmap does the same job as map_groups__fixup_end when used on kernel maps within machine__create_kernel_maps call.

I'm not sure. Modules have end address after machine__create_module() but vmlinux_maps is not. So map_groups__fixup() will set the end of vmlinux_maps but with this change it will have ~0ULL.

Thanks,
Namhyung

Quoted 98 lines

This way we can also remove map_groups__fixup_end function, because there's no user to it. Also moving machine__set_kernel_mmap up in code, so we don't need forward declaration.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 49 ++++++++++++++++++-----------------------------
 1 file changed, 19 insertions(+), 30 deletions(-)

tools/perf/util/machine.c +19 ~~-30~~

index 7563b750137d..49ebd451151a 100644
@@ -1028,13 +1028,6 @@ int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
 	return ret;
 }
 
-static void map_groups__fixup_end(struct map_groups *mg)
-{
-	int i;
-	for (i = 0; i < MAP__NR_TYPES; ++i)
-		__map_groups__fixup_end(mg, i);
-}
-
 static char *get_kernel_version(const char *root_dir)
 {
 	char version[PATH_MAX];

@@ -1220,6 +1213,24 @@ static int machine__create_modules(struct machine *machine)
 	return 0;
 }
 
+static void machine__set_kernel_mmap(struct machine *machine,
+				     u64 start, u64 end)
+{
+	int i;
+
+	for (i = 0; i < MAP__NR_TYPES; i++) {
+		machine->vmlinux_maps[i]->start = start;
+		machine->vmlinux_maps[i]->end   = end;
+
+		/*
+		 * Be a bit paranoid here, some perf.data file came with
+		 * a zero sized synthesized MMAP event for the kernel.
+		 */
+		if (machine->vmlinux_maps[i]->end == 0)
+			machine->vmlinux_maps[i]->end = ~0ULL;
+	}
+}
+
 int machine__create_kernel_maps(struct machine *machine)
 {
 	struct dso *kernel = machine__get_kernel(machine);

@@ -1244,11 +1255,6 @@ int machine__create_kernel_maps(struct machine *machine)
 				 "continuing anyway...\n", machine->pid);
 	}
 
-	/*
-	 * Now that we have all the maps created, just set the ->end of them:
-	 */
-	map_groups__fixup_end(&machine->kmaps);
-
 	if (!machine__get_running_kernel_start(machine, &name, &addr)) {
 		if (name &&
 		    maps__set_kallsyms_ref_reloc_sym(machine->vmlinux_maps, name, addr)) {

@@ -1257,27 +1263,10 @@ int machine__create_kernel_maps(struct machine *machine)
 		}
 	}
 
+	machine__set_kernel_mmap(machine, addr, 0);
 	return 0;
 }
 
-static void machine__set_kernel_mmap(struct machine *machine,
-				     u64 start, u64 end)
-{
-	int i;
-
-	for (i = 0; i < MAP__NR_TYPES; i++) {
-		machine->vmlinux_maps[i]->start = start;
-		machine->vmlinux_maps[i]->end   = end;
-
-		/*
-		 * Be a bit paranoid here, some perf.data file came with
-		 * a zero sized synthesized MMAP event for the kernel.
-		 */
-		if (machine->vmlinux_maps[i]->end == 0)
-			machine->vmlinux_maps[i]->end = ~0ULL;
-	}
-}
-
 static bool machine__uses_kcore(struct machine *machine)
 {
 	struct dso *dso;

Signature · 1 line

2.13.6

Jiri Olsa

2018-02-08 08:57 UTC #8 lore raw

Re: [PATCH 06/10] perf tools: Use machine__set_kernel_mmap instead of map_groups__fixup_end

Quoted 9 lines · Namhyung Kim

Hi Jiri,
Quoted 2 lines · Jiri Olsa
The machine__set_kernel_mmap does the same job as map_groups__fixup_end when used on kernel maps within machine__create_kernel_maps call.
I'm not sure. Modules have end address after machine__create_module() but vmlinux_maps is not. So map_groups__fixup() will set the end of vmlinux_maps but with this change it will have ~0ULL.

ugh, I missed the machine__create_modules call in there, I'll rework and repost the patchset

thanks,
jirka

Jiri Olsa

author reviewer

2018-02-07 14:49 UTC #9 lore raw

PATCH v2 7/10 perf tools: Rename __map_groups__fixup_end to map_groups__fixup_end

In reply to Jiri Olsa

There's no need to keep the '__' prefix now when there's map_groups__fixup_end function gone.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c    | 2 +-
 tools/perf/util/symbol-elf.c | 2 +-
 tools/perf/util/symbol.c     | 2 +-
 tools/perf/util/symbol.h     | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

tools/perf/util/machine.c +1 -1

index 49ebd451151a..a02242eb25a1 100644
@@ -1005,7 +1005,7 @@ int __machine__load_kallsyms(struct machine *machine, const char *filename,
 		 * kernel, with modules between them, fixup the end of all
 		 * sections.
 		 */
-		__map_groups__fixup_end(&machine->kmaps, type);
+		map_groups__fixup_end(&machine->kmaps, type);
 	}
 
 	return ret;

tools/perf/util/symbol-elf.c +1 -1

index 2de770511e70..6d1e4dc2709f 100644
@@ -1124,7 +1124,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 			 * We need to fixup this here too because we create new
 			 * maps here, for things like vsyscall sections.
 			 */
-			__map_groups__fixup_end(kmaps, map->type);
+			map_groups__fixup_end(kmaps, map->type);
 		}
 	}
 	err = nr;

tools/perf/util/symbol.c +1 -1

index dcb81176669a..fe15dafaef66 100644
@@ -228,7 +228,7 @@ void symbols__fixup_end(struct rb_root *symbols)
 		curr->end = roundup(curr->start, 4096) + 4096;
 }
 
-void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
+void map_groups__fixup_end(struct map_groups *mg, enum map_type type)
 {
 	struct maps *maps = &mg->maps[type];
 	struct map *next, *curr;

tools/perf/util/symbol.h +1 -1

index 0563f33c1eb3..cf955f1d1718 100644
@@ -316,7 +316,7 @@ void __symbols__insert(struct rb_root *symbols, struct symbol *sym, bool kernel)
 void symbols__insert(struct rb_root *symbols, struct symbol *sym);
 void symbols__fixup_duplicate(struct rb_root *symbols);
 void symbols__fixup_end(struct rb_root *symbols);
-void __map_groups__fixup_end(struct map_groups *mg, enum map_type type);
+void map_groups__fixup_end(struct map_groups *mg, enum map_type type);
 
 typedef int (*mapfn_t)(u64 start, u64 len, u64 pgoff, void *data);
 int file__read_maps(int fd, bool exe, mapfn_t mapfn, void *data,

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #10 lore raw

PATCH v2 8/10 perf tools: Remove machine__load_kallsyms function

In reply to Jiri Olsa

And replacing it with __machine__load_kallsyms function.

The current machine__load_kallsyms function has no caller, so replacing it directly with __machine__load_kallsyms. Also removing the no_kcore argument as it was always called with true value.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/tests/vmlinux-kallsyms.c |  2 +-
 tools/perf/util/machine.c           | 14 ++++----------
 tools/perf/util/machine.h           |  2 --
 3 files changed, 5 insertions(+), 13 deletions(-)

tools/perf/tests/vmlinux-kallsyms.c +1 -1

index f6789fb029d6..58349297f9fb 100644
@@ -56,7 +56,7 @@ int test__vmlinux_matches_kallsyms(struct test *test __maybe_unused, int subtest
 	 * be compacted against the list of modules found in the "vmlinux"
 	 * code and with the one got from /proc/modules from the "kallsyms" code.
 	 */
-	if (__machine__load_kallsyms(&kallsyms, "/proc/kallsyms", type, true) <= 0) {
+	if (machine__load_kallsyms(&kallsyms, "/proc/kallsyms", type) <= 0) {
 		pr_debug("dso__load_kallsyms ");
 		goto out;
 	}

tools/perf/util/machine.c +4 ~~-10~~

index a02242eb25a1..9074c63c056e 100644
@@ -151,7 +151,7 @@ struct machine *machine__new_kallsyms(void)
 	 *    ask for not using the kcore parsing code, once this one is fixed
 	 *    to create a map per module.
 	 */
-	if (machine && __machine__load_kallsyms(machine, "/proc/kallsyms", MAP__FUNCTION, true) <= 0) {
+	if (machine && machine__load_kallsyms(machine, "/proc/kallsyms", MAP__FUNCTION) <= 0) {
 		machine__delete(machine);
 		machine = NULL;
 	}

@@ -992,11 +992,11 @@ int machines__create_kernel_maps(struct machines *machines, pid_t pid)
 	return machine__create_kernel_maps(machine);
 }
 
-int __machine__load_kallsyms(struct machine *machine, const char *filename,
-			     enum map_type type, bool no_kcore)
+int machine__load_kallsyms(struct machine *machine, const char *filename,
+			     enum map_type type)
 {
 	struct map *map = machine__kernel_map(machine);
-	int ret = __dso__load_kallsyms(map->dso, filename, map, no_kcore);
+	int ret = __dso__load_kallsyms(map->dso, filename, map, true);
 
 	if (ret > 0) {
 		dso__set_loaded(map->dso, type);

@@ -1011,12 +1011,6 @@ int __machine__load_kallsyms(struct machine *machine, const char *filename,
 	return ret;
 }
 
-int machine__load_kallsyms(struct machine *machine, const char *filename,
-			   enum map_type type)
-{
-	return __machine__load_kallsyms(machine, filename, type, false);
-}
-
 int machine__load_vmlinux_path(struct machine *machine, enum map_type type)
 {
 	struct map *map = machine__kernel_map(machine);

tools/perf/util/machine.h +0 -2

index 50d587d34459..66cc200ef86f 100644
@@ -225,8 +225,6 @@ struct map *machine__findnew_module_map(struct machine *machine, u64 start,
 					const char *filename);
 int arch__fix_module_text_start(u64 *start, const char *name);
 
-int __machine__load_kallsyms(struct machine *machine, const char *filename,
-			     enum map_type type, bool no_kcore);
 int machine__load_kallsyms(struct machine *machine, const char *filename,
 			   enum map_type type);
 int machine__load_vmlinux_path(struct machine *machine, enum map_type type);

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:49 UTC #11 lore raw

PATCH v2 9/10 perf tools: Do not create kernel maps in sample__resolve

In reply to Jiri Olsa

There's no need for kernel maps to be allocated at this point - sample processing.

We search for kernel maps using the kernel map_groups in machine::kmaps which is static. If vmlinux maps for any reason still don't exist, the search correctly fails because they are not in the map group.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/event.c | 11 -----------
 1 file changed, 11 deletions(-)

tools/perf/util/event.c +0 ~~-11~~

index 4644e751a3e3..f0a6cbd033cc 100644
@@ -1588,17 +1588,6 @@ int machine__resolve(struct machine *machine, struct addr_location *al,
 		return -1;
 
 	dump_printf(" ... thread: %s:%d\n", thread__comm_str(thread), thread->tid);
-	/*
-	 * Have we already created the kernel maps for this machine?
-	 *
-	 * This should have happened earlier, when we processed the kernel MMAP
-	 * events, but for older perf.data files there was no such thing, so do
-	 * it now.
-	 */
-	if (sample->cpumode == PERF_RECORD_MISC_KERNEL &&
-	    machine__kernel_map(machine) == NULL)
-		machine__create_kernel_maps(machine);
-
 	thread__find_addr_map(thread, sample->cpumode, MAP__FUNCTION, sample->ip, al);
 	dump_printf(" ...... dso: %s\n",
 		    al->map ? al->map->dso->long_name :

Signature · 1 line

2.13.6

Jiri Olsa

author reviewer

2018-02-07 14:48 UTC #12 lore raw

PATCH v2 10/10 perf tools: Check if we read regular file in dso__load

In reply to Jiri Olsa

Current code in dso__load calls the is_regular_file function, but it checks its return value only after calling symsrc__init.

That can make symsrc__init block in elf_* functions on reading the file if the file happens to be device and not regular one.

Make the check before calling symsrc__init.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/symbol.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

tools/perf/util/symbol.c +5 ~~-10~~

index fe15dafaef66..ea002b27b39d 100644
@@ -1580,9 +1580,7 @@ int dso__load(struct dso *dso, struct map *map)
 	for (i = 0; i < DSO_BINARY_TYPE__SYMTAB_CNT; i++) {
 		struct symsrc *ss = &ss_[ss_pos];
 		bool next_slot = false;
-		bool is_reg;
 		bool nsexit;
-		int sirc;
 
 		enum dso_binary_type symtab_type = binary_type_symtab[i];

@@ -1599,18 +1597,15 @@ int dso__load(struct dso *dso, struct map *map)
 		if (nsexit)
 			nsinfo__mountns_exit(&nsc);
 
-		is_reg = is_regular_file(name);
-		sirc = symsrc__init(ss, dso, name, symtab_type);
+		if (!is_regular_file(name))
+			continue;
+
+		if (symsrc__init(ss, dso, name, symtab_type) < 0)
+			continue;
 
 		if (nsexit)
 			nsinfo__mountns_enter(dso->nsinfo, &nsc);
 
-		if (!is_reg || sirc < 0) {
-			if (sirc >= 0)
-				symsrc__destroy(ss);
-			continue;
-		}
-
 		if (!syms_ss && symsrc__has_symtab(ss)) {
 			syms_ss = ss;
 			next_slot = true;

Signature · 1 line

2.13.6