[PATCH 1/2] perf report fix module symbol adjustment for s390x

[PATCH 1/2] perf report fix module symbol adjustment for s390x

[Thomas Richter]

The perf report tool does not display the addresses of kernel
module symbols correctly.

For example symbol qeth_send_ipa_cmd in kernel module qeth.ko
has this relative address for function qeth_send_ipa_cmd()
[root@s8360047 linux]# nm -g drivers/s390/net/qeth.ko | fgrep send_ipa_cmd
0000000000013088 T qeth_send_ipa_cmd

The module is loaded at address
[root@s8360047 linux]# cat /sys/module/qeth/sections/.text
0x000003ff80296d20
[root@s8360047 linux]#

This should result in a start address of
0x13088 + 0x3ff80296d20 = 0x3ff802a9da8

Using crash to verify the address on a live system:
[root@s8360046 linux]# crash vmlinux

crash 7.1.9++
Copyright (C) 2002-2016  Red Hat, Inc.
Copyright (C) 2004, 2005, 2006, 2010  IBM Corporation

[...]

crash> mod -s qeth drivers/s390/net/qeth.ko
     MODULE       NAME        SIZE  OBJECT FILE
     3ff8028d700  qeth      151552  drivers/s390/net/qeth.ko
crash> sym qeth_send_ipa_cmd
3ff802a9da8 (T) qeth_send_ipa_cmd [qeth] /root/linux/drivers/s390/net/qeth_core_main.c: 2944
crash>

Now perf report displays the address of symbol qeth_send_ipa_cmd:
symbol__new: qeth_send_ipa_cmd 0x130f0-0x132ce

There is a difference of 0x68 between the entry in the
symbol table (see nm command above) and perf. The difference is from
the offset the .text segment of qeth.ko:

[root@s8360047 perf]# readelf -a drivers/s390/net/qeth.ko
Section Headers:
  [Nr] Name              Type             Address           Offset
       Size              EntSize          Flags  Link  Info  Align
  [ 0]                   NULL             0000000000000000  00000000
       0000000000000000  0000000000000000           0     0     0
  [ 1] .note.gnu.build-i NOTE             0000000000000000  00000040
       0000000000000024  0000000000000000   A       0     0     4
  [ 2] .text             PROGBITS         0000000000000000  00000068
       000000000001c8a0  0000000000000000  AX       0     0     8

As seen the .text segment has an offset of 0x68 with start address 0x0.
Therefore 0x68 is added to the address of qeth_send_ipa_cmd and thus
0x13088 + 0x68 = 0x130f0 is displayed.

This is wrong, perf report needs to display the start address of
symbol qeth_send_ipa_cmd at 0x13088 + qeth.ko.text section start address.

The qeth.ko module .text start address is available in the qeth.ko
DSO map. Just identify the kernel module symbols and correct the
addresses.

With the fix I see this correct address for symbol:
symbol__new: qeth_send_ipa_cmd 0x3ff802a9da8-0x3ff802a9f86

I am not sure if this patch uses the perfect approach.
Hints and sugguests welcome.

Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
---
 tools/perf/arch/s390/util/sym-handling.c | 7 +++++++
 tools/perf/util/symbol-elf.c             | 8 +++++++-
 tools/perf/util/symbol.h                 | 3 +++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
index b6cd056..79bf238 100644
--- a/tools/perf/arch/s390/util/sym-handling.c
+++ b/tools/perf/arch/s390/util/sym-handling.c
@@ -19,4 +19,11 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
 	return ehdr.e_type == ET_REL || ehdr.e_type == ET_DYN;
 }
 
+u64 arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr __maybe_unused,
+			  struct map *map)
+{
+	if (map->type == MAP__FUNCTION)
+		sym->st_value += map->start;
+	return sym->st_value;
+}
 #endif
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 502505c..d3521d4 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
 void __weak arch__sym_update(struct symbol *s __maybe_unused,
 		GElf_Sym *sym __maybe_unused) { }
 
