[PATCHSET/RFC 0/9] perf tools: Support out-of-tree modules

[PATCHSET/RFC 0/9] perf tools: Support out-of-tree modules

[Namhyung Kim]

Hello,

Currently perf loads modules only in the canonical directory
(/lib/modules/`uname -r`/).  But in some situation users want to use
local or out-of-tree modules which are not placed in the directory.

One example is developing kernel in a qemu environment.  In this case,
guest doesn't see vmlinux or modules but one can load a module in some
directory which was copied separately.  During the development, the
module can be unloaded, fixed and reloaded more than once.

I notice that perf uses symbols in /proc/kallsyms (and /proc/kcore)
and it sometimes shows different result with a same data file.  This
was due to a same module being loaded at a different address or a
reworked module being loaded at a same address.

I think it needs to use build-id cache if possible.  To do that we
need to add an option to search modules and to save them in the
build-id cache automatically.

The code is available on 'perf/kmod-dir-v1' branch at

  git://git.kernel.org/pub/scm/linux/kernel/git/namhyung/linux-perf.git

Thanks,
Namhyung


Namhyung Kim (9):
  perf symbols: Use absolute address to fixup map address
  perf tools: Remove duplicate code
  perf symbols: Discard symbols in kallsyms for loaded modules
  perf symbols: Load kernel module symbols ASAP
  perf symbols: Fixup the end address of kernel map properly
  perf symbols: Use already loaded module dso when loading kcore
  perf tools: Add symbol_conf.use_kcore
  perf record: Not use kcore by default
  perf record: Add --module-dir option

 tools/perf/Documentation/perf-record.txt |  6 ++++
 tools/perf/builtin-record.c              |  7 ++++
 tools/perf/perf-with-kcore.sh            |  1 +
 tools/perf/util/machine.c                | 33 ++++++++---------
 tools/perf/util/map.c                    |  4 +--
 tools/perf/util/symbol.c                 | 61 +++++++++++++++++---------------
 tools/perf/util/symbol.h                 |  6 ++--
 7 files changed, 69 insertions(+), 49 deletions(-)

-- 
2.13.1

[PATCH/RFC 1/9] perf symbols: Use absolute address to fixup map address

[Namhyung Kim]

A symbol address is relative in a map/dso, to setup modules addresses it
should be converted to absolute address.  Note that it only used for
kernel mappings which uses identity map but theorically it should unmap
the address IMHO.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/map.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 2179b2deb730..4867265b800a 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -265,7 +265,7 @@ void map__fixup_start(struct map *map)
 	struct rb_node *nd = rb_first(symbols);
 	if (nd != NULL) {
 		struct symbol *sym = rb_entry(nd, struct symbol, rb_node);
-		map->start = sym->start;
+		map->start = map->unmap_ip(map, sym->start);
 	}
 }
 
@@ -275,7 +275,7 @@ void map__fixup_end(struct map *map)
 	struct rb_node *nd = rb_last(symbols);
 	if (nd != NULL) {
 		struct symbol *sym = rb_entry(nd, struct symbol, rb_node);
-		map->end = sym->end;
+		map->end = map->unmap_ip(map, sym->end);
 	}
 }
 
-- 
2.13.1

[PATCH/RFC 2/9] perf tools: Remove duplicate code

[Namhyung Kim]

The map_groups__set_module_path() is called after
machine__create_module() which sets build-id and symtab type already.
Also remove is_kmod_dso() as there's no user anymore.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/machine.c | 15 ---------------
 1 file changed, 15 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d7f31cb0a4cb..d838f893e639 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1024,12 +1024,6 @@ static char *get_kernel_version(const char *root_dir)
 	return strdup(name);
 }
 
-static bool is_kmod_dso(struct dso *dso)
-{
-	return dso->symtab_type == DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE ||
-	       dso->symtab_type == DSO_BINARY_TYPE__GUEST_KMODULE;
-}
-
 static int map_groups__set_module_path(struct map_groups *mg, const char *path,
 				       struct kmod_path *m)
 {
@@ -1045,15 +1039,6 @@ static int map_groups__set_module_path(struct map_groups *mg, const char *path,
 		return -ENOMEM;
 
 	dso__set_long_name(map->dso, long_name, true);
-	dso__kernel_module_get_build_id(map->dso, "");
-
-	/*
-	 * Full name could reveal us kmod compression, so
-	 * we need to update the symtab_type if needed.
-	 */
-	if (m->comp && is_kmod_dso(map->dso))
-		map->dso->symtab_type++;
-
 	return 0;
 }
 
-- 
2.13.1

[PATCH/RFC 3/9] perf symbols: Discard symbols in kallsyms for loaded modules

[Namhyung Kim]

If a module is already loaded, it should have symbols and no need to
load new symbols from kallsyms.  Actually kallsyms can have different
addresses if the module was reloaded.

Current code just discards the first symbols only, but it should do the
same for all symbols in the module.  Note that the kernel doesn't set
the dso->loaded bit so simply checking it would do the job IMHO.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index e7a98dbd2aed..74078ba595b3 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -787,11 +787,12 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta)
 					curr_map = map;
 					goto discard_symbol;
 				}
-
-				if (curr_map->dso->loaded &&
-				    !machine__is_default_guest(machine))
-					goto discard_symbol;
 			}
+
+			if (curr_map->dso->loaded &&
+			    !machine__is_default_guest(machine))
+				goto discard_symbol;
+
 			/*
 			 * So that we look just like we get from .ko files,
 			 * i.e. not prelinked, relative to map->start.
-- 
2.13.1

[PATCH/RFC 4/9] perf symbols: Load kernel module symbols ASAP

[Namhyung Kim]

When loading kernel symbols from /proc/kallsyms, it might have different
addresses for modules.  We should honor the mmap event recorded in a
perf.data so load the module symbols when it sees the event so that it
cannot be overridden by symbols in /proc/kallsyms later.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/machine.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index d838f893e639..799efe920f0c 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -656,6 +656,9 @@ struct map *machine__findnew_module_map(struct machine *machine, u64 start,
 
 	map_groups__insert(&machine->kmaps, map);
 
+	if (dso->has_build_id)
+		map__load(map);
+
 	/* Put the map here because map_groups__insert alread got it */
 	map__put(map);
 out:
-- 
2.13.1

[PATCH/RFC 5/9] perf symbols: Fixup the end address of kernel map properly

[Namhyung Kim]

When /proc/kallsyms is used for kernel address, addresses in module can
be changed when the module is reloaded.  So if one did perf record with
some module and then for some reason reload the module.  Then perf
report might see a different address for the module and the output can
show incorrect symbols.

For example, let a module XXX was loaded at 0xffffffff8a000000, the
/proc/kallsyms might show following:

  ...
  0xffffffff81234560 t last_symbol_in_vmlinux
  0xffffffff8a000000 t first_symbol_in_XXX
  ...

As kallsyms fixs up the symbol and map address by using next address,
the end address of last_symbol and kernel map would be start address of
XXX (0xffffffff8a000000).  And samples in 0xffffffff8a001234 can be
found in a map for XXX module.

