All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] perf probe: Fix offline module name missmatch issue
@ 2016-04-25 10:38 Ravi Bangoria
  2016-04-25 10:38 ` [RFC] perf probe: Fix module probe issue if no dwarf support Ravi Bangoria
  2016-04-25 21:24 ` [RFC] perf probe: Fix offline module name missmatch issue Masami Hiramatsu
  0 siblings, 2 replies; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-25 10:38 UTC (permalink / raw)
  To: linux-kernel
  Cc: acme, peterz, mingo, alexander.shishkin, masami.hiramatsu.pt,
	wangnan0, namhyung, srikar, naveen.n.rao, Ravi Bangoria

Perf can add a probe on kernel module which has not been loaded yet.
Current implementation finds module name from path. But if filename
is different from actual module name then perf fails to register
probe while loading module because of mismatch in names. For example,
samples/kobject/kobject-example.ko is loaded as kobject_example.

Before applying patch:

  $ sudo ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
    Added new event:
      probe:foo_show       (on foo_show in kobject-example)

    You can now use it in all perf tools, such as:

    perf record -e probe:foo_show -aR sleep 1

  $ cat /sys/kernel/debug/tracing/kprobe_events
    p:probe/foo_show kobject-example:foo_show

  $ insmod kobject-example.ko

  $ lsmod
    Module                  Size  Used by
    kobject_example        16384  0

  Generate read to /sys/kernel/kobject_example/foo while recording data
  with below command
  $ sudo ./perf record -e probe:foo_show -a
    [ perf record: Woken up 1 times to write data ]
    [ perf record: Captured and wrote 0.093 MB perf.data ]

  $./perf report --stdio -F overhead,comm,dso,sym
    Error:
    The perf.data.old file has no samples!

After applying patch:

  $ sudo ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
    Added new event:
      probe:foo_show       (on foo_show in kobject_example)

    You can now use it in all perf tools, such as:

    perf record -e probe:foo_show -aR sleep 1

  $ sudo cat /sys/kernel/debug/tracing/kprobe_events
    p:probe/foo_show kobject_example:foo_show

  $ insmod kobject-example.ko

  $ lsmod
    Module                  Size  Used by
    kobject_example        16384  0

  Generate read to /sys/kernel/kobject_example/foo while recording data
  with below command
  $ sudo ./perf record -e probe:foo_show -a
    [ perf record: Woken up 1 times to write data ]
    [ perf record: Captured and wrote 0.097 MB perf.data (8 samples) ]

  $ sudo ./perf report  --stdio -F overhead,comm,dso,sym
    ...
    # Samples: 8  of event 'probe:foo_show'
    # Event count (approx.): 8
    #
    # Overhead  Command  Shared Object      Symbol
    # ........  .......  .................  ............
    #
       100.00%  cat      [kobject_example]  [k] foo_show

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 78 +++++++++++++++++++++++++++++++++++--------
 tools/perf/util/probe-event.h |  2 ++
 2 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 8319fbb..05d0905 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
 	return true;
 }
 
+/*
+ * NOTE:
+ * '.gnu.linkonce.this_module' section of kernel module elf directly
+ * maps to 'struct module' from linux/module.h. This section contains
+ * actual module name which will be used by kernel after loading it.
+ * But, we cannot use 'struct module' here since linux/module.h is not
+ * exposed to user-space. Offset of 'name' has remained same from long
+ * time, so hardcoding it here.
+ */
+#ifdef __LP64__
+#define MOD_NAME_OFFSET 24
+#else
+#define MOD_NAME_OFFSET 12
+#endif
+
+/*
+ * @module can be module name of module file path. In case of path,
+ * inspect elf and find out what is actual module name.
+ * Caller has to free mod_name after using it.
+ */
+char *find_module_name(const char *module)
+{
+	int fd;
+	Elf *elf;
+	GElf_Ehdr ehdr;
+	GElf_Shdr shdr;
+	Elf_Data *data;
+	Elf_Scn *sec;
+	char *mod_name = NULL;
+
+	fd = open(module, O_RDONLY);
+	if (fd < 0)
+		return NULL;
+
+	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
+	if (elf == NULL)
+		goto elf_err;
+
+	if (gelf_getehdr(elf, &ehdr) == NULL)
+		goto ret_err;
+
+	sec = elf_section_by_name(elf, &ehdr, &shdr,
+			".gnu.linkonce.this_module", NULL);
+	if (!sec)
+		goto ret_err;
+
+	data = elf_getdata(sec, NULL);
+	if (!data || !data->d_buf)
+		goto ret_err;
+
+	mod_name = strdup((char *)data->d_buf + MOD_NAME_OFFSET);
+
+ret_err:
+	elf_end(elf);
+elf_err:
+	close(fd);
+	return mod_name;
+}
+
 #ifdef HAVE_DWARF_SUPPORT
 
 static int kernel_get_module_dso(const char *module, struct dso **pdso)