+u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
+				 struct map *map __maybe_unused)
+{
+	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
+}
+
 int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 		  struct symsrc *runtime_ss, int kmodule)
 {
@@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
 
 			/* Adjust symbol to map to file offset */
 			if (adjust_kernel_syms)
-				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
+				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
 
 			if (strcmp(section_name,
 				   (curr_dso->short_name +
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index 41ebba9..beb8737 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
 #ifdef HAVE_LIBELF_SUPPORT
 bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
 void arch__sym_update(struct symbol *s, GElf_Sym *sym);
+u64 arch__sym_set_address(GElf_Sym *sym,
+			  GElf_Shdr *shdr __maybe_unused,
+			  struct map *map __maybe_unused);
 #endif
 
 #define SYMBOL_A 0
-- 
2.9.3

[PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Thomas Richter]

During work on perf report for s390 I ran into the following issue:

0 0x318 [0x78]: PERF_RECORD_MMAP -1/0:
	[0x3ff804d6990(0xfffffc007fb2966f) @ 0]:
	x /lib/modules/4.12.0perf1+/kernel/drivers/s390/net/qeth_l2.ko

This is a PERF_RECORD_MMAP entry of the perf.data file with an invalid
module size for qeth_l2.ko (the s390 ethernet device driver).
Even a mainframe does not have 0xfffffc007fb2966f bytes of main memory.

It turned out that this wrong size is created by the perf record command.
What happens is this function call sequence from __cmd_record():

perf_session__new():
perf_session__create_kernel_maps():
machine__create_kernel_maps():
machine__create_modules():   Creates map for all loaded kernel modules.
modules__parse():   Reads /proc/modules and extracts module name and
		    load address (1st and last column)
machine__create_module():   Called for every module found in /proc/modules.
                    Creates a new map for every module found and enters
	            module name and start address into the map. Since the
		    module end address is unknown it is set to zero.

This ends up with a kernel module map list sorted by module start addresses.
All module end addresses are zero.

Last machine__create_kernel_maps() calls function map_groups__fixup_end().
This function iterates through the maps and assigns each map entry's
end address the successor map entry start address. The last entry of the
map group has no successor, so ~0 is used as end to consume the remaining
memory.

Later __cmd_record calls function record__synthesize() which in turn calls
perf_event__synthesize_kernel_mmap() and perf_event__synthesize_modules()
to create PERF_REPORT_MMAP entries into the perf.data file.

On s390 this results in the last module qeth_l2.ko
(which has highest start address, see module table:
	[root@s8360047 perf]# cat /proc/modules
	qeth_l2 86016 1 - Live 0x000003ff804d6000
	qeth 266240 1 qeth_l2, Live 0x000003ff80296000
	ccwgroup 24576 1 qeth, Live 0x000003ff80218000
	vmur 36864 0 - Live 0x000003ff80182000
	qdio 143360 2 qeth_l2,qeth, Live 0x000003ff80002000
	[root@s8360047 perf]# )
to be the last entry and its map has an end address of ~0.

When the PERF_RECORD_MMAP entry is created for kernel module qeth_l2.ko
its start address and length is written. The length is calculated in line:
    event->mmap.len   = pos->end - pos->start;
and results in 0xffffffffffffffff - 0x3ff804d6990(*) = 0xfffffc007fb2966f

(*) On s390 the module start address is actually determined by a __weak function
named arch__fix_module_text_start() in machine__create_module().

I think this improvable. We can use the module size (2nd column of /proc/modules)
to get each loaded kernel module size and calculate its end address.
Only for map entries which do not have a valid end address (end is still zero)
we can use the heuristic we have now, that is use successor start address or ~0.

Any other ideas?
Did this issue never showed up before?

Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
---
 tools/perf/util/machine.c    |  4 +++-
 tools/perf/util/symbol-elf.c |  2 +-
 tools/perf/util/symbol.c     | 21 ++++++++++++++-------
 tools/perf/util/symbol.h     |  2 +-
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index 2e9eb6a..eba28ca 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -20,6 +20,7 @@
 #include "unwind.h"
 #include "linux/hash.h"
 #include "asm/bug.h"
+#include <sys/user.h>
 
 #include "sane_ctype.h"
 #include <symbol/kallsyms.h>
@@ -1137,7 +1138,7 @@ int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
 	return 0;
 }
 
-static int machine__create_module(void *arg, const char *name, u64 start)
+static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
 {
 	struct machine *machine = arg;
 	struct map *map;
@@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
 	map = machine__findnew_module_map(machine, start, name);
 	if (map == NULL)
 		return -1;
+	map->end = roundup(start + size, PAGE_SIZE);
 
 	dso__kernel_module_get_build_id(map->dso, machine->root_dir);
 
diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index d3521d4..94b03ad 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -1448,7 +1448,7 @@ static int kcore_copy__parse_kallsyms(struct kcore_copy_info *kci,
 
 static int kcore_copy__process_modules(void *arg,
 				       const char *name __maybe_unused,
-				       u64 start)
+				       u64 start, u64 size __maybe_unused)
 {
 	struct kcore_copy_info *kci = arg;
 
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index e7a98db..ab4fcf2 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -231,7 +231,8 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
 		goto out_unlock;
 
 	for (next = map__next(curr); next; next = map__next(curr)) {
-		curr->end = next->start;
+		if (!curr->end)
+			curr->end = next->start;
 		curr = next;
 	}
 
@@ -239,7 +240,8 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
 	 * We still haven't the actual symbols, so guess the
 	 * last map final address.
 	 */
-	curr->end = ~0ULL;
+	if (!curr->end)
+		curr->end = ~0ULL;
 
 out_unlock:
 	pthread_rwlock_unlock(&maps->lock);
@@ -550,7 +552,7 @@ void dso__sort_by_name(struct dso *dso, enum map_type type)
 
 int modules__parse(const char *filename, void *arg,
 		   int (*process_module)(void *arg, const char *name,
-					 u64 start))
+					 u64 start, u64 size))
 {
 	char *line = NULL;
 	size_t n;
@@ -563,8 +565,8 @@ int modules__parse(const char *filename, void *arg,
 
 	while (1) {
 		char name[PATH_MAX];
-		u64 start;
-		char *sep;
+		u64 start, size;
+		char *sep, *endptr;
 		ssize_t line_len;
 
 		line_len = getline(&line, &n, file);
@@ -596,7 +598,11 @@ int modules__parse(const char *filename, void *arg,
 
 		scnprintf(name, sizeof(name), "[%s]", line);
 
-		err = process_module(arg, name, start);
+		size = strtoul(sep + 1, &endptr, 0);
+		if (*endptr != ' ' && *endptr != '\t')
+			continue;
+
+		err = process_module(arg, name, start, size);
 		if (err)
 			break;
 	}
@@ -943,7 +949,8 @@ static struct module_info *find_module(const char *name,
 	return NULL;
 }
 
-static int __read_proc_modules(void *arg, const char *name, u64 start)
+static int __read_proc_modules(void *arg, const char *name, u64 start,
+			       u64 size __maybe_unused)
 {
 	struct rb_root *modules = arg;
 	struct module_info *mi;
diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
index beb8737..1da5ed4 100644
--- a/tools/perf/util/symbol.h
+++ b/tools/perf/util/symbol.h
@@ -273,7 +273,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size);
 int sysfs__read_build_id(const char *filename, void *bf, size_t size);
 int modules__parse(const char *filename, void *arg,
 		   int (*process_module)(void *arg, const char *name,
-					 u64 start));
+					 u64 start, u64 size));
 int filename__read_debuglink(const char *filename, char *debuglink,
 			     size_t size);
 
-- 
2.9.3

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Arnaldo Carvalho de Melo]

Em Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter escreveu:
> During work on perf report for s390 I ran into the following issue:
> 
> 0 0x318 [0x78]: PERF_RECORD_MMAP -1/0:
> 	[0x3ff804d6990(0xfffffc007fb2966f) @ 0]:
> 	x /lib/modules/4.12.0perf1+/kernel/drivers/s390/net/qeth_l2.ko
> 
> This is a PERF_RECORD_MMAP entry of the perf.data file with an invalid
> module size for qeth_l2.ko (the s390 ethernet device driver).
> Even a mainframe does not have 0xfffffc007fb2966f bytes of main memory.
> 
> It turned out that this wrong size is created by the perf record command.
> What happens is this function call sequence from __cmd_record():
> 
> perf_session__new():
> perf_session__create_kernel_maps():
> machine__create_kernel_maps():
> machine__create_modules():   Creates map for all loaded kernel modules.
> modules__parse():   Reads /proc/modules and extracts module name and
> 		    load address (1st and last column)
> machine__create_module():   Called for every module found in /proc/modules.
>                     Creates a new map for every module found and enters
> 	            module name and start address into the map. Since the
> 		    module end address is unknown it is set to zero.
> 
> This ends up with a kernel module map list sorted by module start addresses.
> All module end addresses are zero.
> 
> Last machine__create_kernel_maps() calls function map_groups__fixup_end().
> This function iterates through the maps and assigns each map entry's
> end address the successor map entry start address. The last entry of the
> map group has no successor, so ~0 is used as end to consume the remaining
> memory.
> 
> Later __cmd_record calls function record__synthesize() which in turn calls
> perf_event__synthesize_kernel_mmap() and perf_event__synthesize_modules()
> to create PERF_REPORT_MMAP entries into the perf.data file.
> 
> On s390 this results in the last module qeth_l2.ko
> (which has highest start address, see module table:
> 	[root@s8360047 perf]# cat /proc/modules
> 	qeth_l2 86016 1 - Live 0x000003ff804d6000
> 	qeth 266240 1 qeth_l2, Live 0x000003ff80296000
> 	ccwgroup 24576 1 qeth, Live 0x000003ff80218000
> 	vmur 36864 0 - Live 0x000003ff80182000
> 	qdio 143360 2 qeth_l2,qeth, Live 0x000003ff80002000
> 	[root@s8360047 perf]# )
> to be the last entry and its map has an end address of ~0.
> 
> When the PERF_RECORD_MMAP entry is created for kernel module qeth_l2.ko
> its start address and length is written. The length is calculated in line:
>     event->mmap.len   = pos->end - pos->start;
> and results in 0xffffffffffffffff - 0x3ff804d6990(*) = 0xfffffc007fb2966f
> 
> (*) On s390 the module start address is actually determined by a __weak function
> named arch__fix_module_text_start() in machine__create_module().
> 
> I think this improvable. We can use the module size (2nd column of /proc/modules)

Nice analysis, I don't recall why we ended up not using that 2nd column
of /proc/modules... :-\o

Humm, probably the modules maps were created from the entries in
/proc/kallsyms, where we don't have the size, then, just like we do
w2hen parsing symbols from /proc/kallsyms, we set the end by looking at
the next entry...

> to get each loaded kernel module size and calculate its end address.
> Only for map entries which do not have a valid end address (end is still zero)
> we can use the heuristic we have now, that is use successor start address or ~0.
> 
> Any other ideas?
> Did this issue never showed up before?
> 
> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> ---
>  tools/perf/util/machine.c    |  4 +++-
>  tools/perf/util/symbol-elf.c |  2 +-
>  tools/perf/util/symbol.c     | 21 ++++++++++++++-------
>  tools/perf/util/symbol.h     |  2 +-
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 2e9eb6a..eba28ca 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -20,6 +20,7 @@
>  #include "unwind.h"
>  #include "linux/hash.h"
>  #include "asm/bug.h"
> +#include <sys/user.h>
>  
>  #include "sane_ctype.h"
>  #include <symbol/kallsyms.h>
> @@ -1137,7 +1138,7 @@ int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
>  	return 0;
>  }
>  
> -static int machine__create_module(void *arg, const char *name, u64 start)
> +static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
>  {
>  	struct machine *machine = arg;
>  	struct map *map;
> @@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
>  	map = machine__findnew_module_map(machine, start, name);
>  	if (map == NULL)
>  		return -1;
> +	map->end = roundup(start + size, PAGE_SIZE);
>  
>  	dso__kernel_module_get_build_id(map->dso, machine->root_dir);
>  
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index d3521d4..94b03ad 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1448,7 +1448,7 @@ static int kcore_copy__parse_kallsyms(struct kcore_copy_info *kci,
>  
>  static int kcore_copy__process_modules(void *arg,
>  				       const char *name __maybe_unused,
> -				       u64 start)
> +				       u64 start, u64 size __maybe_unused)
>  {
>  	struct kcore_copy_info *kci = arg;
>  
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e7a98db..ab4fcf2 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -231,7 +231,8 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
>  		goto out_unlock;
>  
>  	for (next = map__next(curr); next; next = map__next(curr)) {
> -		curr->end = next->start;
> +		if (!curr->end)
> +			curr->end = next->start;
>  		curr = next;
>  	}
>  
> @@ -239,7 +240,8 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
>  	 * We still haven't the actual symbols, so guess the
>  	 * last map final address.
>  	 */
> -	curr->end = ~0ULL;
> +	if (!curr->end)
> +		curr->end = ~0ULL;
>  
>  out_unlock:
>  	pthread_rwlock_unlock(&maps->lock);
> @@ -550,7 +552,7 @@ void dso__sort_by_name(struct dso *dso, enum map_type type)
>  
>  int modules__parse(const char *filename, void *arg,
>  		   int (*process_module)(void *arg, const char *name,
> -					 u64 start))
> +					 u64 start, u64 size))
>  {
>  	char *line = NULL;
>  	size_t n;
> @@ -563,8 +565,8 @@ int modules__parse(const char *filename, void *arg,
>  
>  	while (1) {
>  		char name[PATH_MAX];
> -		u64 start;
> -		char *sep;
> +		u64 start, size;
> +		char *sep, *endptr;
>  		ssize_t line_len;
>  
>  		line_len = getline(&line, &n, file);
> @@ -596,7 +598,11 @@ int modules__parse(const char *filename, void *arg,
>  
>  		scnprintf(name, sizeof(name), "[%s]", line);
>  
> -		err = process_module(arg, name, start);
> +		size = strtoul(sep + 1, &endptr, 0);
> +		if (*endptr != ' ' && *endptr != '\t')
> +			continue;
> +
> +		err = process_module(arg, name, start, size);
>  		if (err)
>  			break;
>  	}
> @@ -943,7 +949,8 @@ static struct module_info *find_module(const char *name,
>  	return NULL;
>  }
>  
> -static int __read_proc_modules(void *arg, const char *name, u64 start)
> +static int __read_proc_modules(void *arg, const char *name, u64 start,
> +			       u64 size __maybe_unused)
>  {
>  	struct rb_root *modules = arg;
>  	struct module_info *mi;
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index beb8737..1da5ed4 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -273,7 +273,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size);
>  int sysfs__read_build_id(const char *filename, void *bf, size_t size);
>  int modules__parse(const char *filename, void *arg,
>  		   int (*process_module)(void *arg, const char *name,
> -					 u64 start));
> +					 u64 start, u64 size));
>  int filename__read_debuglink(const char *filename, char *debuglink,
>  			     size_t size);
>  
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Namhyung Kim]

Hello,

On Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter wrote:
> The perf report tool does not display the addresses of kernel
> module symbols correctly.
> 
> For example symbol qeth_send_ipa_cmd in kernel module qeth.ko
> has this relative address for function qeth_send_ipa_cmd()
> [root@s8360047 linux]# nm -g drivers/s390/net/qeth.ko | fgrep send_ipa_cmd
> 0000000000013088 T qeth_send_ipa_cmd
> 
> The module is loaded at address
> [root@s8360047 linux]# cat /sys/module/qeth/sections/.text
> 0x000003ff80296d20
> [root@s8360047 linux]#
> 
> This should result in a start address of
> 0x13088 + 0x3ff80296d20 = 0x3ff802a9da8
> 
> Using crash to verify the address on a live system:
> [root@s8360046 linux]# crash vmlinux
> 
> crash 7.1.9++
> Copyright (C) 2002-2016  Red Hat, Inc.
> Copyright (C) 2004, 2005, 2006, 2010  IBM Corporation
> 
> [...]
> 
> crash> mod -s qeth drivers/s390/net/qeth.ko
>      MODULE       NAME        SIZE  OBJECT FILE
>      3ff8028d700  qeth      151552  drivers/s390/net/qeth.ko
> crash> sym qeth_send_ipa_cmd
> 3ff802a9da8 (T) qeth_send_ipa_cmd [qeth] /root/linux/drivers/s390/net/qeth_core_main.c: 2944
> crash>
> 
> Now perf report displays the address of symbol qeth_send_ipa_cmd:
> symbol__new: qeth_send_ipa_cmd 0x130f0-0x132ce
> 
> There is a difference of 0x68 between the entry in the
> symbol table (see nm command above) and perf. The difference is from
> the offset the .text segment of qeth.ko:
> 
> [root@s8360047 perf]# readelf -a drivers/s390/net/qeth.ko
> Section Headers:
>   [Nr] Name              Type             Address           Offset
>        Size              EntSize          Flags  Link  Info  Align
>   [ 0]                   NULL             0000000000000000  00000000
>        0000000000000000  0000000000000000           0     0     0
>   [ 1] .note.gnu.build-i NOTE             0000000000000000  00000040
>        0000000000000024  0000000000000000   A       0     0     4
>   [ 2] .text             PROGBITS         0000000000000000  00000068
>        000000000001c8a0  0000000000000000  AX       0     0     8
> 
> As seen the .text segment has an offset of 0x68 with start address 0x0.
> Therefore 0x68 is added to the address of qeth_send_ipa_cmd and thus
> 0x13088 + 0x68 = 0x130f0 is displayed.
> 
> This is wrong, perf report needs to display the start address of
> symbol qeth_send_ipa_cmd at 0x13088 + qeth.ko.text section start address.
> 
> The qeth.ko module .text start address is available in the qeth.ko
> DSO map. Just identify the kernel module symbols and correct the
> addresses.
> 
> With the fix I see this correct address for symbol:
> symbol__new: qeth_send_ipa_cmd 0x3ff802a9da8-0x3ff802a9f86
> 
> I am not sure if this patch uses the perfect approach.
> Hints and sugguests welcome.

I saw this problem on x86_64 too and I don't know why we add sh_offset
to symbols.  To me, current symbol code is somewhat hacky and needs
some love.

Thanks,
Namhyung


> 
> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> ---
>  tools/perf/arch/s390/util/sym-handling.c | 7 +++++++
>  tools/perf/util/symbol-elf.c             | 8 +++++++-
>  tools/perf/util/symbol.h                 | 3 +++
>  3 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
> index b6cd056..79bf238 100644
> --- a/tools/perf/arch/s390/util/sym-handling.c
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -19,4 +19,11 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
>  	return ehdr.e_type == ET_REL || ehdr.e_type == ET_DYN;
>  }
>  
> +u64 arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr __maybe_unused,
> +			  struct map *map)
> +{
> +	if (map->type == MAP__FUNCTION)
> +		sym->st_value += map->start;
> +	return sym->st_value;
> +}
>  #endif
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index 502505c..d3521d4 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
>  		GElf_Sym *sym __maybe_unused) { }
>  
> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
> +				 struct map *map __maybe_unused)
> +{
> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
> +}
> +
>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>  		  struct symsrc *runtime_ss, int kmodule)
>  {
> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>  
>  			/* Adjust symbol to map to file offset */
>  			if (adjust_kernel_syms)
> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
>  
>  			if (strcmp(section_name,
>  				   (curr_dso->short_name +
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 41ebba9..beb8737 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
>  #ifdef HAVE_LIBELF_SUPPORT
>  bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
>  void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> +u64 arch__sym_set_address(GElf_Sym *sym,
> +			  GElf_Shdr *shdr __maybe_unused,
> +			  struct map *map __maybe_unused);
>  #endif
>  
>  #define SYMBOL_A 0
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Namhyung Kim]

On Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter wrote:
> During work on perf report for s390 I ran into the following issue:
> 
> 0 0x318 [0x78]: PERF_RECORD_MMAP -1/0:
> 	[0x3ff804d6990(0xfffffc007fb2966f) @ 0]:
> 	x /lib/modules/4.12.0perf1+/kernel/drivers/s390/net/qeth_l2.ko
> 
> This is a PERF_RECORD_MMAP entry of the perf.data file with an invalid
> module size for qeth_l2.ko (the s390 ethernet device driver).
> Even a mainframe does not have 0xfffffc007fb2966f bytes of main memory.
> 
> It turned out that this wrong size is created by the perf record command.
> What happens is this function call sequence from __cmd_record():
> 
> perf_session__new():
> perf_session__create_kernel_maps():
> machine__create_kernel_maps():
> machine__create_modules():   Creates map for all loaded kernel modules.
> modules__parse():   Reads /proc/modules and extracts module name and
> 		    load address (1st and last column)
> machine__create_module():   Called for every module found in /proc/modules.
>                     Creates a new map for every module found and enters
> 	            module name and start address into the map. Since the
> 		    module end address is unknown it is set to zero.
> 
> This ends up with a kernel module map list sorted by module start addresses.
> All module end addresses are zero.
> 
> Last machine__create_kernel_maps() calls function map_groups__fixup_end().
> This function iterates through the maps and assigns each map entry's
> end address the successor map entry start address. The last entry of the
> map group has no successor, so ~0 is used as end to consume the remaining
> memory.
> 
> Later __cmd_record calls function record__synthesize() which in turn calls
> perf_event__synthesize_kernel_mmap() and perf_event__synthesize_modules()
> to create PERF_REPORT_MMAP entries into the perf.data file.
> 
> On s390 this results in the last module qeth_l2.ko
> (which has highest start address, see module table:
> 	[root@s8360047 perf]# cat /proc/modules
> 	qeth_l2 86016 1 - Live 0x000003ff804d6000
> 	qeth 266240 1 qeth_l2, Live 0x000003ff80296000
> 	ccwgroup 24576 1 qeth, Live 0x000003ff80218000
> 	vmur 36864 0 - Live 0x000003ff80182000
> 	qdio 143360 2 qeth_l2,qeth, Live 0x000003ff80002000
> 	[root@s8360047 perf]# )
> to be the last entry and its map has an end address of ~0.
> 
> When the PERF_RECORD_MMAP entry is created for kernel module qeth_l2.ko
> its start address and length is written. The length is calculated in line:
>     event->mmap.len   = pos->end - pos->start;
> and results in 0xffffffffffffffff - 0x3ff804d6990(*) = 0xfffffc007fb2966f
> 
> (*) On s390 the module start address is actually determined by a __weak function
> named arch__fix_module_text_start() in machine__create_module().
> 
> I think this improvable. We can use the module size (2nd column of /proc/modules)
> to get each loaded kernel module size and calculate its end address.
> Only for map entries which do not have a valid end address (end is still zero)
> we can use the heuristic we have now, that is use successor start address or ~0.
> 
> Any other ideas?
> Did this issue never showed up before?

Did you hit a real problem with this?  I think it doesn't affect
symbol resolution anyway.  I saw a related problem before though..

  https://lkml.org/lkml/2017/6/23/63

In this case a module was unloaded after perf record, and loaded again
(but on a slightly different address) before perf report.  The record
data has old address but perf report tried to use current address so
it could not find symbols from the module.  I need to update the
patchset..

Anyway I'm ok with this patch.

Thanks,
Namhyung


> 
> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> ---
>  tools/perf/util/machine.c    |  4 +++-
>  tools/perf/util/symbol-elf.c |  2 +-
>  tools/perf/util/symbol.c     | 21 ++++++++++++++-------
>  tools/perf/util/symbol.h     |  2 +-
>  4 files changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 2e9eb6a..eba28ca 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -20,6 +20,7 @@
>  #include "unwind.h"
>  #include "linux/hash.h"
>  #include "asm/bug.h"
> +#include <sys/user.h>
>  
>  #include "sane_ctype.h"
>  #include <symbol/kallsyms.h>
> @@ -1137,7 +1138,7 @@ int __weak arch__fix_module_text_start(u64 *start __maybe_unused,
>  	return 0;
>  }
>  
> -static int machine__create_module(void *arg, const char *name, u64 start)
> +static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
>  {
>  	struct machine *machine = arg;
>  	struct map *map;
> @@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
>  	map = machine__findnew_module_map(machine, start, name);
>  	if (map == NULL)
>  		return -1;
> +	map->end = roundup(start + size, PAGE_SIZE);
>  
>  	dso__kernel_module_get_build_id(map->dso, machine->root_dir);
>  
> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> index d3521d4..94b03ad 100644
> --- a/tools/perf/util/symbol-elf.c
> +++ b/tools/perf/util/symbol-elf.c
> @@ -1448,7 +1448,7 @@ static int kcore_copy__parse_kallsyms(struct kcore_copy_info *kci,
>  
>  static int kcore_copy__process_modules(void *arg,
>  				       const char *name __maybe_unused,
> -				       u64 start)
> +				       u64 start, u64 size __maybe_unused)
>  {
>  	struct kcore_copy_info *kci = arg;
>  
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e7a98db..ab4fcf2 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -231,7 +231,8 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
>  		goto out_unlock;
>  
>  	for (next = map__next(curr); next; next = map__next(curr)) {
> -		curr->end = next->start;
> +		if (!curr->end)
> +			curr->end = next->start;
>  		curr = next;
>  	}
>  
> @@ -239,7 +240,8 @@ void __map_groups__fixup_end(struct map_groups *mg, enum map_type type)
>  	 * We still haven't the actual symbols, so guess the
>  	 * last map final address.
>  	 */
> -	curr->end = ~0ULL;
> +	if (!curr->end)
> +		curr->end = ~0ULL;
>  
>  out_unlock:
>  	pthread_rwlock_unlock(&maps->lock);
> @@ -550,7 +552,7 @@ void dso__sort_by_name(struct dso *dso, enum map_type type)
>  
>  int modules__parse(const char *filename, void *arg,
>  		   int (*process_module)(void *arg, const char *name,
> -					 u64 start))
> +					 u64 start, u64 size))
>  {
>  	char *line = NULL;
>  	size_t n;
> @@ -563,8 +565,8 @@ int modules__parse(const char *filename, void *arg,
>  
>  	while (1) {
>  		char name[PATH_MAX];
> -		u64 start;
> -		char *sep;
> +		u64 start, size;
> +		char *sep, *endptr;
>  		ssize_t line_len;
>  
>  		line_len = getline(&line, &n, file);
> @@ -596,7 +598,11 @@ int modules__parse(const char *filename, void *arg,
>  
>  		scnprintf(name, sizeof(name), "[%s]", line);
>  
> -		err = process_module(arg, name, start);
> +		size = strtoul(sep + 1, &endptr, 0);
> +		if (*endptr != ' ' && *endptr != '\t')
> +			continue;
> +
> +		err = process_module(arg, name, start, size);
>  		if (err)
>  			break;
>  	}
> @@ -943,7 +949,8 @@ static struct module_info *find_module(const char *name,
>  	return NULL;
>  }
>  
> -static int __read_proc_modules(void *arg, const char *name, u64 start)
> +static int __read_proc_modules(void *arg, const char *name, u64 start,
> +			       u64 size __maybe_unused)
>  {
>  	struct rb_root *modules = arg;
>  	struct module_info *mi;
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index beb8737..1da5ed4 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -273,7 +273,7 @@ int filename__read_build_id(const char *filename, void *bf, size_t size);
>  int sysfs__read_build_id(const char *filename, void *bf, size_t size);
>  int modules__parse(const char *filename, void *arg,
>  		   int (*process_module)(void *arg, const char *name,
> -					 u64 start));
> +					 u64 start, u64 size));
>  int filename__read_debuglink(const char *filename, char *debuglink,
>  			     size_t size);
>  
> -- 
> 2.9.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Thomas-Mich Richter]

On 07/25/2017 08:58 AM, Namhyung Kim wrote:
> On Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter wrote:
>> During work on perf report for s390 I ran into the following issue:


[...]

> 
> Did you hit a real problem with this?  I think it doesn't affect
> symbol resolution anyway.  I saw a related problem before though..

Sure, that is why I startet digging.... Here is the output of perf report
which triggered my investigation last week:


0.79%     0.00%  sshd            [qeth_l2]                   [k] 0xfffffc007fb2dbb2
            |
            ---0x44da
               skb_vlan_push
               iplstart

As said before qeth_l2.ko is the last kernel modules listed in /proc/modules.

>   https://lkml.org/lkml/2017/6/23/63
> 
> In this case a module was unloaded after perf record, and loaded again
> (but on a slightly different address) before perf report.  The record
> data has old address but perf report tried to use current address so
> it could not find symbols from the module.  I need to update the
> patchset..

Ok I have looked at this patch series but still do not understand every
detail of it. I have not seen it before, I do not
follow the linux-kernel mailing list. This amount of traffic kills me...
(or my thunderbird) and I end up deleting the incoming mails all the time.

However I do follow linux-perf-user mailing list. I found this sufficient
because right now I only work on the perf user tool and not the kernel part.

> 
> Anyway I'm ok with this patch.
> 
> Thanks,
> Namhyung
> 

[...]
-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Thomas-Mich Richter]

On 07/24/2017 08:20 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter escreveu:


[...]

> 
> Nice analysis, I don't recall why we ended up not using that 2nd column
> of /proc/modules... :-\o

Nor do I :-)

> 
> Humm, probably the modules maps were created from the entries in
> /proc/kallsyms, where we don't have the size, then, just like we do
> w2hen parsing symbols from /proc/kallsyms, we set the end by looking at
> the next entry...
> 

The maps where created for the kernel by reading 
dso: [kernel.vmlinux] (/lib/modules/4.12.0perf1+/build/vmlinux, Functions, 
   loaded, b31f6c58ffca1ca9ecc4a3451b7bb1b1a9a6be0e)

and for the module by reading 
dso: [qeth_l2] (/lib/modules/4.12.0perf1+/kernel/drivers/s390/net/qeth_l2.ko, Functions, 
    loaded, ed1c8278b4b1cb67262e83cd535f155415197610)

In fact I printed out the possible paths to search for the kernel:
vmlinux_path__add 0 vmlinux
vmlinux_path__add 1 /boot/vmlinux
vmlinux_path__init kernel_version:4.12.0perf1+
vmlinux_path__add 2 /boot/vmlinux-4.12.0perf1+
vmlinux_path__add 3 /usr/lib/debug/boot/vmlinux-4.12.0perf1+
vmlinux_path__add 4 /lib/modules/4.12.0perf1+/build/vmlinux
vmlinux_path__add 5 /usr/lib/debug/lib/modules/4.12.0perf1+/vmlinux
vmlinux_path__add 6 /usr/lib/debug/boot/vmlinux-4.12.0perf1+.debug

So there is no kallsyms involved.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Thomas-Mich Richter]

On 07/25/2017 08:14 AM, Namhyung Kim wrote:
> Hello,
> 
> On Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter wrote:
>> The perf report tool does not display the addresses of kernel
>> module symbols correctly.
>>
>> For example symbol qeth_send_ipa_cmd in kernel module qeth.ko
>> has this relative address for function qeth_send_ipa_cmd()
>> [root@s8360047 linux]# nm -g drivers/s390/net/qeth.ko | fgrep send_ipa_cmd
>> 0000000000013088 T qeth_send_ipa_cmd
>>
>> The module is loaded at address
>> [root@s8360047 linux]# cat /sys/module/qeth/sections/.text
>> 0x000003ff80296d20
>> [root@s8360047 linux]#
>>
>> This should result in a start address of
>> 0x13088 + 0x3ff80296d20 = 0x3ff802a9da8
>>
>> Using crash to verify the address on a live system:
>> [root@s8360046 linux]# crash vmlinux
>>
>> crash 7.1.9++
>> Copyright (C) 2002-2016  Red Hat, Inc.
>> Copyright (C) 2004, 2005, 2006, 2010  IBM Corporation
>>
>> [...]
>>
>> crash> mod -s qeth drivers/s390/net/qeth.ko
>>      MODULE       NAME        SIZE  OBJECT FILE
>>      3ff8028d700  qeth      151552  drivers/s390/net/qeth.ko
>> crash> sym qeth_send_ipa_cmd
>> 3ff802a9da8 (T) qeth_send_ipa_cmd [qeth] /root/linux/drivers/s390/net/qeth_core_main.c: 2944
>> crash>
>>
>> Now perf report displays the address of symbol qeth_send_ipa_cmd:
>> symbol__new: qeth_send_ipa_cmd 0x130f0-0x132ce
>>
>> There is a difference of 0x68 between the entry in the
>> symbol table (see nm command above) and perf. The difference is from
>> the offset the .text segment of qeth.ko:
>>
>> [root@s8360047 perf]# readelf -a drivers/s390/net/qeth.ko
>> Section Headers:
>>   [Nr] Name              Type             Address           Offset
>>        Size              EntSize          Flags  Link  Info  Align
>>   [ 0]                   NULL             0000000000000000  00000000
>>        0000000000000000  0000000000000000           0     0     0
>>   [ 1] .note.gnu.build-i NOTE             0000000000000000  00000040
>>        0000000000000024  0000000000000000   A       0     0     4
>>   [ 2] .text             PROGBITS         0000000000000000  00000068
>>        000000000001c8a0  0000000000000000  AX       0     0     8
>>
>> As seen the .text segment has an offset of 0x68 with start address 0x0.
>> Therefore 0x68 is added to the address of qeth_send_ipa_cmd and thus
>> 0x13088 + 0x68 = 0x130f0 is displayed.
>>
>> This is wrong, perf report needs to display the start address of
>> symbol qeth_send_ipa_cmd at 0x13088 + qeth.ko.text section start address.
>>
>> The qeth.ko module .text start address is available in the qeth.ko
>> DSO map. Just identify the kernel module symbols and correct the
>> addresses.
>>
>> With the fix I see this correct address for symbol:
>> symbol__new: qeth_send_ipa_cmd 0x3ff802a9da8-0x3ff802a9f86
>>
>> I am not sure if this patch uses the perfect approach.
>> Hints and sugguests welcome.
> 
> I saw this problem on x86_64 too and I don't know why we add sh_offset
> to symbols.  To me, current symbol code is somewhat hacky and needs
> some love.
> 
> Thanks,
> Namhyung
> 

Okay, how do we continue with this issue?
The patch I submitted uses a __weak function of s390x and does not
address other platforms.

> 
>>
>> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
>> ---
>>  tools/perf/arch/s390/util/sym-handling.c | 7 +++++++
>>  tools/perf/util/symbol-elf.c             | 8 +++++++-
>>  tools/perf/util/symbol.h                 | 3 +++
>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
>> index b6cd056..79bf238 100644
>> --- a/tools/perf/arch/s390/util/sym-handling.c
>> +++ b/tools/perf/arch/s390/util/sym-handling.c
>> @@ -19,4 +19,11 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
>>  	return ehdr.e_type == ET_REL || ehdr.e_type == ET_DYN;
>>  }
>>  
>> +u64 arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr __maybe_unused,
>> +			  struct map *map)
>> +{
>> +	if (map->type == MAP__FUNCTION)
>> +		sym->st_value += map->start;
>> +	return sym->st_value;
>> +}
>>  #endif
>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>> index 502505c..d3521d4 100644
>> --- a/tools/perf/util/symbol-elf.c
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
>>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
>>  		GElf_Sym *sym __maybe_unused) { }
>>  
>> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
>> +				 struct map *map __maybe_unused)
>> +{
>> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
>> +}
>> +
>>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>>  		  struct symsrc *runtime_ss, int kmodule)
>>  {
>> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>>  
>>  			/* Adjust symbol to map to file offset */
>>  			if (adjust_kernel_syms)
>> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
>> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
>>  
>>  			if (strcmp(section_name,
>>  				   (curr_dso->short_name +
>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>> index 41ebba9..beb8737 100644
>> --- a/tools/perf/util/symbol.h
>> +++ b/tools/perf/util/symbol.h
>> @@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
>>  #ifdef HAVE_LIBELF_SUPPORT
>>  bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
>>  void arch__sym_update(struct symbol *s, GElf_Sym *sym);
>> +u64 arch__sym_set_address(GElf_Sym *sym,
>> +			  GElf_Shdr *shdr __maybe_unused,
>> +			  struct map *map __maybe_unused);
>>  #endif
>>  
>>  #define SYMBOL_A 0
>> -- 
>> 2.9.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Namhyung Kim]

On Tue, Jul 25, 2017 at 10:49:24AM +0200, Thomas-Mich Richter wrote:
> On 07/25/2017 08:14 AM, Namhyung Kim wrote:
> > Hello,
> > 
> > On Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter wrote:
> >> The perf report tool does not display the addresses of kernel
> >> module symbols correctly.
> >>
> >> For example symbol qeth_send_ipa_cmd in kernel module qeth.ko
> >> has this relative address for function qeth_send_ipa_cmd()
> >> [root@s8360047 linux]# nm -g drivers/s390/net/qeth.ko | fgrep send_ipa_cmd
> >> 0000000000013088 T qeth_send_ipa_cmd
> >>
> >> The module is loaded at address
> >> [root@s8360047 linux]# cat /sys/module/qeth/sections/.text
> >> 0x000003ff80296d20
> >> [root@s8360047 linux]#
> >>
> >> This should result in a start address of
> >> 0x13088 + 0x3ff80296d20 = 0x3ff802a9da8
> >>
> >> Using crash to verify the address on a live system:
> >> [root@s8360046 linux]# crash vmlinux
> >>
> >> crash 7.1.9++
> >> Copyright (C) 2002-2016  Red Hat, Inc.
> >> Copyright (C) 2004, 2005, 2006, 2010  IBM Corporation
> >>
> >> [...]
> >>
> >> crash> mod -s qeth drivers/s390/net/qeth.ko
> >>      MODULE       NAME        SIZE  OBJECT FILE
> >>      3ff8028d700  qeth      151552  drivers/s390/net/qeth.ko
> >> crash> sym qeth_send_ipa_cmd
> >> 3ff802a9da8 (T) qeth_send_ipa_cmd [qeth] /root/linux/drivers/s390/net/qeth_core_main.c: 2944
> >> crash>
> >>
> >> Now perf report displays the address of symbol qeth_send_ipa_cmd:
> >> symbol__new: qeth_send_ipa_cmd 0x130f0-0x132ce
> >>
> >> There is a difference of 0x68 between the entry in the
> >> symbol table (see nm command above) and perf. The difference is from
> >> the offset the .text segment of qeth.ko:
> >>
> >> [root@s8360047 perf]# readelf -a drivers/s390/net/qeth.ko
> >> Section Headers:
> >>   [Nr] Name              Type             Address           Offset
> >>        Size              EntSize          Flags  Link  Info  Align
> >>   [ 0]                   NULL             0000000000000000  00000000
> >>        0000000000000000  0000000000000000           0     0     0
> >>   [ 1] .note.gnu.build-i NOTE             0000000000000000  00000040
> >>        0000000000000024  0000000000000000   A       0     0     4
> >>   [ 2] .text             PROGBITS         0000000000000000  00000068
> >>        000000000001c8a0  0000000000000000  AX       0     0     8
> >>
> >> As seen the .text segment has an offset of 0x68 with start address 0x0.
> >> Therefore 0x68 is added to the address of qeth_send_ipa_cmd and thus
> >> 0x13088 + 0x68 = 0x130f0 is displayed.
> >>
> >> This is wrong, perf report needs to display the start address of
> >> symbol qeth_send_ipa_cmd at 0x13088 + qeth.ko.text section start address.
> >>
> >> The qeth.ko module .text start address is available in the qeth.ko
> >> DSO map. Just identify the kernel module symbols and correct the
> >> addresses.
> >>
> >> With the fix I see this correct address for symbol:
> >> symbol__new: qeth_send_ipa_cmd 0x3ff802a9da8-0x3ff802a9f86
> >>
> >> I am not sure if this patch uses the perfect approach.
> >> Hints and sugguests welcome.
> > 
> > I saw this problem on x86_64 too and I don't know why we add sh_offset
> > to symbols.  To me, current symbol code is somewhat hacky and needs
> > some love.
> > 
> > Thanks,
> > Namhyung
> > 
> 
> Okay, how do we continue with this issue?
> The patch I submitted uses a __weak function of s390x and does not
> address other platforms.

Not sure.  The symbol code is complex and needs to consider many
cases.  I think we need to rethink the 'adjust_symbols' logic and
to check adding 'sh_offset' is valid.

Thanks,
Namhyung


> 
> > 
> >>
> >> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> >> ---
> >>  tools/perf/arch/s390/util/sym-handling.c | 7 +++++++
> >>  tools/perf/util/symbol-elf.c             | 8 +++++++-
> >>  tools/perf/util/symbol.h                 | 3 +++
> >>  3 files changed, 17 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
> >> index b6cd056..79bf238 100644
> >> --- a/tools/perf/arch/s390/util/sym-handling.c
> >> +++ b/tools/perf/arch/s390/util/sym-handling.c
> >> @@ -19,4 +19,11 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
> >>  	return ehdr.e_type == ET_REL || ehdr.e_type == ET_DYN;
> >>  }
> >>  
> >> +u64 arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr __maybe_unused,
> >> +			  struct map *map)
> >> +{
> >> +	if (map->type == MAP__FUNCTION)
> >> +		sym->st_value += map->start;
> >> +	return sym->st_value;
> >> +}
> >>  #endif
> >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> >> index 502505c..d3521d4 100644
> >> --- a/tools/perf/util/symbol-elf.c
> >> +++ b/tools/perf/util/symbol-elf.c
> >> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
> >>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
> >>  		GElf_Sym *sym __maybe_unused) { }
> >>  
> >> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
> >> +				 struct map *map __maybe_unused)
> >> +{
> >> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
> >> +}
> >> +
> >>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> >>  		  struct symsrc *runtime_ss, int kmodule)
> >>  {
> >> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> >>  
> >>  			/* Adjust symbol to map to file offset */
> >>  			if (adjust_kernel_syms)
> >> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> >> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
> >>  
> >>  			if (strcmp(section_name,
> >>  				   (curr_dso->short_name +
> >> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> >> index 41ebba9..beb8737 100644
> >> --- a/tools/perf/util/symbol.h
> >> +++ b/tools/perf/util/symbol.h
> >> @@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
> >>  #ifdef HAVE_LIBELF_SUPPORT
> >>  bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
> >>  void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> >> +u64 arch__sym_set_address(GElf_Sym *sym,
> >> +			  GElf_Shdr *shdr __maybe_unused,
> >> +			  struct map *map __maybe_unused);
> >>  #endif
> >>  
> >>  #define SYMBOL_A 0
> >> -- 
> >> 2.9.3
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > 
> 
> 
> -- 
> Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
> --
> Vorsitzende des Aufsichtsrats: Martina Koederitz 
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
> 

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Arnaldo Carvalho de Melo]

Em Tue, Jul 25, 2017 at 07:32:40PM +0900, Namhyung Kim escreveu:
> On Tue, Jul 25, 2017 at 10:49:24AM +0200, Thomas-Mich Richter wrote:
> > On 07/25/2017 08:14 AM, Namhyung Kim wrote:
> > > Hello,
> > > 
> > > On Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter wrote:
> > >> The perf report tool does not display the addresses of kernel
> > >> module symbols correctly.
> > >>
> > >> For example symbol qeth_send_ipa_cmd in kernel module qeth.ko
> > >> has this relative address for function qeth_send_ipa_cmd()
> > >> [root@s8360047 linux]# nm -g drivers/s390/net/qeth.ko | fgrep send_ipa_cmd
> > >> 0000000000013088 T qeth_send_ipa_cmd
> > >>
> > >> The module is loaded at address
> > >> [root@s8360047 linux]# cat /sys/module/qeth/sections/.text
> > >> 0x000003ff80296d20
> > >> [root@s8360047 linux]#
> > >>
> > >> This should result in a start address of
> > >> 0x13088 + 0x3ff80296d20 = 0x3ff802a9da8
> > >>
> > >> Using crash to verify the address on a live system:
> > >> [root@s8360046 linux]# crash vmlinux
> > >>
> > >> crash 7.1.9++
> > >> Copyright (C) 2002-2016  Red Hat, Inc.
> > >> Copyright (C) 2004, 2005, 2006, 2010  IBM Corporation
> > >>
> > >> [...]
> > >>
> > >> crash> mod -s qeth drivers/s390/net/qeth.ko
> > >>      MODULE       NAME        SIZE  OBJECT FILE
> > >>      3ff8028d700  qeth      151552  drivers/s390/net/qeth.ko
> > >> crash> sym qeth_send_ipa_cmd
> > >> 3ff802a9da8 (T) qeth_send_ipa_cmd [qeth] /root/linux/drivers/s390/net/qeth_core_main.c: 2944
> > >> crash>
> > >>
> > >> Now perf report displays the address of symbol qeth_send_ipa_cmd:
> > >> symbol__new: qeth_send_ipa_cmd 0x130f0-0x132ce
> > >>
> > >> There is a difference of 0x68 between the entry in the
> > >> symbol table (see nm command above) and perf. The difference is from
> > >> the offset the .text segment of qeth.ko:
> > >>
> > >> [root@s8360047 perf]# readelf -a drivers/s390/net/qeth.ko
> > >> Section Headers:
> > >>   [Nr] Name              Type             Address           Offset
> > >>        Size              EntSize          Flags  Link  Info  Align
> > >>   [ 0]                   NULL             0000000000000000  00000000
> > >>        0000000000000000  0000000000000000           0     0     0
> > >>   [ 1] .note.gnu.build-i NOTE             0000000000000000  00000040
> > >>        0000000000000024  0000000000000000   A       0     0     4
> > >>   [ 2] .text             PROGBITS         0000000000000000  00000068
> > >>        000000000001c8a0  0000000000000000  AX       0     0     8
> > >>
> > >> As seen the .text segment has an offset of 0x68 with start address 0x0.
> > >> Therefore 0x68 is added to the address of qeth_send_ipa_cmd and thus
> > >> 0x13088 + 0x68 = 0x130f0 is displayed.
> > >>
> > >> This is wrong, perf report needs to display the start address of
> > >> symbol qeth_send_ipa_cmd at 0x13088 + qeth.ko.text section start address.
> > >>
> > >> The qeth.ko module .text start address is available in the qeth.ko
> > >> DSO map. Just identify the kernel module symbols and correct the
> > >> addresses.
> > >>
> > >> With the fix I see this correct address for symbol:
> > >> symbol__new: qeth_send_ipa_cmd 0x3ff802a9da8-0x3ff802a9f86
> > >>
> > >> I am not sure if this patch uses the perfect approach.
> > >> Hints and sugguests welcome.
> > > 
> > > I saw this problem on x86_64 too and I don't know why we add sh_offset
> > > to symbols.  To me, current symbol code is somewhat hacky and needs
> > > some love.
> > > 
> > > Thanks,
> > > Namhyung
> > > 
> > 
> > Okay, how do we continue with this issue?
> > The patch I submitted uses a __weak function of s390x and does not
> > address other platforms.
> 
> Not sure.  The symbol code is complex and needs to consider many
> cases.  I think we need to rethink the 'adjust_symbols' logic and
> to check adding 'sh_offset' is valid.

right, there are way too many details, we need more tests in 'perf test'
and then go on simplifying that code.

But as a first step I think we can add yet this arch specific bit, at
least we will have it documented how this works in yet another arch,
then, when simplifying the code, we would take it into account, and
retest on those arches.

- Arnaldo
 
> Thanks,
> Namhyung
> 
> 
> > 
> > > 
> > >>
> > >> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
> > >> ---
> > >>  tools/perf/arch/s390/util/sym-handling.c | 7 +++++++
> > >>  tools/perf/util/symbol-elf.c             | 8 +++++++-
> > >>  tools/perf/util/symbol.h                 | 3 +++
> > >>  3 files changed, 17 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
> > >> index b6cd056..79bf238 100644
> > >> --- a/tools/perf/arch/s390/util/sym-handling.c
> > >> +++ b/tools/perf/arch/s390/util/sym-handling.c
> > >> @@ -19,4 +19,11 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
> > >>  	return ehdr.e_type == ET_REL || ehdr.e_type == ET_DYN;
> > >>  }
> > >>  
> > >> +u64 arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr __maybe_unused,
> > >> +			  struct map *map)
> > >> +{
> > >> +	if (map->type == MAP__FUNCTION)
> > >> +		sym->st_value += map->start;
> > >> +	return sym->st_value;
> > >> +}
> > >>  #endif
> > >> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
> > >> index 502505c..d3521d4 100644
> > >> --- a/tools/perf/util/symbol-elf.c
> > >> +++ b/tools/perf/util/symbol-elf.c
> > >> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
> > >>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
> > >>  		GElf_Sym *sym __maybe_unused) { }
> > >>  
> > >> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
> > >> +				 struct map *map __maybe_unused)
> > >> +{
> > >> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
> > >> +}
> > >> +
> > >>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > >>  		  struct symsrc *runtime_ss, int kmodule)
> > >>  {
> > >> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
> > >>  
> > >>  			/* Adjust symbol to map to file offset */
> > >>  			if (adjust_kernel_syms)
> > >> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> > >> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
> > >>  
> > >>  			if (strcmp(section_name,
> > >>  				   (curr_dso->short_name +
> > >> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> > >> index 41ebba9..beb8737 100644
> > >> --- a/tools/perf/util/symbol.h
> > >> +++ b/tools/perf/util/symbol.h
> > >> @@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
> > >>  #ifdef HAVE_LIBELF_SUPPORT
> > >>  bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
> > >>  void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> > >> +u64 arch__sym_set_address(GElf_Sym *sym,
> > >> +			  GElf_Shdr *shdr __maybe_unused,
> > >> +			  struct map *map __maybe_unused);
> > >>  #endif
> > >>  
> > >>  #define SYMBOL_A 0
> > >> -- 
> > >> 2.9.3
> > >>
> > >> --
> > >> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> > >> the body of a message to majordomo@vger.kernel.org
> > >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> > 
> > -- 
> > Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
> > --
> > Vorsitzende des Aufsichtsrats: Martina Koederitz 
> > Geschäftsführung: Dirk Wittkopp
> > Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
> > 

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Thomas-Mich Richter]

On 07/25/2017 04:20 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 25, 2017 at 07:32:40PM +0900, Namhyung Kim escreveu:
>> On Tue, Jul 25, 2017 at 10:49:24AM +0200, Thomas-Mich Richter wrote:
>>> On 07/25/2017 08:14 AM, Namhyung Kim wrote:
>>>> Hello,
>>>>
>>>> On Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter wrote:
>>>>> The perf report tool does not display the addresses of kernel
>>>>> module symbols correctly.
>>>>>
>>>>> For example symbol qeth_send_ipa_cmd in kernel module qeth.ko
>>>>> has this relative address for function qeth_send_ipa_cmd()
>>>>> [root@s8360047 linux]# nm -g drivers/s390/net/qeth.ko | fgrep send_ipa_cmd
>>>>> 0000000000013088 T qeth_send_ipa_cmd
>>>>>
>>>>> The module is loaded at address
>>>>> [root@s8360047 linux]# cat /sys/module/qeth/sections/.text
>>>>> 0x000003ff80296d20
>>>>> [root@s8360047 linux]#
>>>>>
>>>>> This should result in a start address of
>>>>> 0x13088 + 0x3ff80296d20 = 0x3ff802a9da8
>>>>>
>>>>> Using crash to verify the address on a live system:
>>>>> [root@s8360046 linux]# crash vmlinux
>>>>>
>>>>> crash 7.1.9++
>>>>> Copyright (C) 2002-2016  Red Hat, Inc.
>>>>> Copyright (C) 2004, 2005, 2006, 2010  IBM Corporation
>>>>>
>>>>> [...]
>>>>>
>>>>> crash> mod -s qeth drivers/s390/net/qeth.ko
>>>>>      MODULE       NAME        SIZE  OBJECT FILE
>>>>>      3ff8028d700  qeth      151552  drivers/s390/net/qeth.ko
>>>>> crash> sym qeth_send_ipa_cmd
>>>>> 3ff802a9da8 (T) qeth_send_ipa_cmd [qeth] /root/linux/drivers/s390/net/qeth_core_main.c: 2944
>>>>> crash>
>>>>>
>>>>> Now perf report displays the address of symbol qeth_send_ipa_cmd:
>>>>> symbol__new: qeth_send_ipa_cmd 0x130f0-0x132ce
>>>>>
>>>>> There is a difference of 0x68 between the entry in the
>>>>> symbol table (see nm command above) and perf. The difference is from
>>>>> the offset the .text segment of qeth.ko:
>>>>>
>>>>> [root@s8360047 perf]# readelf -a drivers/s390/net/qeth.ko
>>>>> Section Headers:
>>>>>   [Nr] Name              Type             Address           Offset
>>>>>        Size              EntSize          Flags  Link  Info  Align
>>>>>   [ 0]                   NULL             0000000000000000  00000000
>>>>>        0000000000000000  0000000000000000           0     0     0
>>>>>   [ 1] .note.gnu.build-i NOTE             0000000000000000  00000040
>>>>>        0000000000000024  0000000000000000   A       0     0     4
>>>>>   [ 2] .text             PROGBITS         0000000000000000  00000068
>>>>>        000000000001c8a0  0000000000000000  AX       0     0     8
>>>>>
>>>>> As seen the .text segment has an offset of 0x68 with start address 0x0.
>>>>> Therefore 0x68 is added to the address of qeth_send_ipa_cmd and thus
>>>>> 0x13088 + 0x68 = 0x130f0 is displayed.
>>>>>
>>>>> This is wrong, perf report needs to display the start address of
>>>>> symbol qeth_send_ipa_cmd at 0x13088 + qeth.ko.text section start address.
>>>>>
>>>>> The qeth.ko module .text start address is available in the qeth.ko
>>>>> DSO map. Just identify the kernel module symbols and correct the
>>>>> addresses.
>>>>>
>>>>> With the fix I see this correct address for symbol:
>>>>> symbol__new: qeth_send_ipa_cmd 0x3ff802a9da8-0x3ff802a9f86
>>>>>
>>>>> I am not sure if this patch uses the perfect approach.
>>>>> Hints and sugguests welcome.
>>>>
>>>> I saw this problem on x86_64 too and I don't know why we add sh_offset
>>>> to symbols.  To me, current symbol code is somewhat hacky and needs
>>>> some love.
>>>>
>>>> Thanks,
>>>> Namhyung
>>>>
>>>
>>> Okay, how do we continue with this issue?
>>> The patch I submitted uses a __weak function of s390x and does not
>>> address other platforms.
>>
>> Not sure.  The symbol code is complex and needs to consider many
>> cases.  I think we need to rethink the 'adjust_symbols' logic and
>> to check adding 'sh_offset' is valid.
> 
> right, there are way too many details, we need more tests in 'perf test'
> and then go on simplifying that code.
> 
> But as a first step I think we can add yet this arch specific bit, at
> least we will have it documented how this works in yet another arch,
> then, when simplifying the code, we would take it into account, and
> retest on those arches.
> 
> - Arnaldo
> 

Arnaldo, Namhyung,

just a quick reminder, do you intend to pick these two patches?
Should I resent them again?

Please let me know?

>> Thanks,
>> Namhyung
>>
>>
>>>
>>>>
>>>>>
>>>>> Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
>>>>> ---
>>>>>  tools/perf/arch/s390/util/sym-handling.c | 7 +++++++
>>>>>  tools/perf/util/symbol-elf.c             | 8 +++++++-
>>>>>  tools/perf/util/symbol.h                 | 3 +++
>>>>>  3 files changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c
>>>>> index b6cd056..79bf238 100644
>>>>> --- a/tools/perf/arch/s390/util/sym-handling.c
>>>>> +++ b/tools/perf/arch/s390/util/sym-handling.c
>>>>> @@ -19,4 +19,11 @@ bool elf__needs_adjust_symbols(GElf_Ehdr ehdr)
>>>>>  	return ehdr.e_type == ET_REL || ehdr.e_type == ET_DYN;
>>>>>  }
>>>>>  
>>>>> +u64 arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr __maybe_unused,
>>>>> +			  struct map *map)
>>>>> +{
>>>>> +	if (map->type == MAP__FUNCTION)
>>>>> +		sym->st_value += map->start;
>>>>> +	return sym->st_value;
>>>>> +}
>>>>>  #endif
>>>>> diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
>>>>> index 502505c..d3521d4 100644
>>>>> --- a/tools/perf/util/symbol-elf.c
>>>>> +++ b/tools/perf/util/symbol-elf.c
>>>>> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
>>>>>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
>>>>>  		GElf_Sym *sym __maybe_unused) { }
>>>>>  
>>>>> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
>>>>> +				 struct map *map __maybe_unused)
>>>>> +{
>>>>> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
>>>>> +}
>>>>> +
>>>>>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>>>>>  		  struct symsrc *runtime_ss, int kmodule)
>>>>>  {
>>>>> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>>>>>  
>>>>>  			/* Adjust symbol to map to file offset */
>>>>>  			if (adjust_kernel_syms)
>>>>> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
>>>>> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
>>>>>  
>>>>>  			if (strcmp(section_name,
>>>>>  				   (curr_dso->short_name +
>>>>> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
>>>>> index 41ebba9..beb8737 100644
>>>>> --- a/tools/perf/util/symbol.h
>>>>> +++ b/tools/perf/util/symbol.h
>>>>> @@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
>>>>>  #ifdef HAVE_LIBELF_SUPPORT
>>>>>  bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
>>>>>  void arch__sym_update(struct symbol *s, GElf_Sym *sym);
>>>>> +u64 arch__sym_set_address(GElf_Sym *sym,
>>>>> +			  GElf_Shdr *shdr __maybe_unused,
>>>>> +			  struct map *map __maybe_unused);
>>>>>  #endif
>>>>>  
>>>>>  #define SYMBOL_A 0
>>>>> -- 
>>>>> 2.9.3
>>>>>
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
>>>>> the body of a message to majordomo@vger.kernel.org
>>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>
>>>
>>>
>>> -- 
>>> Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
>>> --
>>> Vorsitzende des Aufsichtsrats: Martina Koederitz 
>>> Geschäftsführung: Dirk Wittkopp
>>> Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Arnaldo Carvalho de Melo]

Em Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter escreveu:
> +++ b/tools/perf/util/symbol-elf.c
> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
>  		GElf_Sym *sym __maybe_unused) { }
>  
> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
> +				 struct map *map __maybe_unused)
> +{
> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
> +}
> +
>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>  		  struct symsrc *runtime_ss, int kmodule)
>  {
> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>  
>  			/* Adjust symbol to map to file offset */
>  			if (adjust_kernel_syms)
> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);