But later, XXX was reloaded at 0xffffffff8a007000, slightly higher than
before.  Now reading /proc/kallsyms tells that the end address of last
symbol would be 0xfffffff8a007000 and so kernel map is same.  Now
samples in 0xffffffff8a001234 - formerly in the XXX module - would go to
the kernel map and show no symbols.

In this case, perf can know the address of map of XXX from mmap event in
perf.data so it can adjust kernel map address using the address of XXX
map.  To do that, replace map__fixup_end() by __map_groups__fixup_end().
This still have incorrect end address of last symbol in the kernel map
but it's ok since samples in that address wouldn't go to the kernel map
anyway.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 74078ba595b3..ce79a51f25bf 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1832,7 +1832,7 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
 		dso->binary_type = DSO_BINARY_TYPE__KALLSYMS;
 		dso__set_long_name(dso, DSO__NAME_KALLSYMS, false);
 		map__fixup_start(map);
-		map__fixup_end(map);
+		__map_groups__fixup_end(map->groups, map->type);
 	}
 
 	return err;
@@ -1880,7 +1880,7 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
 		machine__mmap_name(machine, path, sizeof(path));
 		dso__set_long_name(dso, strdup(path), true);
 		map__fixup_start(map);
-		map__fixup_end(map);
+		__map_groups__fixup_end(map->groups, map->type);
 	}
 
 	return err;
-- 
2.13.1

[PATCH/RFC 6/9] perf symbols: Use already loaded module dso when loading kcore

[Namhyung Kim]

Even every module has loaded onto same addresses, some modules can be
changed and reloaded.  In that case it needs to access to the old module
in the build-id cache.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 45 +++++++++++++++++++++++----------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index ce79a51f25bf..fe46eb782297 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1155,6 +1155,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	int err, fd;
 	char kcore_filename[PATH_MAX];
 	struct symbol *sym;
+	struct map_groups old_mg;
 
 	if (!kmaps)
 		return -EINVAL;
@@ -1196,15 +1197,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 		goto out_err;
 	}
 
-	/* Remove old maps */
-	old_map = map_groups__first(kmaps, map->type);
-	while (old_map) {
-		struct map *next = map_groups__next(old_map);
-
-		if (old_map != map)
-			map_groups__remove(kmaps, old_map);
-		old_map = next;
-	}
+	old_mg = *kmaps;
+	kmaps->maps[map->type].entries = RB_ROOT;
 
 	/* Find the kernel map using the first symbol */
 	sym = dso__first_symbol(dso, map->type);
@@ -1223,24 +1217,31 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
 	while (!list_empty(&md.maps)) {
 		new_map = list_entry(md.maps.next, struct map, node);
 		list_del_init(&new_map->node);
-		if (new_map == replacement_map) {
-			map->start	= new_map->start;
-			map->end	= new_map->end;
-			map->pgoff	= new_map->pgoff;
-			map->map_ip	= new_map->map_ip;
-			map->unmap_ip	= new_map->unmap_ip;
-			/* Ensure maps are correctly ordered */
-			map__get(map);
-			map_groups__remove(kmaps, map);
-			map_groups__insert(kmaps, map);
-			map__put(map);
-		} else {
-			map_groups__insert(kmaps, new_map);
+
+		map_groups__insert(kmaps, new_map);
+
+		if (new_map != replacement_map) {
+			old_map = map_groups__find(&old_mg, new_map->type, new_map->start);
+			if (old_map && dso__loaded(old_map->dso, old_map->type)) {
+				new_map->pgoff = old_map->pgoff;
+
+				dso__put(new_map->dso);
+				new_map->dso = dso__get(old_map->dso);
+			}
 		}
 
 		map__put(new_map);
 	}
 
+	/* Remove old maps */
+	old_map = map_groups__first(&old_mg, map->type);
+	while (old_map) {
+		struct map *next = map_groups__next(old_map);
+
+		map_groups__remove(&old_mg, old_map);
+		old_map = next;
+	}
+
 	/*
 	 * Set the data type and long name so that kcore can be read via
 	 * dso__data_read_addr().
-- 
2.13.1

[PATCH/RFC 7/9] perf tools: Add symbol_conf.use_kcore

[Namhyung Kim]

The use_kcore field is to control usage of /proc/kcore when loading
symbols.  This patch only introduces the new field and don't change any
behavior by itself.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 3 ++-
 tools/perf/util/symbol.h | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index fe46eb782297..3f789f33c2ef 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -36,6 +36,7 @@ char **vmlinux_path;
 
 struct symbol_conf symbol_conf = {
 	.use_modules		= true,
+	.use_kcore		= true,
 	.try_vmlinux_path	= true,
 	.annotate_src		= true,
 	.demangle		= true,
@@ -1324,7 +1325,7 @@ int __dso__load_kallsyms(struct dso *dso, const char *filename,
 int dso__load_kallsyms(struct dso *dso, const char *filename,
 		       struct map *map)
 {
-	return __dso__load_kallsyms(dso, filename, map, false);
+	return __dso__load_kallsyms(dso, filename, map, !symbol_conf.use_kcore);
 }
 
 static int dso__load_perf_map(struct dso *dso, struct map *map)
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 41ebba9a2eb2..88361eeae813 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -119,7 +119,8 @@ struct symbol_conf {
 			hide_unresolved,
 			raw_trace,
 			report_hierarchy,
-			inline_name;
+			inline_name,
+			use_kcore;
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-- 
2.13.1

[PATCH/RFC 8/9] perf record: Not use kcore by default

[Namhyung Kim]

Change perf record not to use /proc/kcore by default.  This is for
kernel developers who use qemu or kvmtools to test their kernels.  On
those environment, kernel image was loaded directly by qemu and the
vmlinux might not be available on the guest.

At the last stage of perf record, it finds hit DSOs to mark them to
record the build-ids.  During this process, it tries to load kernel maps
and falls back to use kallsyms with kcore.  But dso__load_kcore()
removes old mappings so all module info would disappear.

I'm not sure this is intended but it'd be good if it could keep the info
and use it for build-id cache.  Add --use-kcore option to request it
explicitly (like in perf-with-kcore).

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-record.txt | 3 +++
 tools/perf/builtin-record.c              | 5 +++++
 tools/perf/perf-with-kcore.sh            | 1 +
 3 files changed, 9 insertions(+)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index b0e9e921d534..eb2f5fb90534 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -477,6 +477,9 @@ config terms. For example: 'cycles/overwrite/' and 'instructions/no-overwrite/'.
 
 Implies --tail-synthesize.
 
+--use-kcore::
+Use /proc/kcore for symbols and object code reading
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index ee7d0a82ccd0..a6a6cb56fdf5 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1669,6 +1669,8 @@ static struct option __record_options[] = {
 			  "signal"),
 	OPT_BOOLEAN(0, "dry-run", &dry_run,
 		    "Parse options then exit"),
+	OPT_BOOLEAN(0, "use-kcore", &symbol_conf.use_kcore,
+		    "Use /proc/kcore for object code"),
 	OPT_END()
 };
 
@@ -1705,6 +1707,9 @@ int cmd_record(int argc, const char **argv)
 	if (rec->evlist == NULL)
 		return -ENOMEM;
 
+	/* default to not use kcore, user can change it by --use-kcore option */
+	symbol_conf.use_kcore = false;
+
 	err = perf_config(perf_record_config, rec);
 	if (err)
 		return err;
diff --git a/tools/perf/perf-with-kcore.sh b/tools/perf/perf-with-kcore.sh
index 7e47a7cbc195..4efde3e577dd 100644
--- a/tools/perf/perf-with-kcore.sh
+++ b/tools/perf/perf-with-kcore.sh
@@ -233,6 +233,7 @@ fi
 
 case "$PERF_SUB_COMMAND" in
 "record")