@@ -583,32 +642,23 @@ static int add_module_to_probe_trace_events(struct probe_trace_event *tevs,
 					    int ntevs, const char *module)
 {
 	int i, ret = 0;
-	char *tmp;
+	char *mod_name;
 
 	if (!module)
 		return 0;
 
-	tmp = strrchr(module, '/');
-	if (tmp) {
-		/* This is a module path -- get the module name */
-		module = strdup(tmp + 1);
-		if (!module)
-			return -ENOMEM;
-		tmp = strchr(module, '.');
-		if (tmp)
-			*tmp = '\0';
-		tmp = (char *)module;	/* For free() */
-	}
+	mod_name = find_module_name(module);
 
 	for (i = 0; i < ntevs; i++) {
-		tevs[i].point.module = strdup(module);
+		tevs[i].point.module =
+			strdup(mod_name ? mod_name : module);
 		if (!tevs[i].point.module) {
 			ret = -ENOMEM;
 			break;
 		}
 	}
 
-	free(tmp);
+	free(mod_name);
 	return ret;
 }
 
diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
index e54e7b0..0468fa3 100644
--- a/tools/perf/util/probe-event.h
+++ b/tools/perf/util/probe-event.h
@@ -166,4 +166,6 @@ int e_snprintf(char *str, size_t size, const char *format, ...)
 int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
 			    struct perf_probe_arg *pvar);
 
+char *find_module_name(const char *module);
+
 #endif /*_PROBE_EVENT_H */
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [RFC] perf probe: Fix module probe issue if no dwarf support
  2016-04-25 10:38 [RFC] perf probe: Fix offline module name missmatch issue Ravi Bangoria
@ 2016-04-25 10:38 ` Ravi Bangoria
  2016-04-25 21:29   ` Masami Hiramatsu
  2016-04-25 21:24 ` [RFC] perf probe: Fix offline module name missmatch issue Masami Hiramatsu
  1 sibling, 1 reply; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-25 10:38 UTC (permalink / raw)
  To: linux-kernel
  Cc: acme, peterz, mingo, alexander.shishkin, masami.hiramatsu.pt,
	wangnan0, namhyung, srikar, naveen.n.rao, Ravi Bangoria

Perf is not able to register probe in kernel module when dwarf supprt
is not there(and so it goes for symtab). Perf passes full path of
module where only module name is required which is causing the problem.
This patch fixes this issue.

Before applying patch:

  $ dpkg -s libdw-dev
    dpkg-query: package 'libdw-dev' is not installed ...

  $ ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
    Added new event:
      probe:foo_show (on foo_show in /linux/samples/kobject/kobject-example.ko)

    You can now use it in all perf tools, such as:

  perf record -e probe:foo_show -aR sleep 1

  $ cat /sys/kernel/debug/tracing/kprobe_events
    p:probe/foo_show /linux/samples/kobject/kobject-example.ko:foo_show

After applying patch:

  $ ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
    Added new event:
      probe:foo_show (on foo_show in kobject_example)

    You can now use it in all perf tools, such as:

  perf record -e probe:foo_show -aR sleep 1

  $ cat /sys/kernel/debug/tracing/kprobe_events
    p:probe/foo_show kobject_example:foo_show

Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
---
 tools/perf/util/probe-event.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index 05d0905..54e6a5a 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -2566,6 +2566,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 	struct probe_trace_point *tp;
 	int num_matched_functions;
 	int ret, i, j, skipped = 0;
+	char *mod_name;
 
 	map = get_target_map(pev->target, pev->uprobes);
 	if (!map) {
@@ -2650,9 +2651,19 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
 		tp->realname = strdup_or_goto(sym->name, nomem_out);
 
 		tp->retprobe = pp->retprobe;
-		if (pev->target)
-			tev->point.module = strdup_or_goto(pev->target,
-							   nomem_out);
+		if (pev->target) {
+			if (pev->uprobes) {
+				tev->point.module = strdup_or_goto(pev->target,
+								   nomem_out);
+			} else {
+				mod_name = find_module_name(pev->target);
+				tev->point.module =
+					strdup(mod_name ? mod_name : pev->target);
+				free(mod_name);
+				if (!tev->point.module)
+					goto nomem_out;
+			}
+		}
 		tev->uprobes = pev->uprobes;
 		tev->nargs = pev->nargs;
 		if (tev->nargs) {
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-25 10:38 [RFC] perf probe: Fix offline module name missmatch issue Ravi Bangoria
  2016-04-25 10:38 ` [RFC] perf probe: Fix module probe issue if no dwarf support Ravi Bangoria
