Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1673282 > unrolled thread

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

Started byNamhyung Kim <namhyung@kernel.org>
First post2017-06-23 08:00 +0200
Last post2017-06-25 16:50 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCHSET/RFC 0/9] perf tools: Support out-of-tree modules Namhyung Kim <namhyung@kernel.org> - 2017-06-23 08:00 +0200
    [PATCH/RFC 9/9] perf record: Add --module-dir option Namhyung Kim <namhyung@kernel.org> - 2017-06-23 08:00 +0200
      Re: [PATCH/RFC 9/9] perf record: Add --module-dir option Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-06-23 16:50 +0200
        Re: [PATCH/RFC 9/9] perf record: Add --module-dir option Namhyung Kim <namhyung@kernel.org> - 2017-06-25 16:50 +0200

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

FromNamhyung Kim <namhyung@kernel.org>
Date2017-06-23 08:00 +0200
Subject[PATCHSET/RFC 0/9] perf tools: Support out-of-tree modules
Message-ID<tVpDH-6AF-5@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

FromNamhyung Kim <namhyung@kernel.org>
Date2017-06-23 08:00 +0200
Subject[PATCH/RFC 9/9] perf record: Add --module-dir option
Message-ID<tVpNp-6E0-27@gated-at.bofh.it>
In reply to#1673282
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

[toc] | [prev] | [next] | [standalone]

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

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2017-06-23 16:50 +0200
SubjectRe: [PATCH/RFC 9/9] perf record: Add --module-dir option
Message-ID<tVy4h-3pQ-1@gated-at.bofh.it>
In reply to#1673285
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

[toc] | [prev] | [next] | [standalone]

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

FromNamhyung Kim <namhyung@kernel.org>
Date2017-06-25 16:50 +0200
SubjectRe: [PATCH/RFC 9/9] perf record: Add --module-dir option
Message-ID<tWh1n-6hv-9@gated-at.bofh.it>
In reply to#1673613
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web