+	PERF_OPTIONS+=("--use-kcore")
 	while [ "$1" != "--" ] ; do
 		PERF_OPTIONS+=("$1")
 		shift || break
-- 
2.13.1

[PATCH/RFC 9/9] perf record: Add --module-dir option

[Namhyung Kim]

Currently perf only searches module binaries on the canonical
directory (/lib/modules/`uname -r`).  But sometimes user needs to load
local modules.  These cannot be copied to the build-id cache since long
name (i.e. real path) of DSOs was not set.

This patch fixes the problem by adding a new --module-dir option so that
perf can record modules in the directory.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Wang Nan <wangnan0@huawei.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/Documentation/perf-record.txt |  3 +++
 tools/perf/builtin-record.c              |  2 ++
 tools/perf/util/machine.c                | 15 ++++++++++++++-
 tools/perf/util/symbol.h                 |  3 ++-
 4 files changed, 21 insertions(+), 2 deletions(-)

diff --git a/tools/perf/Documentation/perf-record.txt b/tools/perf/Documentation/perf-record.txt
index eb2f5fb90534..9030ace9010f 100644
--- a/tools/perf/Documentation/perf-record.txt
+++ b/tools/perf/Documentation/perf-record.txt
@@ -480,6 +480,9 @@ Implies --tail-synthesize.
 --use-kcore::
 Use /proc/kcore for symbols and object code reading
 
+--module-dir=PATH::
+Directory name where extra modules are located.
+
 SEE ALSO
 --------
 linkperf:perf-stat[1], linkperf:perf-list[1]
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index a6a6cb56fdf5..8a67fafc0d5b 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1671,6 +1671,8 @@ static struct option __record_options[] = {
 		    "Parse options then exit"),
 	OPT_BOOLEAN(0, "use-kcore", &symbol_conf.use_kcore,
 		    "Use /proc/kcore for object code"),
+	OPT_STRING(0, "module-dir", &symbol_conf.extra_module_path, "path",
+		   "directory name where extra modules are located"),
 	OPT_END()
 };
 
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 799efe920f0c..9a18365c443a 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1108,6 +1108,7 @@ static int machine__set_modules_path(struct machine *machine)
 {
 	char *version;
 	char modules_path[PATH_MAX];
+	int ret;
 
 	version = get_kernel_version(machine->root_dir);
 	if (!version)
@@ -1117,7 +1118,19 @@ static int machine__set_modules_path(struct machine *machine)
 		 machine->root_dir, version);
 	free(version);
 
-	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
+	ret = map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
+	if (ret < 0)
+		return ret;
+
+	if (symbol_conf.extra_module_path) {
+		snprintf(modules_path, sizeof(modules_path), "%s/%s",
+			 machine->root_dir, symbol_conf.extra_module_path);
+
+		ret = map_groups__set_modules_path_dir(&machine->kmaps,
+						       modules_path, 0);
+	}
+
+	return ret;
 }
 int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
 				const char *name __maybe_unused)
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 88361eeae813..59370ceb87c4 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -124,7 +124,8 @@ struct symbol_conf {
 	const char	*vmlinux_name,
 			*kallsyms_name,
 			*source_prefix,
-			*field_sep;
+			*field_sep,
+			*extra_module_path;
 	const char	*default_guest_vmlinux_name,
 			*default_guest_kallsyms,
 			*default_guest_modules;
-- 
2.13.1

Re: [PATCH/RFC 3/9] perf symbols: Discard symbols in kallsyms for loaded modules

[Arnaldo Carvalho de Melo]

Em Fri, Jun 23, 2017 at 02:48:21PM +0900, Namhyung Kim escreveu:
> If a module is already loaded, it should have symbols and no need to
> load new symbols from kallsyms.  Actually kallsyms can have different
> addresses if the module was reloaded.

Well, if it is loaded, then it should match what is in kallsyms, no?
 
> Current code just discards the first symbols only, but it should do the
> same for all symbols in the module.  Note that the kernel doesn't set
> the dso->loaded bit so simply checking it would do the job IMHO.

The kernel sets dso->loaded? Can you rephrase this part? And if this
procedure doesn't set dso->loaded how can simply checking it do the job?

Can you clarify the above sentence? Please elaborate the scenario more
fully.

- Arnaldo
 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/symbol.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e7a98dbd2aed..74078ba595b3 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -787,11 +787,12 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta)