Since this is just used when "adjusting" the symbol wrt file offset,
can we have a more descriptive name? Like:

void __weak arch__adjust_sym_map_offset(GElf_Sym *sym, GElf_Shdr *shdr,
					struct map *map __maybe_unused)
{
	sym->st_value -= shdr->sh_addr - shdr->sh_offset;
}

Then have it as:

-			/* Adjust symbol to map to file offset */
 			if (adjust_kernel_syms)
-				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
+				arch__adjust_sym_map_offset(&sym, &shdr, map);

I.e. the weak function name becomes the comment :-)

>  
>  			if (strcmp(section_name,
>  				   (curr_dso->short_name +
> diff --git a/tools/perf/util/symbol.h b/tools/perf/util/symbol.h
> index 41ebba9..beb8737 100644
> --- a/tools/perf/util/symbol.h
> +++ b/tools/perf/util/symbol.h
> @@ -343,6 +343,9 @@ int setup_intlist(struct intlist **list, const char *list_str,
>  #ifdef HAVE_LIBELF_SUPPORT
>  bool elf__needs_adjust_symbols(GElf_Ehdr ehdr);
>  void arch__sym_update(struct symbol *s, GElf_Sym *sym);
> +u64 arch__sym_set_address(GElf_Sym *sym,
> +			  GElf_Shdr *shdr __maybe_unused,
> +			  struct map *map __maybe_unused);
>  #endif
>  
>  #define SYMBOL_A 0
> -- 
> 2.9.3

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Arnaldo Carvalho de Melo]

Em Mon, Jul 24, 2017 at 03:20:07PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter escreveu:
> > +++ b/tools/perf/util/machine.c
> > @@ -20,6 +20,7 @@
> > +#include <sys/user.h>

no need for this header, see below.

> > -static int machine__create_module(void *arg, const char *name, u64 start)
> > +static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
> >  {
> > @@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
> >  	map = machine__findnew_module_map(machine, start, name);
> >  	if (map == NULL)
> >  		return -1;
> > +	map->end = roundup(start + size, PAGE_SIZE);

We have the page_size variable, that gets its value at:

tools/perf/perf.c:	page_size = sysconf(_SC_PAGE_SIZE);

always available, determined at run time.

Also, what is the reason for the roundup?

> >  	dso__kernel_module_get_build_id(map->dso, machine->root_dir);

- Arnaldo

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Thomas-Mich Richter]

On 08/02/2017 08:42 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2017 at 04:35:13PM +0200, Thomas Richter escreveu:
>> +++ b/tools/perf/util/symbol-elf.c
>> @@ -793,6 +793,12 @@ static u64 ref_reloc(struct kmap *kmap)
>>  void __weak arch__sym_update(struct symbol *s __maybe_unused,
>>  		GElf_Sym *sym __maybe_unused) { }
>>  
>> +u64 __weak arch__sym_set_address(GElf_Sym *sym, GElf_Shdr *shdr,
>> +				 struct map *map __maybe_unused)
>> +{
>> +	return sym->st_value - (shdr->sh_addr - shdr->sh_offset);
>> +}
>> +
>>  int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>>  		  struct symsrc *runtime_ss, int kmodule)
>>  {
>> @@ -973,7 +979,7 @@ int dso__load_sym(struct dso *dso, struct map *map, struct symsrc *syms_ss,
>>  
>>  			/* Adjust symbol to map to file offset */
>>  			if (adjust_kernel_syms)
>> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
>> +				sym.st_value = arch__sym_set_address(&sym, &shdr, map);
> 
> Since this is just used when "adjusting" the symbol wrt file offset,
> can we have a more descriptive name? Like:
> 
> void __weak arch__adjust_sym_map_offset(GElf_Sym *sym, GElf_Shdr *shdr,
> 					struct map *map __maybe_unused)
> {
> 	sym->st_value -= shdr->sh_addr - shdr->sh_offset;
> }
> 
> Then have it as:
> 
> -			/* Adjust symbol to map to file offset */
>  			if (adjust_kernel_syms)
> -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> +				arch__adjust_sym_map_offset(&sym, &shdr, map);
> 
> I.e. the weak function name becomes the comment :-)
> 

Ok , I will provide a version 2 with your findings fixed. I'll include a 
Reviewed-by:
if this is ok with you.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Thomas-Mich Richter]

On 08/02/2017 08:50 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2017 at 03:20:07PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter escreveu:
>>> +++ b/tools/perf/util/machine.c
>>> @@ -20,6 +20,7 @@
>>> +#include <sys/user.h>
> 
> no need for this header, see below.
> 
>>> -static int machine__create_module(void *arg, const char *name, u64 start)
>>> +static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
>>>  {
>>> @@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
>>>  	map = machine__findnew_module_map(machine, start, name);
>>>  	if (map == NULL)
>>>  		return -1;
>>> +	map->end = roundup(start + size, PAGE_SIZE);
> 
> We have the page_size variable, that gets its value at:
> 
> tools/perf/perf.c:	page_size = sysconf(_SC_PAGE_SIZE);
> 
> always available, determined at run time.
> 
> Also, what is the reason for the roundup?
> 

Ok I missed that page_size and will use it.
The idea for the roundup to page size is the assumption that the kernel
allocates full pages for memory to load modules. The size filed in the /proc/modules
output refers to the module size in bytes.

If this is wrong we can omit the roundup.

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 1/2] perf report fix module symbol adjustment for s390x

[Arnaldo Carvalho de Melo]

Em Thu, Aug 03, 2017 at 11:37:02AM +0200, Thomas-Mich Richter escreveu:
> On 08/02/2017 08:42 PM, Arnaldo Carvalho de Melo wrote:
> > Then have it as:
> > -			/* Adjust symbol to map to file offset */
> >  			if (adjust_kernel_syms)
> > -				sym.st_value -= shdr.sh_addr - shdr.sh_offset;
> > +				arch__adjust_sym_map_offset(&sym, &shdr, map);

> > I.e. the weak function name becomes the comment :-)
 
> Ok , I will provide a version 2 with your findings fixed. I'll include a 
> Reviewed-by:
> if this is ok with you.

yeah.

- Arnaldo

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Arnaldo Carvalho de Melo]

