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

[Thomas-Mich Richter <tmricht@linux.vnet.ibm.com>]

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