>  					curr_map = map;
>  					goto discard_symbol;
>  				}
> -
> -				if (curr_map->dso->loaded &&
> -				    !machine__is_default_guest(machine))
> -					goto discard_symbol;
>  			}
> +
> +			if (curr_map->dso->loaded &&
> +			    !machine__is_default_guest(machine))
> +				goto discard_symbol;
> +
>  			/*
>  			 * So that we look just like we get from .ko files,
>  			 * i.e. not prelinked, relative to map->start.
> -- 
> 2.13.1

Re: [PATCH/RFC 6/9] perf symbols: Use already loaded module dso when loading kcore

[Arnaldo Carvalho de Melo]

Em Fri, Jun 23, 2017 at 02:48:24PM +0900, Namhyung Kim escreveu:
> Even every module has loaded onto same addresses, some modules can be
> changed and reloaded.

Can you rephrase the above statement?

You mean even if a module is reloaded at the same address it can have a
different symtab and thus misresolve symbols if we get a sample for one
module version and try to resolve symbols using a symtab for another
module version?

> In that case it needs to access to the old module
> in the build-id cache.

Right, we need to make sure that during a 'perf record' session a módule
isn't reloaded, because at 'perf record' exit we will record that module
build-id in a perf.data header and grab a copy (or a hardlink, if
possible) into the ~/.debug build-id cache, then, at 'perf report' time
we will read that build-id perf.data table and lookup in the build-id
cache to get the matching symtab (and full elf .ko file to do things
like annotation).

- Arnaldo
 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/symbol.c | 45 +++++++++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index ce79a51f25bf..fe46eb782297 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1155,6 +1155,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  	int err, fd;
>  	char kcore_filename[PATH_MAX];
>  	struct symbol *sym;
> +	struct map_groups old_mg;
>  
>  	if (!kmaps)
>  		return -EINVAL;
> @@ -1196,15 +1197,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  		goto out_err;
>  	}
>  
> -	/* Remove old maps */
> -	old_map = map_groups__first(kmaps, map->type);
> -	while (old_map) {
> -		struct map *next = map_groups__next(old_map);
> -
> -		if (old_map != map)
> -			map_groups__remove(kmaps, old_map);
> -		old_map = next;
> -	}
> +	old_mg = *kmaps;
> +	kmaps->maps[map->type].entries = RB_ROOT;
>  
>  	/* Find the kernel map using the first symbol */
>  	sym = dso__first_symbol(dso, map->type);
> @@ -1223,24 +1217,31 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
>  	while (!list_empty(&md.maps)) {
>  		new_map = list_entry(md.maps.next, struct map, node);
>  		list_del_init(&new_map->node);
> -		if (new_map == replacement_map) {
> -			map->start	= new_map->start;
> -			map->end	= new_map->end;
> -			map->pgoff	= new_map->pgoff;
> -			map->map_ip	= new_map->map_ip;
> -			map->unmap_ip	= new_map->unmap_ip;
> -			/* Ensure maps are correctly ordered */
> -			map__get(map);
> -			map_groups__remove(kmaps, map);
> -			map_groups__insert(kmaps, map);
> -			map__put(map);
> -		} else {
> -			map_groups__insert(kmaps, new_map);
> +
> +		map_groups__insert(kmaps, new_map);
> +
> +		if (new_map != replacement_map) {
> +			old_map = map_groups__find(&old_mg, new_map->type, new_map->start);
> +			if (old_map && dso__loaded(old_map->dso, old_map->type)) {
> +				new_map->pgoff = old_map->pgoff;
> +
> +				dso__put(new_map->dso);
> +				new_map->dso = dso__get(old_map->dso);
> +			}
>  		}
>  
>  		map__put(new_map);
>  	}
>  
> +	/* Remove old maps */
> +	old_map = map_groups__first(&old_mg, map->type);
> +	while (old_map) {
> +		struct map *next = map_groups__next(old_map);
> +
> +		map_groups__remove(&old_mg, old_map);
> +		old_map = next;
> +	}
> +
>  	/*
>  	 * Set the data type and long name so that kcore can be read via
>  	 * dso__data_read_addr().
> -- 
> 2.13.1

Re: [PATCH/RFC 4/9] perf symbols: Load kernel module symbols ASAP

[Arnaldo Carvalho de Melo]

Em Fri, Jun 23, 2017 at 02:48:22PM +0900, Namhyung Kim escreveu:
> When loading kernel symbols from /proc/kallsyms, it might have different
> addresses for modules.  We should honor the mmap event recorded in a
> perf.data so load the module symbols when it sees the event so that it
> cannot be overridden by symbols in /proc/kallsyms later.
 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/machine.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index d838f893e639..799efe920f0c 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -656,6 +656,9 @@ struct map *machine__findnew_module_map(struct machine *machine, u64 start,
>  
>  	map_groups__insert(&machine->kmaps, map);
>  
> +	if (dso->has_build_id)
> +		map__load(map);

How could this be set at this point? I just tried processing a
PERF_RECORD_MMAP event for a kernel module and at this point it is set
to false :-\

Humm, but I did it for video.ko, that has no hits in this case, so this
must be coming from reading the perf.data build-id table... /me tries
that...  yeah, when processing the buildid table in perf.data we
populate the machine->dsos list and set the dso->has_build_id flag:

[root@jouet ~]# perf buildid-list | grep .ko
57a47c2f0d85ef5726b80e8f55403c3f508a4eac /lib/modules/4.12.0-rc4+/kernel/drivers/net/wireless/intel/iwlwifi/iwlwifi.ko
[root@jouet ~]# 
(gdb) bt
#0  machine__findnew_module_map (machine=0x21c6c78, start=18446744072647282688, filename=0x7fd8fc057a00 "/lib/modules/4.12.0-rc4+/kernel/drivers/net/wireless/intel/iwlwifi/iwlwifi.ko")
    at util/machine.c:650
#1  0x00000000005094a3 in machine__process_kernel_mmap_event (machine=0x21c6c78, event=0x7fd8fc0579d8) at util/machine.c:1274
#2  0x00000000005099de in machine__process_mmap_event (machine=0x21c6c78, event=0x7fd8fc0579d8, sample=0x7fffffffbef0) at util/machine.c:1433
#3  0x00000000004cc021 in perf_event__process_mmap (tool=0x7fffffffc440, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, machine=0x21c6c78) at util/event.c:1265
#4  0x0000000000512686 in machines__deliver_event (machines=0x21c6c78, evlist=0x21c6f30, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, tool=0x7fffffffc440, file_offset=10712)
    at util/session.c:1250
#5  0x00000000005129ce in perf_session__deliver_event (session=0x21c6b90, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, tool=0x7fffffffc440, file_offset=10712) at util/session.c:1310
#6  0x0000000000513255 in perf_session__process_event (session=0x21c6b90, event=0x7fd8fc0579d8, file_offset=10712) at util/session.c:1490
#7  0x0000000000513ff4 in __perf_session__process_events (session=0x21c6b90, data_offset=232, data_size=23120, file_size=23352) at util/session.c:1867
#8  0x00000000005141f2 in perf_session__process_events (session=0x21c6b90) at util/session.c:1921
#9  0x00000000004427c9 in __cmd_report (rep=0x7fffffffc440) at builtin-report.c:575
#10 0x000000000044427c in cmd_report (argc=0, argv=0x7fffffffe160) at builtin-report.c:1054
#11 0x00000000004bad91 in run_builtin (p=0xa28fd0 <commands+240>, argc=2, argv=0x7fffffffe160) at perf.c:296
#12 0x00000000004baffe in handle_internal_command (argc=2, argv=0x7fffffffe160) at perf.c:348
#13 0x00000000004bb150 in run_argv (argcp=0x7fffffffdfbc, argv=0x7fffffffdfb0) at perf.c:392
#14 0x00000000004bb52a in main (argc=2, argv=0x7fffffffe160) at perf.c:530
(gdb) p dso
$127 = (struct dso *) 0x21c7dc0
(gdb) p dso->has_build_id
$128 = 1 '\001'
(gdb) 

So what you're doing is to load it here... perhaps we should not do
that, and instead in the kallsyms loading code, when processing the
symbols for this specific module, discard them and leave its symbol
table unpopulated, then, later, when resolving a sample in this module,
it will see that it is not loaded, will see that it has a build-id and
will use that, and _refuse_ to get it from anywhere else than an ELF
file with that build-id.

I.e. in general, when we have a build-id for a given DSO, we should not
read it from kallsyms or any other file that don't have a matching
build-id.

The only possibility would be: hey, I have a build id, and reading the
currently loaded module (/sys/module/e1000e/notes/.note.gnu.build-id) I
see it _still_ has that same build-id, ok, I can read it from kallsyms.

What do you think?

- Arnaldo


> +
>  	/* Put the map here because map_groups__insert alread got it */
>  	map__put(map);
>  out:
> -- 
> 2.13.1

Re: [PATCH/RFC 5/9] perf symbols: Fixup the end address of kernel map properly

[Arnaldo Carvalho de Melo]

Em Fri, Jun 23, 2017 at 02:48:23PM +0900, Namhyung Kim escreveu:
> When /proc/kallsyms is used for kernel address, addresses in module can
> be changed when the module is reloaded.  So if one did perf record with
> some module and then for some reason reload the module.  Then perf
> report might see a different address for the module and the output can
> show incorrect symbols.

If we, as mentioned in another message, make sure that no module reload
is performed while perf record is running, and then refuse to use
kallsyms if the buildid for the running module is different from the one
in the perf.data build id header, then this is solved, no?
 
> For example, let a module XXX was loaded at 0xffffffff8a000000, the
> /proc/kallsyms might show following:
> 
>   ...
>   0xffffffff81234560 t last_symbol_in_vmlinux
>   0xffffffff8a000000 t first_symbol_in_XXX
>   ...
> 
> As kallsyms fixs up the symbol and map address by using next address,
> the end address of last_symbol and kernel map would be start address of
> XXX (0xffffffff8a000000).  And samples in 0xffffffff8a001234 can be
> found in a map for XXX module.
> 
> But later, XXX was reloaded at 0xffffffff8a007000, slightly higher than
> before.  Now reading /proc/kallsyms tells that the end address of last
> symbol would be 0xfffffff8a007000 and so kernel map is same.  Now
> samples in 0xffffffff8a001234 - formerly in the XXX module - would go to
> the kernel map and show no symbols.
> 
> In this case, perf can know the address of map of XXX from mmap event in
> perf.data so it can adjust kernel map address using the address of XXX
> map.  To do that, replace map__fixup_end() by __map_groups__fixup_end().
> This still have incorrect end address of last symbol in the kernel map
> but it's ok since samples in that address wouldn't go to the kernel map
> anyway.
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Wang Nan <wangnan0@huawei.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/symbol.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 74078ba595b3..ce79a51f25bf 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1832,7 +1832,7 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
>  		dso->binary_type = DSO_BINARY_TYPE__KALLSYMS;
>  		dso__set_long_name(dso, DSO__NAME_KALLSYMS, false);
>  		map__fixup_start(map);
> -		map__fixup_end(map);
> +		__map_groups__fixup_end(map->groups, map->type);
>  	}
>  
>  	return err;
> @@ -1880,7 +1880,7 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
>  		machine__mmap_name(machine, path, sizeof(path));
>  		dso__set_long_name(dso, strdup(path), true);
>  		map__fixup_start(map);
> -		map__fixup_end(map);
> +		__map_groups__fixup_end(map->groups, map->type);
>  	}
>  
>  	return err;
> -- 
> 2.13.1