Em Thu, Aug 03, 2017 at 11:41:02AM +0200, Thomas-Mich Richter escreveu:
> On 08/02/2017 08:50 PM, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Jul 24, 2017 at 03:20:07PM -0300, Arnaldo Carvalho de Melo escreveu:
> >> Em Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter escreveu:
> >>> +++ b/tools/perf/util/machine.c
> >>> @@ -20,6 +20,7 @@
> >>> +#include <sys/user.h>
> > 
> > no need for this header, see below.
> > 
> >>> -static int machine__create_module(void *arg, const char *name, u64 start)
> >>> +static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
> >>>  {
> >>> @@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
> >>>  	map = machine__findnew_module_map(machine, start, name);
> >>>  	if (map == NULL)
> >>>  		return -1;
> >>> +	map->end = roundup(start + size, PAGE_SIZE);
> > 
> > We have the page_size variable, that gets its value at:
> > 
> > tools/perf/perf.c:	page_size = sysconf(_SC_PAGE_SIZE);
> > 
> > always available, determined at run time.
> > 
> > Also, what is the reason for the roundup?
> > 
> 
> Ok I missed that page_size and will use it.
> The idea for the roundup to page size is the assumption that the kernel
> allocates full pages for memory to load modules. The size filed in the /proc/modules
> output refers to the module size in bytes.
> 
> If this is wrong we can omit the roundup.

Its not wrong, just looks unnecessary, a distraction, I'll remove it, ok?

- Arnaldo

 
> -- 
> Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
> --
> Vorsitzende des Aufsichtsrats: Martina Koederitz 
> Geschäftsführung: Dirk Wittkopp
> Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-perf-users" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Thomas-Mich Richter]

On 08/04/2017 07:06 PM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Aug 03, 2017 at 11:41:02AM +0200, Thomas-Mich Richter escreveu:
>> On 08/02/2017 08:50 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Mon, Jul 24, 2017 at 03:20:07PM -0300, Arnaldo Carvalho de Melo escreveu:
>>>> Em Mon, Jul 24, 2017 at 04:35:14PM +0200, Thomas Richter escreveu:
>>>>> +++ b/tools/perf/util/machine.c
>>>>> @@ -20,6 +20,7 @@
>>>>> +#include <sys/user.h>
>>>
>>> no need for this header, see below.
>>>
>>>>> -static int machine__create_module(void *arg, const char *name, u64 start)
>>>>> +static int machine__create_module(void *arg, const char *name, u64 start, u64 size)
>>>>>  {
>>>>> @@ -1148,6 +1149,7 @@ static int machine__create_module(void *arg, const char *name, u64 start)
>>>>>  	map = machine__findnew_module_map(machine, start, name);
>>>>>  	if (map == NULL)
>>>>>  		return -1;
>>>>> +	map->end = roundup(start + size, PAGE_SIZE);
>>>
>>> We have the page_size variable, that gets its value at:
>>>
>>> tools/perf/perf.c:	page_size = sysconf(_SC_PAGE_SIZE);
>>>
>>> always available, determined at run time.
>>>
>>> Also, what is the reason for the roundup?
>>>
>>
>> Ok I missed that page_size and will use it.
>> The idea for the roundup to page size is the assumption that the kernel
>> allocates full pages for memory to load modules. The size filed in the /proc/modules
>> output refers to the module size in bytes.
>>
>> If this is wrong we can omit the roundup.
> 
> Its not wrong, just looks unnecessary, a distraction, I'll remove it, ok?
> 
> - Arnaldo
> 

Sure go ahead.


-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Arnaldo Carvalho de Melo]

Em Mon, Aug 07, 2017 at 09:26:14AM +0200, Thomas-Mich Richter escreveu:
> On 08/04/2017 07:06 PM, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Aug 03, 2017 at 11:41:02AM +0200, Thomas-Mich Richter escreveu:
> >> The idea for the roundup to page size is the assumption that the kernel
> >> allocates full pages for memory to load modules. The size filed in the /proc/modules
> >> output refers to the module size in bytes.

> >> If this is wrong we can omit the roundup.

> > Its not wrong, just looks unnecessary, a distraction, I'll remove it, ok?

> Sure go ahead.

Thanks, applied.

- Arnaldo

Re: [PATCH 2/2] perf record: wrong size in perf_record_mmap for last kernel module

[Thomas-Mich Richter]

On 08/07/2017 06:05 PM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 07, 2017 at 09:26:14AM +0200, Thomas-Mich Richter escreveu:
>> On 08/04/2017 07:06 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Aug 03, 2017 at 11:41:02AM +0200, Thomas-Mich Richter escreveu:
>>>> The idea for the roundup to page size is the assumption that the kernel
>>>> allocates full pages for memory to load modules. The size filed in the /proc/modules
>>>> output refers to the module size in bytes.
> 
>>>> If this is wrong we can omit the roundup.
> 
>>> Its not wrong, just looks unnecessary, a distraction, I'll remove it, ok?
> 
>> Sure go ahead.
> 
> Thanks, applied.
> 
> - Arnaldo

Thanks for picking this one. Out of curiosity, why didn't you pick the first
part of the patch set:

[PATCH 1/2] perf report fix module symbol adjustment for s390x

Anything wrong with it? Should I repost it?
Thanks

-- 
Thomas Richter, Dept 3303, IBM LTC Boeblingen Germany
--
Vorsitzende des Aufsichtsrats: Martina Koederitz 
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294