@ 2016-04-25 21:24 ` Masami Hiramatsu
  2016-04-26  2:19   ` Masami Hiramatsu
  2016-04-26  8:56   ` Ravi Bangoria
  1 sibling, 2 replies; 12+ messages in thread
From: Masami Hiramatsu @ 2016-04-25 21:24 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, wangnan0, namhyung, srikar, naveen.n.rao

Hi Ravi,

On Mon, 25 Apr 2016 16:08:27 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Perf can add a probe on kernel module which has not been loaded yet.
> Current implementation finds module name from path. But if filename
> is different from actual module name then perf fails to register
> probe while loading module because of mismatch in names. For example,
> samples/kobject/kobject-example.ko is loaded as kobject_example.

Ah! right, good catch!
Have some comment below;

>
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 8319fbb..05d0905 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>  	return true;
>  }
>  
> +/*
> + * NOTE:
> + * '.gnu.linkonce.this_module' section of kernel module elf directly
> + * maps to 'struct module' from linux/module.h. This section contains
> + * actual module name which will be used by kernel after loading it.
> + * But, we cannot use 'struct module' here since linux/module.h is not
> + * exposed to user-space. Offset of 'name' has remained same from long
> + * time, so hardcoding it here.
> + */
> +#ifdef __LP64__
> +#define MOD_NAME_OFFSET 24
> +#else
> +#define MOD_NAME_OFFSET 12
> +#endif
> +
> +/*
> + * @module can be module name of module file path. In case of path,
> + * inspect elf and find out what is actual module name.
> + * Caller has to free mod_name after using it.
> + */
> +char *find_module_name(const char *module)

Could you make this function static, since there is no caller outside
this file?

> +{
> +	int fd;
> +	Elf *elf;
> +	GElf_Ehdr ehdr;
> +	GElf_Shdr shdr;
> +	Elf_Data *data;
> +	Elf_Scn *sec;
> +	char *mod_name = NULL;
> +
> +	fd = open(module, O_RDONLY);
> +	if (fd < 0)
> +		return NULL;
> +
> +	elf = elf_begin(fd, PERF_ELF_C_READ_MMAP, NULL);
> +	if (elf == NULL)
> +		goto elf_err;
> +
> +	if (gelf_getehdr(elf, &ehdr) == NULL)
> +		goto ret_err;
> +
> +	sec = elf_section_by_name(elf, &ehdr, &shdr,
> +			".gnu.linkonce.this_module", NULL);
> +	if (!sec)
> +		goto ret_err;
> +
> +	data = elf_getdata(sec, NULL);
> +	if (!data || !data->d_buf)
> +		goto ret_err;
> +
> +	mod_name = strdup((char *)data->d_buf + MOD_NAME_OFFSET);
> +
> +ret_err:
> +	elf_end(elf);
> +elf_err:
> +	close(fd);
> +	return mod_name;
> +}
> +
>  #ifdef HAVE_DWARF_SUPPORT
>  
>  static int kernel_get_module_dso(const char *module, struct dso **pdso)
> @@ -583,32 +642,23 @@ static int add_module_to_probe_trace_events(struct probe_trace_event *tevs,
>  					    int ntevs, const char *module)
>  {
>  	int i, ret = 0;
> -	char *tmp;
> +	char *mod_name;
>  
>  	if (!module)
>  		return 0;
>  
> -	tmp = strrchr(module, '/');
> -	if (tmp) {
> -		/* This is a module path -- get the module name */
> -		module = strdup(tmp + 1);
> -		if (!module)
> -			return -ENOMEM;
> -		tmp = strchr(module, '.');
> -		if (tmp)
> -			*tmp = '\0';
> -		tmp = (char *)module;	/* For free() */
> -	}
> +	mod_name = find_module_name(module);
>  
>  	for (i = 0; i < ntevs; i++) {
> -		tevs[i].point.module = strdup(module);
> +		tevs[i].point.module =
> +			strdup(mod_name ? mod_name : module);
>  		if (!tevs[i].point.module) {
>  			ret = -ENOMEM;
>  			break;
>  		}
>  	}
>  
> -	free(tmp);
> +	free(mod_name);
>  	return ret;
>  }
>  
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index e54e7b0..0468fa3 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -166,4 +166,6 @@ int e_snprintf(char *str, size_t size, const char *format, ...)
>  int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
>  			    struct perf_probe_arg *pvar);
>  
> +char *find_module_name(const char *module);

And remove this.

Others looks good to me!

Thank you,

> +
>  #endif /*_PROBE_EVENT_H */
> -- 
> 2.1.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix module probe issue if no dwarf support
  2016-04-25 10:38 ` [RFC] perf probe: Fix module probe issue if no dwarf support Ravi Bangoria
@ 2016-04-25 21:29   ` Masami Hiramatsu
  2016-04-26  9:04     ` Ravi Bangoria
  0 siblings, 1 reply; 12+ messages in thread
From: Masami Hiramatsu @ 2016-04-25 21:29 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, wangnan0, namhyung, srikar, naveen.n.rao