Re: [PATCH/RFC 9/9] perf record: Add --module-dir option

[Arnaldo Carvalho de Melo]

Em Fri, Jun 23, 2017 at 02:48:27PM +0900, Namhyung Kim escreveu:
> Currently perf only searches module binaries on the canonical
> directory (/lib/modules/`uname -r`).  But sometimes user needs to load
> local modules.  These cannot be copied to the build-id cache since long
> name (i.e. real path) of DSOs was not set.
 
> This patch fixes the problem by adding a new --module-dir option so that
> perf can record modules in the directory.
 
> +++ b/tools/perf/Documentation/perf-record.txt
> @@ -480,6 +480,9 @@ Implies --tail-synthesize.
> +--module-dir=PATH::
> +Directory name where extra modules are located.

> +++ b/tools/perf/builtin-record.c
> @@ -1671,6 +1671,8 @@ static struct option __record_options[] = {
>  		    "Parse options then exit"),
>  	OPT_BOOLEAN(0, "use-kcore", &symbol_conf.use_kcore,
>  		    "Use /proc/kcore for object code"),
> +	OPT_STRING(0, "module-dir", &symbol_conf.extra_module_path, "path",
> +		   "directory name where extra modules are located"),
>  	OPT_END()
>  };
>  
> +++ b/tools/perf/util/machine.c
> @@ -1117,7 +1118,19 @@ static int machine__set_modules_path(struct machine *machine)
>  		 machine->root_dir, version);
>  	free(version);
>  
> -	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> +	ret = map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (symbol_conf.extra_module_path) {
> +		snprintf(modules_path, sizeof(modules_path), "%s/%s",
> +			 machine->root_dir, symbol_conf.extra_module_path);
> +
> +		ret = map_groups__set_modules_path_dir(&machine->kmaps,
> +						       modules_path, 0);

What if we have samples in a module that is in the canonical dir _and_
samples in a module in this extra_module_path? Shouldn't we have
something like vmlinux_path, but for modules? Where you can add entries
in that path?

I'm ok with adding entries to where module files are looked up, with
semantics similar to $PATH in a shell, i.e. entries added via
--module-path (rename of your --module-dir) will take precedence over
the canonical dir (/lib/modules/`uname -r`).

But for the future I think we should try to get a PERF_RECORD_MMAP that
will allow reloading modules during a 'perf record' session when a
module gets reloaded, I thought about using existing tracepoints, but...

[root@jouet ~]# cat /sys/kernel/debug/tracing/events/module/module_load/format 
name: module_load
ID: 370
format:
	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
	field:int common_pid;	offset:4;	size:4;	signed:1;

	field:unsigned int taints;	offset:8;	size:4;	signed:0;
	field:__data_loc char[] name;	offset:12;	size:4;	signed:1;

print fmt: "%s %s", __get_str(name), __print_flags(REC->taints, "", { (1UL << 0), "P" }, { (1UL << 12), "O" }, { (1UL << 1), "F" }, { (1UL << 10), "C" }, { (1UL << 13), "E" })
[root@jouet ~]# perf trace --no-syscalls --event module:module_load modprobe ppp
modprobe: FATAL: Module ppp not found in directory /lib/modules/4.12.0-rc4+
[root@jouet ~]# perf trace --no-syscalls --event module:module_load modprobe e1000
     0.000 module:module_load:e1000 )
[root@jouet ~]#

It just gets us the module name, not the file from what it is loaded, bummer :-\

Something like:

diff --git a/kernel/module.c b/kernel/module.c
index 4a3665f8f837..50e9718a141d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3739,6 +3739,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
 
 	/* Done! */
 	trace_module_load(mod);
+	perf_event_mmap_mod(mod);
 
 	return do_init_module(mod);
 
 ----------

struct module *mod has:

  mod->name:		"e1000"
  mod->debug->filename: /lib/modules/4.11.0-rc8+/kernel/drivers/net/ethernet/intel/e1000/e1000.ko

and at that point we also have load_info, htat has ELF headers where we could extract the build-id
and insert it in a field in a new PERF_RECORD_MMAP3 record :-)

  perf_event_mmap_mod() would be modelled after perf_event_mmap(vma), but getting what
  it needs not from the vma present in a sys_mmap() but from struct module and load_info.

- Arnaldo