On Mon, 25 Apr 2016 16:08:28 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Perf is not able to register probe in kernel module when dwarf supprt
> is not there(and so it goes for symtab). Perf passes full path of
> module where only module name is required which is causing the problem.
> This patch fixes this issue.
> 
> Before applying patch:
> 
>   $ dpkg -s libdw-dev
>     dpkg-query: package 'libdw-dev' is not installed ...
> 
>   $ ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
>     Added new event:
>       probe:foo_show (on foo_show in /linux/samples/kobject/kobject-example.ko)
> 
>     You can now use it in all perf tools, such as:
> 
>   perf record -e probe:foo_show -aR sleep 1
> 
>   $ cat /sys/kernel/debug/tracing/kprobe_events
>     p:probe/foo_show /linux/samples/kobject/kobject-example.ko:foo_show
> 
> After applying patch:
> 
>   $ ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
>     Added new event:
>       probe:foo_show (on foo_show in kobject_example)
> 
>     You can now use it in all perf tools, such as:
> 
>   perf record -e probe:foo_show -aR sleep 1
> 
>   $ cat /sys/kernel/debug/tracing/kprobe_events
>     p:probe/foo_show kobject_example:foo_show
> 

Looks good to me :)
However, it seems that this patch depends on your previous patch
("perf probe: Fix offline module name missmatch issue")
In that case, could you make these a series of patches?

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,

> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/util/probe-event.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 05d0905..54e6a5a 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -2566,6 +2566,7 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  	struct probe_trace_point *tp;
>  	int num_matched_functions;
>  	int ret, i, j, skipped = 0;
> +	char *mod_name;
>  
>  	map = get_target_map(pev->target, pev->uprobes);
>  	if (!map) {
> @@ -2650,9 +2651,19 @@ static int find_probe_trace_events_from_map(struct perf_probe_event *pev,
>  		tp->realname = strdup_or_goto(sym->name, nomem_out);
>  
>  		tp->retprobe = pp->retprobe;
> -		if (pev->target)
> -			tev->point.module = strdup_or_goto(pev->target,
> -							   nomem_out);
> +		if (pev->target) {
> +			if (pev->uprobes) {
> +				tev->point.module = strdup_or_goto(pev->target,
> +								   nomem_out);
> +			} else {
> +				mod_name = find_module_name(pev->target);
> +				tev->point.module =
> +					strdup(mod_name ? mod_name : pev->target);
> +				free(mod_name);
> +				if (!tev->point.module)
> +					goto nomem_out;
> +			}
> +		}
>  		tev->uprobes = pev->uprobes;
>  		tev->nargs = pev->nargs;
>  		if (tev->nargs) {
> -- 
> 2.1.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-25 21:24 ` [RFC] perf probe: Fix offline module name missmatch issue Masami Hiramatsu
@ 2016-04-26  2:19   ` Masami Hiramatsu
  2016-04-26  9:00     ` Ravi Bangoria
  2016-04-26  8:56   ` Ravi Bangoria
  1 sibling, 1 reply; 12+ messages in thread
From: Masami Hiramatsu @ 2016-04-26  2:19 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: Ravi Bangoria, linux-kernel, acme, peterz, mingo,
	alexander.shishkin, masami.hiramatsu.pt, wangnan0, namhyung,
	srikar, naveen.n.rao

On Tue, 26 Apr 2016 06:24:38 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:
> > +/*
> > + * NOTE:
> > + * '.gnu.linkonce.this_module' section of kernel module elf directly
> > + * maps to 'struct module' from linux/module.h. This section contains
> > + * actual module name which will be used by kernel after loading it.
> > + * But, we cannot use 'struct module' here since linux/module.h is not
> > + * exposed to user-space. Offset of 'name' has remained same from long
> > + * time, so hardcoding it here.
> > + */

BTW, is there no way to get the module name avoiding to access
this "hidden" data structure?
This looks very tricky way...

Thanks,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-25 21:24 ` [RFC] perf probe: Fix offline module name missmatch issue Masami Hiramatsu
  2016-04-26  2:19   ` Masami Hiramatsu
@ 2016-04-26  8:56   ` Ravi Bangoria
  2016-04-26  9:15     ` Wangnan (F)
  2016-04-26 10:46     ` Masami Hiramatsu
  1 sibling, 2 replies; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-26  8:56 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, wangnan0, namhyung, srikar, naveen.n.rao

Thanks Masami for reviewing.

Please find my replies to your comment.

On Tuesday 26 April 2016 02:54 AM, Masami Hiramatsu wrote:
> Hi Ravi,
>
> On Mon, 25 Apr 2016 16:08:27 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Perf can add a probe on kernel module which has not been loaded yet.
>> Current implementation finds module name from path. But if filename
>> is different from actual module name then perf fails to register
>> probe while loading module because of mismatch in names. For example,
>> samples/kobject/kobject-example.ko is loaded as kobject_example.
> Ah! right, good catch!
> Have some comment below;
>
>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>> index 8319fbb..05d0905 100644
>> --- a/tools/perf/util/probe-event.c
>> +++ b/tools/perf/util/probe-event.c
>> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>>   	return true;
>>   }
>>   
>> +/*
>> + * NOTE:
>> + * '.gnu.linkonce.this_module' section of kernel module elf directly
>> + * maps to 'struct module' from linux/module.h. This section contains
>> + * actual module name which will be used by kernel after loading it.
>> + * But, we cannot use 'struct module' here since linux/module.h is not
>> + * exposed to user-space. Offset of 'name' has remained same from long
>> + * time, so hardcoding it here.
>> + */
>> +#ifdef __LP64__
>> +#define MOD_NAME_OFFSET 24
>> +#else
>> +#define MOD_NAME_OFFSET 12
>> +#endif
>> +
>> +/*
>> + * @module can be module name of module file path. In case of path,
>> + * inspect elf and find out what is actual module name.
>> + * Caller has to free mod_name after using it.
>> + */
>> +char *find_module_name(const char *module)
> Could you make this function static, since there is no caller outside
> this file?

Yes. no caller outside of this file. But,

In this patch, function find_module_name is defined outside of
#ifdef HAVE_DWARF_SUPPORT while it's being called from inside of
#ifdef HAVE_DWARF_SUPPORT.

If I make it static and if there is no dwarf support, there will be 
compilation
error about function defined but not used.

And in second patch("perf probe: Fix module probe issue if no dwarf 
support"),
I'm calling it from outside of #ifdef HAVE_DWARF_SUPPORT.

So I have two options:
1. merge both the patches and make definition as static
2. make function static in second patch

I've chose second approach and sent v2. But please let me know if there is
better way to do it.

Regards,
Ravi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-26  2:19   ` Masami Hiramatsu
@ 2016-04-26  9:00     ` Ravi Bangoria
  0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-26  9:00 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, wangnan0, namhyung, srikar, naveen.n.rao

Thanks Masami,

On Tuesday 26 April 2016 07:49 AM, Masami Hiramatsu wrote:
> On Tue, 26 Apr 2016 06:24:38 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
>>> +/*
>>> + * NOTE:
>>> + * '.gnu.linkonce.this_module' section of kernel module elf directly
>>> + * maps to 'struct module' from linux/module.h. This section contains
>>> + * actual module name which will be used by kernel after loading it.
>>> + * But, we cannot use 'struct module' here since linux/module.h is not
>>> + * exposed to user-space. Offset of 'name' has remained same from long
>>> + * time, so hardcoding it here.
>>> + */
> BTW, is there no way to get the module name avoiding to access
> this "hidden" data structure?
> This looks very tricky way...

So this is the same approach kernel use to find module name when module is
loaded. Please refer this function for more detail:

kernel/module.c ::  static struct module *setup_load_info(...)

Regards,
Ravi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix module probe issue if no dwarf support
  2016-04-25 21:29   ` Masami Hiramatsu
@ 2016-04-26  9:04     ` Ravi Bangoria
  0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-26  9:04 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, wangnan0, namhyung, srikar, naveen.n.rao



On Tuesday 26 April 2016 02:59 AM, Masami Hiramatsu wrote:
> On Mon, 25 Apr 2016 16:08:28 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Perf is not able to register probe in kernel module when dwarf supprt
>> is not there(and so it goes for symtab). Perf passes full path of
>> module where only module name is required which is causing the problem.
>> This patch fixes this issue.
>>
>> Before applying patch:
>>
>>    $ dpkg -s libdw-dev
>>      dpkg-query: package 'libdw-dev' is not installed ...
>>
>>    $ ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
>>      Added new event:
>>        probe:foo_show (on foo_show in /linux/samples/kobject/kobject-example.ko)
>>
>>      You can now use it in all perf tools, such as:
>>
>>    perf record -e probe:foo_show -aR sleep 1
>>
>>    $ cat /sys/kernel/debug/tracing/kprobe_events
>>      p:probe/foo_show /linux/samples/kobject/kobject-example.ko:foo_show
>>
>> After applying patch:
>>
>>    $ ./perf probe -m /linux/samples/kobject/kobject-example.ko foo_show
>>      Added new event:
>>        probe:foo_show (on foo_show in kobject_example)
>>
>>      You can now use it in all perf tools, such as:
>>
>>    perf record -e probe:foo_show -aR sleep 1
>>
>>    $ cat /sys/kernel/debug/tracing/kprobe_events
>>      p:probe/foo_show kobject_example:foo_show
>>
> Looks good to me :)
> However, it seems that this patch depends on your previous patch
> ("perf probe: Fix offline module name missmatch issue")
> In that case, could you make these a series of patches?
>
> Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks Masami,

I've sent v2 with changes you suggested. Please review it.

Regards,
Ravi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-26  8:56   ` Ravi Bangoria
@ 2016-04-26  9:15     ` Wangnan (F)
  2016-04-26 10:45       ` Ravi Bangoria
  2016-04-26 10:46     ` Masami Hiramatsu
  1 sibling, 1 reply; 12+ messages in thread
From: Wangnan (F) @ 2016-04-26  9:15 UTC (permalink / raw)
  To: Ravi Bangoria, Masami Hiramatsu
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, namhyung, srikar, naveen.n.rao



On 2016/4/26 16:56, Ravi Bangoria wrote:
> Thanks Masami for reviewing.
>
> Please find my replies to your comment.
>
> On Tuesday 26 April 2016 02:54 AM, Masami Hiramatsu wrote:
>> Hi Ravi,
>>
>> On Mon, 25 Apr 2016 16:08:27 +0530
>> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>
>>> Perf can add a probe on kernel module which has not been loaded yet.
>>> Current implementation finds module name from path. But if filename
>>> is different from actual module name then perf fails to register
>>> probe while loading module because of mismatch in names. For example,
>>> samples/kobject/kobject-example.ko is loaded as kobject_example.
>> Ah! right, good catch!
>> Have some comment below;
>>
>>> diff --git a/tools/perf/util/probe-event.c 
>>> b/tools/perf/util/probe-event.c
>>> index 8319fbb..05d0905 100644
>>> --- a/tools/perf/util/probe-event.c
>>> +++ b/tools/perf/util/probe-event.c
>>> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char 
>>> *symbol, unsigned long address)
>>>       return true;
>>>   }
>>>   +/*
>>> + * NOTE:
>>> + * '.gnu.linkonce.this_module' section of kernel module elf directly
>>> + * maps to 'struct module' from linux/module.h. This section contains
>>> + * actual module name which will be used by kernel after loading it.
>>> + * But, we cannot use 'struct module' here since linux/module.h is not
>>> + * exposed to user-space. Offset of 'name' has remained same from long
>>> + * time, so hardcoding it here.
>>> + */
>>> +#ifdef __LP64__
>>> +#define MOD_NAME_OFFSET 24
>>> +#else
>>> +#define MOD_NAME_OFFSET 12
>>> +#endif
>>> +
>>> +/*
>>> + * @module can be module name of module file path. In case of path,
>>> + * inspect elf and find out what is actual module name.
>>> + * Caller has to free mod_name after using it.
>>> + */
>>> +char *find_module_name(const char *module)
>> Could you make this function static, since there is no caller outside
>> this file?
>
> Yes. no caller outside of this file. But,
>
> In this patch, function find_module_name is defined outside of
> #ifdef HAVE_DWARF_SUPPORT while it's being called from inside of
> #ifdef HAVE_DWARF_SUPPORT.
>
> If I make it static and if there is no dwarf support, there will be 
> compilation
> error about function defined but not used.
>
> And in second patch("perf probe: Fix module probe issue if no dwarf 
> support"),
> I'm calling it from outside of #ifdef HAVE_DWARF_SUPPORT.
>
> So I have two options:
> 1. merge both the patches and make definition as static
> 2. make function static in second patch
>
> I've chose second approach and sent v2. But please let me know if 
> there is
> better way to do it.
>

Try __maybe_unused directive?

Thank you.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-26  9:15     ` Wangnan (F)
@ 2016-04-26 10:45       ` Ravi Bangoria
  0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-26 10:45 UTC (permalink / raw)
  To: Wangnan (F), Masami Hiramatsu
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, namhyung, srikar, naveen.n.rao



On Tuesday 26 April 2016 02:45 PM, Wangnan (F) wrote:
>
>
> On 2016/4/26 16:56, Ravi Bangoria wrote:
>> Thanks Masami for reviewing.
>>
>> Please find my replies to your comment.
>>
>> On Tuesday 26 April 2016 02:54 AM, Masami Hiramatsu wrote:
>>> Hi Ravi,
>>>
>>> On Mon, 25 Apr 2016 16:08:27 +0530
>>> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>>
>>>> Perf can add a probe on kernel module which has not been loaded yet.
>>>> Current implementation finds module name from path. But if filename
>>>> is different from actual module name then perf fails to register
>>>> probe while loading module because of mismatch in names. For example,
>>>> samples/kobject/kobject-example.ko is loaded as kobject_example.
>>> Ah! right, good catch!
>>> Have some comment below;
>>>
>>>> diff --git a/tools/perf/util/probe-event.c 
>>>> b/tools/perf/util/probe-event.c
>>>> index 8319fbb..05d0905 100644
>>>> --- a/tools/perf/util/probe-event.c
>>>> +++ b/tools/perf/util/probe-event.c
>>>> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char 
>>>> *symbol, unsigned long address)
>>>>       return true;
>>>>   }
>>>>   +/*
>>>> + * NOTE:
>>>> + * '.gnu.linkonce.this_module' section of kernel module elf directly
>>>> + * maps to 'struct module' from linux/module.h. This section contains
>>>> + * actual module name which will be used by kernel after loading it.
>>>> + * But, we cannot use 'struct module' here since linux/module.h is 
>>>> not
>>>> + * exposed to user-space. Offset of 'name' has remained same from 
>>>> long
>>>> + * time, so hardcoding it here.
>>>> + */
>>>> +#ifdef __LP64__
>>>> +#define MOD_NAME_OFFSET 24
>>>> +#else
>>>> +#define MOD_NAME_OFFSET 12
>>>> +#endif
>>>> +
>>>> +/*
>>>> + * @module can be module name of module file path. In case of path,
>>>> + * inspect elf and find out what is actual module name.
>>>> + * Caller has to free mod_name after using it.
>>>> + */
>>>> +char *find_module_name(const char *module)
>>> Could you make this function static, since there is no caller outside
>>> this file?
>>
>> Yes. no caller outside of this file. But,
>>
>> In this patch, function find_module_name is defined outside of
>> #ifdef HAVE_DWARF_SUPPORT while it's being called from inside of
>> #ifdef HAVE_DWARF_SUPPORT.
>>
>> If I make it static and if there is no dwarf support, there will be 
>> compilation
>> error about function defined but not used.
>>
>> And in second patch("perf probe: Fix module probe issue if no dwarf 
>> support"),
>> I'm calling it from outside of #ifdef HAVE_DWARF_SUPPORT.
>>
>> So I have two options:
>> 1. merge both the patches and make definition as static
>> 2. make function static in second patch
>>
>> I've chose second approach and sent v2. But please let me know if 
>> there is
>> better way to do it.
>>
>
> Try __maybe_unused directive?
>

Thanks Wangnan for suggestion,

Actually I tried to use __maybe_unused with definition of 
find_module_name but
it throws following compilation error:

util/probe-event.c:289:1: error: expected ‘,’ or ‘;’ before ‘{’ token
  {
  ^
util/probe-event.c:288:14: error: ‘find_module_name’ declared ‘static’ 
but never defined [-Werror=unused-function]
  static char *find_module_name(const char *module) __maybe_unused
               ^
   CC       util/zlib.o
cc1: all warnings being treated as errors


I've to declare prototype of function with __maybe_unused before it's 
definition to
resolve this error. And, anyway this is temporary and need to be removed 
in patch 2,
I think no need to do this change.

Regards,
Ravi

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-26  8:56   ` Ravi Bangoria
  2016-04-26  9:15     ` Wangnan (F)
@ 2016-04-26 10:46     ` Masami Hiramatsu
  2016-04-26 14:29       ` Ravi Bangoria
  1 sibling, 1 reply; 12+ messages in thread
From: Masami Hiramatsu @ 2016-04-26 10:46 UTC (permalink / raw)
  To: Ravi Bangoria
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin,
	masami.hiramatsu.pt, wangnan0, namhyung, srikar, naveen.n.rao

On Tue, 26 Apr 2016 14:26:48 +0530
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Thanks Masami for reviewing.
> 
> Please find my replies to your comment.
> 
> On Tuesday 26 April 2016 02:54 AM, Masami Hiramatsu wrote:
> > Hi Ravi,
> >
> > On Mon, 25 Apr 2016 16:08:27 +0530
> > Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
> >
> >> Perf can add a probe on kernel module which has not been loaded yet.
> >> Current implementation finds module name from path. But if filename
> >> is different from actual module name then perf fails to register
> >> probe while loading module because of mismatch in names. For example,
> >> samples/kobject/kobject-example.ko is loaded as kobject_example.
> > Ah! right, good catch!
> > Have some comment below;
> >
> >> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> >> index 8319fbb..05d0905 100644
> >> --- a/tools/perf/util/probe-event.c
> >> +++ b/tools/perf/util/probe-event.c
> >> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
> >>   	return true;
> >>   }
> >>   
> >> +/*
> >> + * NOTE:
> >> + * '.gnu.linkonce.this_module' section of kernel module elf directly
> >> + * maps to 'struct module' from linux/module.h. This section contains
> >> + * actual module name which will be used by kernel after loading it.
> >> + * But, we cannot use 'struct module' here since linux/module.h is not
> >> + * exposed to user-space. Offset of 'name' has remained same from long
> >> + * time, so hardcoding it here.
> >> + */
> >> +#ifdef __LP64__
> >> +#define MOD_NAME_OFFSET 24
> >> +#else
> >> +#define MOD_NAME_OFFSET 12
> >> +#endif
> >> +
> >> +/*
> >> + * @module can be module name of module file path. In case of path,
> >> + * inspect elf and find out what is actual module name.
> >> + * Caller has to free mod_name after using it.
> >> + */
> >> +char *find_module_name(const char *module)
> > Could you make this function static, since there is no caller outside
> > this file?
> 
> Yes. no caller outside of this file. But,
> 
> In this patch, function find_module_name is defined outside of
> #ifdef HAVE_DWARF_SUPPORT while it's being called from inside of
> #ifdef HAVE_DWARF_SUPPORT.
> 
> If I make it static and if there is no dwarf support, there will be 
> compilation
> error about function defined but not used.
> 
> And in second patch("perf probe: Fix module probe issue if no dwarf 
> support"),
> I'm calling it from outside of #ifdef HAVE_DWARF_SUPPORT.
> 
> So I have two options:
> 1. merge both the patches and make definition as static
> 2. make function static in second patch
> 
> I've chose second approach and sent v2. But please let me know if there is
> better way to do it.

Ah, I see.
In that case, you can swap the patch in the series and move find_module_name
in the other patch ;)

Thanks!

-- 
Masami Hiramatsu <mhiramat@kernel.org>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [RFC] perf probe: Fix offline module name missmatch issue
  2016-04-26 10:46     ` Masami Hiramatsu
@ 2016-04-26 14:29       ` Ravi Bangoria
  0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2016-04-26 14:29 UTC (permalink / raw)
  To: Masami Hiramatsu
  Cc: linux-kernel, acme, peterz, mingo, alexander.shishkin, wangnan0,
	namhyung, srikar, naveen.n.rao



On Tuesday 26 April 2016 04:16 PM, Masami Hiramatsu wrote:
> On Tue, 26 Apr 2016 14:26:48 +0530
> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>
>> Thanks Masami for reviewing.
>>
>> Please find my replies to your comment.
>>
>> On Tuesday 26 April 2016 02:54 AM, Masami Hiramatsu wrote:
>>> Hi Ravi,
>>>
>>> On Mon, 25 Apr 2016 16:08:27 +0530
>>> Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
>>>
>>>> Perf can add a probe on kernel module which has not been loaded yet.
>>>> Current implementation finds module name from path. But if filename
>>>> is different from actual module name then perf fails to register
>>>> probe while loading module because of mismatch in names. For example,
>>>> samples/kobject/kobject-example.ko is loaded as kobject_example.
>>> Ah! right, good catch!
>>> Have some comment below;
>>>
>>>> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
>>>> index 8319fbb..05d0905 100644
>>>> --- a/tools/perf/util/probe-event.c
>>>> +++ b/tools/perf/util/probe-event.c
>>>> @@ -265,6 +265,65 @@ static bool kprobe_warn_out_range(const char *symbol, unsigned long address)
>>>>    	return true;
>>>>    }
>>>>    
>>>> +/*
>>>> + * NOTE:
>>>> + * '.gnu.linkonce.this_module' section of kernel module elf directly
>>>> + * maps to 'struct module' from linux/module.h. This section contains
>>>> + * actual module name which will be used by kernel after loading it.
>>>> + * But, we cannot use 'struct module' here since linux/module.h is not
>>>> + * exposed to user-space. Offset of 'name' has remained same from long
>>>> + * time, so hardcoding it here.
>>>> + */
>>>> +#ifdef __LP64__
>>>> +#define MOD_NAME_OFFSET 24
>>>> +#else
>>>> +#define MOD_NAME_OFFSET 12
>>>> +#endif
>>>> +
>>>> +/*
>>>> + * @module can be module name of module file path. In case of path,
>>>> + * inspect elf and find out what is actual module name.
>>>> + * Caller has to free mod_name after using it.
>>>> + */
>>>> +char *find_module_name(const char *module)
>>> Could you make this function static, since there is no caller outside
>>> this file?
>> Yes. no caller outside of this file. But,
>>
>> In this patch, function find_module_name is defined outside of
>> #ifdef HAVE_DWARF_SUPPORT while it's being called from inside of
>> #ifdef HAVE_DWARF_SUPPORT.
>>
>> If I make it static and if there is no dwarf support, there will be
>> compilation
>> error about function defined but not used.
>>
>> And in second patch("perf probe: Fix module probe issue if no dwarf
>> support"),
>> I'm calling it from outside of #ifdef HAVE_DWARF_SUPPORT.
>>
>> So I have two options:
>> 1. merge both the patches and make definition as static
>> 2. make function static in second patch
>>
>> I've chose second approach and sent v2. But please let me know if there is
>> better way to do it.
> Ah, I see.
> In that case, you can swap the patch in the series and move find_module_name
> in the other patch ;)

Thanks :)  Sent patch with changes. Please review it.

Regards,
Ravi

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2016-04-26 14:30 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-25 10:38 [RFC] perf probe: Fix offline module name missmatch issue Ravi Bangoria
2016-04-25 10:38 ` [RFC] perf probe: Fix module probe issue if no dwarf support Ravi Bangoria
2016-04-25 21:29   ` Masami Hiramatsu
2016-04-26  9:04     ` Ravi Bangoria
2016-04-25 21:24 ` [RFC] perf probe: Fix offline module name missmatch issue Masami Hiramatsu
2016-04-26  2:19   ` Masami Hiramatsu
2016-04-26  9:00     ` Ravi Bangoria
2016-04-26  8:56   ` Ravi Bangoria
2016-04-26  9:15     ` Wangnan (F)
2016-04-26 10:45       ` Ravi Bangoria
2016-04-26 10:46     ` Masami Hiramatsu
2016-04-26 14:29       ` Ravi Bangoria

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.