> +	}
> +
> +	return ret;
>  }
>  int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
>  				const char *name __maybe_unused)
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 88361eeae813..59370ceb87c4 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -124,7 +124,8 @@ struct symbol_conf {
>  	const char	*vmlinux_name,
>  			*kallsyms_name,
>  			*source_prefix,
> -			*field_sep;
> +			*field_sep,
> +			*extra_module_path;
>  	const char	*default_guest_vmlinux_name,
>  			*default_guest_kallsyms,
>  			*default_guest_modules;
> -- 
> 2.13.1

Re: [PATCH/RFC 3/9] perf symbols: Discard symbols in kallsyms for loaded modules

[Namhyung Kim]

On Fri, Jun 23, 2017 at 10:51:05AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 23, 2017 at 02:48:21PM +0900, Namhyung Kim escreveu:
> > If a module is already loaded, it should have symbols and no need to
> > load new symbols from kallsyms.  Actually kallsyms can have different
> > addresses if the module was reloaded.
> 
> Well, if it is loaded, then it should match what is in kallsyms, no?

No, the perf.data was recorded with a module, and then the module was
modified and reloaded at the same address.

Maybe the word "loaded" can be confusing - if perf.data file has
symbols in the module v1 but kallsyms has symbols in the module v2.
So it needs to read symbols from the module v1 (in the build-id cache).

>  
> > Current code just discards the first symbols only, but it should do the
> > same for all symbols in the module.  Note that the kernel doesn't set
> > the dso->loaded bit so simply checking it would do the job IMHO.
> 
> The kernel sets dso->loaded? Can you rephrase this part? And if this
> procedure doesn't set dso->loaded how can simply checking it do the job?
> 
> Can you clarify the above sentence? Please elaborate the scenario more
> fully.

Oh, I made it confusing, sorry..  I mean that it's in the middle of
the dso__load() so the (kernel) dso->loaded bit was not set yet.

Thanks,
Namhyung


> 
> - Arnaldo
>  
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Wang Nan <wangnan0@huawei.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/symbol.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index e7a98dbd2aed..74078ba595b3 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -787,11 +787,12 @@ static int dso__split_kallsyms(struct dso *dso, struct map *map, u64 delta)
> >  					curr_map = map;
> >  					goto discard_symbol;
> >  				}
> > -
> > -				if (curr_map->dso->loaded &&
> > -				    !machine__is_default_guest(machine))
> > -					goto discard_symbol;
> >  			}
> > +
> > +			if (curr_map->dso->loaded &&
> > +			    !machine__is_default_guest(machine))
> > +				goto discard_symbol;
> > +
> >  			/*
> >  			 * So that we look just like we get from .ko files,
> >  			 * i.e. not prelinked, relative to map->start.
> > -- 
> > 2.13.1

Re: [PATCH/RFC 6/9] perf symbols: Use already loaded module dso when loading kcore

[Namhyung Kim]

On Fri, Jun 23, 2017 at 10:55:47AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 23, 2017 at 02:48:24PM +0900, Namhyung Kim escreveu:
> > Even every module has loaded onto same addresses, some modules can be
> > changed and reloaded.
> 
> Can you rephrase the above statement?
> 
> You mean even if a module is reloaded at the same address it can have a
> different symtab and thus misresolve symbols if we get a sample for one
> module version and try to resolve symbols using a symtab for another
> module version?

Right, as I said in other thread, the module can be unloaded, modified
and reloaded between "perf record" and "perf report".


> 
> > In that case it needs to access to the old module
> > in the build-id cache.
> 
> Right, we need to make sure that during a 'perf record' session a módule
> isn't reloaded, because at 'perf record' exit we will record that module
> build-id in a perf.data header and grab a copy (or a hardlink, if
> possible) into the ~/.debug build-id cache, then, at 'perf report' time
> we will read that build-id perf.data table and lookup in the build-id
> cache to get the matching symtab (and full elf .ko file to do things
> like annotation).

That's the desired behavior.  But "perf report" didn't use modules in
the build-id cache when it used kcore (and kallsyms).

Thanks,
Namhyung


>  
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Wang Nan <wangnan0@huawei.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/symbol.c | 45 +++++++++++++++++++++++----------------------
> >  1 file changed, 23 insertions(+), 22 deletions(-)
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index ce79a51f25bf..fe46eb782297 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1155,6 +1155,7 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >  	int err, fd;
> >  	char kcore_filename[PATH_MAX];
> >  	struct symbol *sym;
> > +	struct map_groups old_mg;
> >  
> >  	if (!kmaps)
> >  		return -EINVAL;
> > @@ -1196,15 +1197,8 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >  		goto out_err;
> >  	}
> >  
> > -	/* Remove old maps */
> > -	old_map = map_groups__first(kmaps, map->type);
> > -	while (old_map) {
> > -		struct map *next = map_groups__next(old_map);
> > -
> > -		if (old_map != map)
> > -			map_groups__remove(kmaps, old_map);
> > -		old_map = next;
> > -	}
> > +	old_mg = *kmaps;
> > +	kmaps->maps[map->type].entries = RB_ROOT;
> >  
> >  	/* Find the kernel map using the first symbol */
> >  	sym = dso__first_symbol(dso, map->type);
> > @@ -1223,24 +1217,31 @@ static int dso__load_kcore(struct dso *dso, struct map *map,
> >  	while (!list_empty(&md.maps)) {
> >  		new_map = list_entry(md.maps.next, struct map, node);
> >  		list_del_init(&new_map->node);
> > -		if (new_map == replacement_map) {
> > -			map->start	= new_map->start;
> > -			map->end	= new_map->end;
> > -			map->pgoff	= new_map->pgoff;
> > -			map->map_ip	= new_map->map_ip;
> > -			map->unmap_ip	= new_map->unmap_ip;
> > -			/* Ensure maps are correctly ordered */
> > -			map__get(map);
> > -			map_groups__remove(kmaps, map);
> > -			map_groups__insert(kmaps, map);
> > -			map__put(map);
> > -		} else {
> > -			map_groups__insert(kmaps, new_map);
> > +
> > +		map_groups__insert(kmaps, new_map);
> > +
> > +		if (new_map != replacement_map) {
> > +			old_map = map_groups__find(&old_mg, new_map->type, new_map->start);
> > +			if (old_map && dso__loaded(old_map->dso, old_map->type)) {
> > +				new_map->pgoff = old_map->pgoff;
> > +
> > +				dso__put(new_map->dso);
> > +				new_map->dso = dso__get(old_map->dso);
> > +			}
> >  		}
> >  
> >  		map__put(new_map);
> >  	}
> >  
> > +	/* Remove old maps */
> > +	old_map = map_groups__first(&old_mg, map->type);
> > +	while (old_map) {
> > +		struct map *next = map_groups__next(old_map);
> > +
> > +		map_groups__remove(&old_mg, old_map);
> > +		old_map = next;
> > +	}
> > +
> >  	/*
> >  	 * Set the data type and long name so that kcore can be read via
> >  	 * dso__data_read_addr().
> > -- 
> > 2.13.1

Re: [PATCH/RFC 4/9] perf symbols: Load kernel module symbols ASAP

[Namhyung Kim]

On Fri, Jun 23, 2017 at 11:26:04AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 23, 2017 at 02:48:22PM +0900, Namhyung Kim escreveu:
> > When loading kernel symbols from /proc/kallsyms, it might have different
> > addresses for modules.  We should honor the mmap event recorded in a
> > perf.data so load the module symbols when it sees the event so that it
> > cannot be overridden by symbols in /proc/kallsyms later.
>  
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Wang Nan <wangnan0@huawei.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/machine.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index d838f893e639..799efe920f0c 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -656,6 +656,9 @@ struct map *machine__findnew_module_map(struct machine *machine, u64 start,
> >  
> >  	map_groups__insert(&machine->kmaps, map);
> >  
> > +	if (dso->has_build_id)
> > +		map__load(map);
> 
> How could this be set at this point? I just tried processing a
> PERF_RECORD_MMAP event for a kernel module and at this point it is set
> to false :-\
> 
> Humm, but I did it for video.ko, that has no hits in this case, so this
> must be coming from reading the perf.data build-id table... /me tries
> that...  yeah, when processing the buildid table in perf.data we
> populate the machine->dsos list and set the dso->has_build_id flag:
> 
> [root@jouet ~]# perf buildid-list | grep .ko
> 57a47c2f0d85ef5726b80e8f55403c3f508a4eac /lib/modules/4.12.0-rc4+/kernel/drivers/net/wireless/intel/iwlwifi/iwlwifi.ko
> [root@jouet ~]# 
> (gdb) bt
> #0  machine__findnew_module_map (machine=0x21c6c78, start=18446744072647282688, filename=0x7fd8fc057a00 "/lib/modules/4.12.0-rc4+/kernel/drivers/net/wireless/intel/iwlwifi/iwlwifi.ko")
>     at util/machine.c:650
> #1  0x00000000005094a3 in machine__process_kernel_mmap_event (machine=0x21c6c78, event=0x7fd8fc0579d8) at util/machine.c:1274
> #2  0x00000000005099de in machine__process_mmap_event (machine=0x21c6c78, event=0x7fd8fc0579d8, sample=0x7fffffffbef0) at util/machine.c:1433
> #3  0x00000000004cc021 in perf_event__process_mmap (tool=0x7fffffffc440, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, machine=0x21c6c78) at util/event.c:1265
> #4  0x0000000000512686 in machines__deliver_event (machines=0x21c6c78, evlist=0x21c6f30, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, tool=0x7fffffffc440, file_offset=10712)
>     at util/session.c:1250
> #5  0x00000000005129ce in perf_session__deliver_event (session=0x21c6b90, event=0x7fd8fc0579d8, sample=0x7fffffffbef0, tool=0x7fffffffc440, file_offset=10712) at util/session.c:1310
> #6  0x0000000000513255 in perf_session__process_event (session=0x21c6b90, event=0x7fd8fc0579d8, file_offset=10712) at util/session.c:1490
> #7  0x0000000000513ff4 in __perf_session__process_events (session=0x21c6b90, data_offset=232, data_size=23120, file_size=23352) at util/session.c:1867
> #8  0x00000000005141f2 in perf_session__process_events (session=0x21c6b90) at util/session.c:1921
> #9  0x00000000004427c9 in __cmd_report (rep=0x7fffffffc440) at builtin-report.c:575
> #10 0x000000000044427c in cmd_report (argc=0, argv=0x7fffffffe160) at builtin-report.c:1054
> #11 0x00000000004bad91 in run_builtin (p=0xa28fd0 <commands+240>, argc=2, argv=0x7fffffffe160) at perf.c:296
> #12 0x00000000004baffe in handle_internal_command (argc=2, argv=0x7fffffffe160) at perf.c:348
> #13 0x00000000004bb150 in run_argv (argcp=0x7fffffffdfbc, argv=0x7fffffffdfb0) at perf.c:392
> #14 0x00000000004bb52a in main (argc=2, argv=0x7fffffffe160) at perf.c:530
> (gdb) p dso
> $127 = (struct dso *) 0x21c7dc0
> (gdb) p dso->has_build_id
> $128 = 1 '\001'
> (gdb) 
> 
> So what you're doing is to load it here... perhaps we should not do
> that, and instead in the kallsyms loading code, when processing the
> symbols for this specific module, discard them and leave its symbol
> table unpopulated, then, later, when resolving a sample in this module,
> it will see that it is not loaded, will see that it has a build-id and
> will use that, and _refuse_ to get it from anywhere else than an ELF
> file with that build-id.
> 
> I.e. in general, when we have a build-id for a given DSO, we should not
> read it from kallsyms or any other file that don't have a matching
> build-id.
> 
> The only possibility would be: hey, I have a build id, and reading the
> currently loaded module (/sys/module/e1000e/notes/.note.gnu.build-id) I
> see it _still_ has that same build-id, ok, I can read it from kallsyms.
> 
> What do you think?

I'm ok with that, will change to compare build-id before loading
symbols.

Thanks,
Namhyung

Re: [PATCH/RFC 5/9] perf symbols: Fixup the end address of kernel map properly

[Namhyung Kim]

On Fri, Jun 23, 2017 at 11:27:25AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 23, 2017 at 02:48:23PM +0900, Namhyung Kim escreveu:
> > When /proc/kallsyms is used for kernel address, addresses in module can
> > be changed when the module is reloaded.  So if one did perf record with
> > some module and then for some reason reload the module.  Then perf
> > report might see a different address for the module and the output can
> > show incorrect symbols.
> 
> If we, as mentioned in another message, make sure that no module reload
> is performed while perf record is running, and then refuse to use
> kallsyms if the buildid for the running module is different from the one
> in the perf.data build id header, then this is solved, no?

No.  This can happen when a (same) module is reload at a different
address.  Even if we refuse the symbols of the module in
dso__split_kallsyms(), the last symbols in the kernel dso was already
fixed up with a different address and then the map end address would
be invalid.  This can make the module map overlapped to the kernel
map, so invisible when it finds maps for samples.

Thanks,
Namhyung


>  
> > For example, let a module XXX was loaded at 0xffffffff8a000000, the
> > /proc/kallsyms might show following:
> > 
> >   ...
> >   0xffffffff81234560 t last_symbol_in_vmlinux
> >   0xffffffff8a000000 t first_symbol_in_XXX
> >   ...
> > 
> > As kallsyms fixs up the symbol and map address by using next address,
> > the end address of last_symbol and kernel map would be start address of
> > XXX (0xffffffff8a000000).  And samples in 0xffffffff8a001234 can be
> > found in a map for XXX module.
> > 
> > But later, XXX was reloaded at 0xffffffff8a007000, slightly higher than
> > before.  Now reading /proc/kallsyms tells that the end address of last
> > symbol would be 0xfffffff8a007000 and so kernel map is same.  Now
> > samples in 0xffffffff8a001234 - formerly in the XXX module - would go to
> > the kernel map and show no symbols.
> > 
> > In this case, perf can know the address of map of XXX from mmap event in
> > perf.data so it can adjust kernel map address using the address of XXX
> > map.  To do that, replace map__fixup_end() by __map_groups__fixup_end().
> > This still have incorrect end address of last symbol in the kernel map
> > but it's ok since samples in that address wouldn't go to the kernel map
> > anyway.
> > 
> > Cc: Adrian Hunter <adrian.hunter@intel.com>
> > Cc: Wang Nan <wangnan0@huawei.com>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  tools/perf/util/symbol.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 74078ba595b3..ce79a51f25bf 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1832,7 +1832,7 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
> >  		dso->binary_type = DSO_BINARY_TYPE__KALLSYMS;
> >  		dso__set_long_name(dso, DSO__NAME_KALLSYMS, false);
> >  		map__fixup_start(map);
> > -		map__fixup_end(map);
> > +		__map_groups__fixup_end(map->groups, map->type);
> >  	}
> >  
> >  	return err;
> > @@ -1880,7 +1880,7 @@ static int dso__load_guest_kernel_sym(struct dso *dso, struct map *map)
> >  		machine__mmap_name(machine, path, sizeof(path));
> >  		dso__set_long_name(dso, strdup(path), true);
> >  		map__fixup_start(map);
> > -		map__fixup_end(map);
> > +		__map_groups__fixup_end(map->groups, map->type);
> >  	}
> >  
> >  	return err;
> > -- 
> > 2.13.1

Re: [PATCH/RFC 9/9] perf record: Add --module-dir option

[Namhyung Kim]

On Fri, Jun 23, 2017 at 11:45:24AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jun 23, 2017 at 02:48:27PM +0900, Namhyung Kim escreveu:
> > Currently perf only searches module binaries on the canonical
> > directory (/lib/modules/`uname -r`).  But sometimes user needs to load
> > local modules.  These cannot be copied to the build-id cache since long
> > name (i.e. real path) of DSOs was not set.
>  
> > This patch fixes the problem by adding a new --module-dir option so that
> > perf can record modules in the directory.
>  
> > +++ b/tools/perf/Documentation/perf-record.txt
> > @@ -480,6 +480,9 @@ Implies --tail-synthesize.
> > +--module-dir=PATH::
> > +Directory name where extra modules are located.
> 
> > +++ b/tools/perf/builtin-record.c
> > @@ -1671,6 +1671,8 @@ static struct option __record_options[] = {
> >  		    "Parse options then exit"),
> >  	OPT_BOOLEAN(0, "use-kcore", &symbol_conf.use_kcore,
> >  		    "Use /proc/kcore for object code"),
> > +	OPT_STRING(0, "module-dir", &symbol_conf.extra_module_path, "path",
> > +		   "directory name where extra modules are located"),
> >  	OPT_END()
> >  };
> >  
> > +++ b/tools/perf/util/machine.c
> > @@ -1117,7 +1118,19 @@ static int machine__set_modules_path(struct machine *machine)
> >  		 machine->root_dir, version);
> >  	free(version);
> >  
> > -	return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> > +	ret = map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if (symbol_conf.extra_module_path) {
> > +		snprintf(modules_path, sizeof(modules_path), "%s/%s",
> > +			 machine->root_dir, symbol_conf.extra_module_path);
> > +
> > +		ret = map_groups__set_modules_path_dir(&machine->kmaps,
> > +						       modules_path, 0);
> 
> What if we have samples in a module that is in the canonical dir _and_
> samples in a module in this extra_module_path? Shouldn't we have
> something like vmlinux_path, but for modules? Where you can add entries
> in that path?
> 
> I'm ok with adding entries to where module files are looked up, with
> semantics similar to $PATH in a shell, i.e. entries added via
> --module-path (rename of your --module-dir) will take precedence over
> the canonical dir (/lib/modules/`uname -r`).

I think we can check the build-id in sysfs and binaries in both
directories.  If the sysfs is not avaiable (i.e. the module is
unloaded in the meantime), the precedence can be used IMHO.

Thanks,
Namhyung


> 
> But for the future I think we should try to get a PERF_RECORD_MMAP that
> will allow reloading modules during a 'perf record' session when a
> module gets reloaded, I thought about using existing tracepoints, but...
> 
> [root@jouet ~]# cat /sys/kernel/debug/tracing/events/module/module_load/format 
> name: module_load
> ID: 370
> format:
> 	field:unsigned short common_type;	offset:0;	size:2;	signed:0;
> 	field:unsigned char common_flags;	offset:2;	size:1;	signed:0;
> 	field:unsigned char common_preempt_count;	offset:3;	size:1;	signed:0;
> 	field:int common_pid;	offset:4;	size:4;	signed:1;
> 
> 	field:unsigned int taints;	offset:8;	size:4;	signed:0;
> 	field:__data_loc char[] name;	offset:12;	size:4;	signed:1;
> 
> print fmt: "%s %s", __get_str(name), __print_flags(REC->taints, "", { (1UL << 0), "P" }, { (1UL << 12), "O" }, { (1UL << 1), "F" }, { (1UL << 10), "C" }, { (1UL << 13), "E" })
> [root@jouet ~]# perf trace --no-syscalls --event module:module_load modprobe ppp
> modprobe: FATAL: Module ppp not found in directory /lib/modules/4.12.0-rc4+
> [root@jouet ~]# perf trace --no-syscalls --event module:module_load modprobe e1000
>      0.000 module:module_load:e1000 )
> [root@jouet ~]#
> 
> It just gets us the module name, not the file from what it is loaded, bummer :-\
> 
> Something like:
> 
> diff --git a/kernel/module.c b/kernel/module.c
> index 4a3665f8f837..50e9718a141d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -3739,6 +3739,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
>  
>  	/* Done! */
>  	trace_module_load(mod);
> +	perf_event_mmap_mod(mod);
>  
>  	return do_init_module(mod);
>  
>  ----------
> 
> struct module *mod has:
> 
>   mod->name:		"e1000"
>   mod->debug->filename: /lib/modules/4.11.0-rc8+/kernel/drivers/net/ethernet/intel/e1000/e1000.ko
> 
> and at that point we also have load_info, htat has ELF headers where we could extract the build-id
> and insert it in a field in a new PERF_RECORD_MMAP3 record :-)
> 
>   perf_event_mmap_mod() would be modelled after perf_event_mmap(vma), but getting what
>   it needs not from the vma present in a sys_mmap() but from struct module and load_info.
> 
> - Arnaldo
> 
> > +	}
> > +
> > +	return ret;
> >  }
> >  int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
> >  				const char *name __maybe_unused)
> > diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> > index 88361eeae813..59370ceb87c4 100644
> > --- a/tools/perf/util/symbol.h
> > +++ b/tools/perf/util/symbol.h
> > @@ -124,7 +124,8 @@ struct symbol_conf {
> >  	const char	*vmlinux_name,
> >  			*kallsyms_name,
> >  			*source_prefix,
> > -			*field_sep;
> > +			*field_sep,
> > +			*extra_module_path;
> >  	const char	*default_guest_vmlinux_name,
> >  			*default_guest_kallsyms,
> >  			*default_guest_modules;
> > -- 
> > 2.13.1