* [RFC PATCH V4]s390/perf:fix 'start' address of module's map @ 2016-07-21 3:10 Song Shan Gong 2016-07-21 3:10 ` [PATCH] s390/perf: fix " Song Shan Gong 0 siblings, 1 reply; 17+ messages in thread From: Song Shan Gong @ 2016-07-21 3:10 UTC (permalink / raw) To: acme, jolsa; +Cc: dsahern, linux-kernel, borntraeger, Song Shan Gong Change log: >From V3: 1.move fixup function to tools/perf/arch/s390/util/machine.c; 2. Add acked-by info from Jiri Olsa <jolsa@kernel.org>; >From V2: 1. remove redundancy string 'module_name'; >From V1: 1.change func name from 'fix__arch_module_baseaddr' to 'fix__arch_module_text_start'; 2.Parse '.text' start addr from 'sys' through 'sysfs__read_ull', not 'hex2u64()'; 2.Perfect code: check return value and allocated pointer by 'strdup'. Song Shan Gong (1): s390/perf: fix 'start' address of module's map tools/perf/arch/s390/util/Build | 2 ++ tools/perf/arch/s390/util/sym-handling.c | 27 +++++++++++++++++++++++++++ tools/perf/util/machine.c | 8 ++++++++ tools/perf/util/machine.h | 1 + 4 files changed, 38 insertions(+) create mode 100644 tools/perf/arch/s390/util/sym-handling.c -- 2.3.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] s390/perf: fix 'start' address of module's map 2016-07-21 3:10 [RFC PATCH V4]s390/perf:fix 'start' address of module's map Song Shan Gong @ 2016-07-21 3:10 ` Song Shan Gong 2016-07-22 2:47 ` Songshan Gong ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Song Shan Gong @ 2016-07-21 3:10 UTC (permalink / raw) To: acme, jolsa; +Cc: dsahern, linux-kernel, borntraeger, Song Shan Gong At preset, when creating module's map, perf gets 'start' address by parsing '/proc/modules', but it's module base address, isn't the start address of '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and 'PLT' relocations before '.text' section. So there exists an offset between module base address and '.text' section, which will incur wrong symbol resolution for modules. Fix this bug by getting 'start' address of module's map from parsing '/sys/module/[module name]/sections/.text', not from '/proc/modules'. Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> Acked-by: Jiri Olsa <jolsa@kernel.org> --- tools/perf/arch/s390/util/Build | 2 ++ tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ tools/perf/util/machine.c | 8 ++++++++ tools/perf/util/machine.h | 1 + 4 files changed, 30 insertions(+) create mode 100644 tools/perf/arch/s390/util/machine.c diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build index 8a61372..5bd7b92 100644 --- a/tools/perf/arch/s390/util/Build +++ b/tools/perf/arch/s390/util/Build @@ -2,3 +2,5 @@ libperf-y += header.o libperf-y += kvm-stat.o libperf-$(CONFIG_DWARF) += dwarf-regs.o + +libperf-y += machine.o diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c new file mode 100644 index 0000000..b9a95a1 --- /dev/null +++ b/tools/perf/arch/s390/util/machine.c @@ -0,0 +1,19 @@ +#include <unistd.h> +#include <stdio.h> +#include <string.h> +#include "util.h" +#include "machine.h" +#include "api/fs/fs.h" + +int arch__fix_module_text_start(u64 *start, const char *name) +{ + char path[PATH_MAX]; + + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", + (int)strlen(name) - 2, name + 1); + + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) + return -1; + + return 0; +} diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index b177218..97cc9f0 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); } +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, + const char *name __maybe_unused) +{ + return 0; +} static int machine__create_module(void *arg, const char *name, u64 start) { struct machine *machine = arg; struct map *map; + if (arch__fix_module_text_start(&start, name) < 0) + return -1; + map = machine__findnew_module_map(machine, start, name); if (map == NULL) return -1; diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h index 41ac9cf..20739f7 100644 --- a/tools/perf/util/machine.h +++ b/tools/perf/util/machine.h @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, struct map *machine__findnew_module_map(struct machine *machine, u64 start, const char *filename); +int arch__fix_module_text_start(u64 *start, const char *name); int __machine__load_kallsyms(struct machine *machine, const char *filename, enum map_type type, bool no_kcore, symbol_filter_t filter); -- 2.3.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-21 3:10 ` [PATCH] s390/perf: fix " Song Shan Gong @ 2016-07-22 2:47 ` Songshan Gong 2016-07-26 6:54 ` Jiri Olsa 2016-07-26 19:50 ` Arnaldo Carvalho de Melo 2016-07-27 10:41 ` [tip:perf/urgent] perf s390: Fix " tip-bot for Song Shan Gong 2 siblings, 1 reply; 17+ messages in thread From: Songshan Gong @ 2016-07-22 2:47 UTC (permalink / raw) To: acme, jolsa; +Cc: dsahern, linux-kernel, borntraeger Has the patch been accepted by upstream? 在 7/21/2016 11:10 AM, Song Shan Gong 写道: > At preset, when creating module's map, perf gets 'start' address by parsing > '/proc/modules', but it's module base address, isn't the start address of > '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and > 'PLT' relocations before '.text' section. So there exists an offset between > module base address and '.text' section, which will incur wrong symbol > resolution for modules. > > Fix this bug by getting 'start' address of module's map from parsing > '/sys/module/[module name]/sections/.text', not from '/proc/modules'. > > Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> > Acked-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/arch/s390/util/Build | 2 ++ > tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ > tools/perf/util/machine.c | 8 ++++++++ > tools/perf/util/machine.h | 1 + > 4 files changed, 30 insertions(+) > create mode 100644 tools/perf/arch/s390/util/machine.c > > diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build > index 8a61372..5bd7b92 100644 > --- a/tools/perf/arch/s390/util/Build > +++ b/tools/perf/arch/s390/util/Build > @@ -2,3 +2,5 @@ libperf-y += header.o > libperf-y += kvm-stat.o > > libperf-$(CONFIG_DWARF) += dwarf-regs.o > + > +libperf-y += machine.o > diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c > new file mode 100644 > index 0000000..b9a95a1 > --- /dev/null > +++ b/tools/perf/arch/s390/util/machine.c > @@ -0,0 +1,19 @@ > +#include <unistd.h> > +#include <stdio.h> > +#include <string.h> > +#include "util.h" > +#include "machine.h" > +#include "api/fs/fs.h" > + > +int arch__fix_module_text_start(u64 *start, const char *name) > +{ > + char path[PATH_MAX]; > + > + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", > + (int)strlen(name) - 2, name + 1); > + > + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) > + return -1; > + > + return 0; > +} > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index b177218..97cc9f0 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) > > return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); > } > +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, > + const char *name __maybe_unused) > +{ > + return 0; > +} > > static int machine__create_module(void *arg, const char *name, u64 start) > { > struct machine *machine = arg; > struct map *map; > > + if (arch__fix_module_text_start(&start, name) < 0) > + return -1; > + > map = machine__findnew_module_map(machine, start, name); > if (map == NULL) > return -1; > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h > index 41ac9cf..20739f7 100644 > --- a/tools/perf/util/machine.h > +++ b/tools/perf/util/machine.h > @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, > > struct map *machine__findnew_module_map(struct machine *machine, u64 start, > const char *filename); > +int arch__fix_module_text_start(u64 *start, const char *name); > > int __machine__load_kallsyms(struct machine *machine, const char *filename, > enum map_type type, bool no_kcore, symbol_filter_t filter); > -- SongShan Gong ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-22 2:47 ` Songshan Gong @ 2016-07-26 6:54 ` Jiri Olsa 0 siblings, 0 replies; 17+ messages in thread From: Jiri Olsa @ 2016-07-26 6:54 UTC (permalink / raw) To: Songshan Gong; +Cc: acme, jolsa, dsahern, linux-kernel, borntraeger On Fri, Jul 22, 2016 at 10:47:34AM +0800, Songshan Gong wrote: > Has the patch been accepted by upstream? > > 在 7/21/2016 11:10 AM, Song Shan Gong 写道: > > At preset, when creating module's map, perf gets 'start' address by parsing > > '/proc/modules', but it's module base address, isn't the start address of > > '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and > > 'PLT' relocations before '.text' section. So there exists an offset between > > module base address and '.text' section, which will incur wrong symbol > > resolution for modules. > > > > Fix this bug by getting 'start' address of module's map from parsing > > '/sys/module/[module name]/sections/.text', not from '/proc/modules'. > > > > Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> > > Acked-by: Jiri Olsa <jolsa@kernel.org> I think it's good to go, Arnaldo, could you please take this one? thanks, jirka ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-21 3:10 ` [PATCH] s390/perf: fix " Song Shan Gong 2016-07-22 2:47 ` Songshan Gong @ 2016-07-26 19:50 ` Arnaldo Carvalho de Melo 2016-07-26 20:14 ` Christian Borntraeger 2016-07-27 10:05 ` Songshan Gong 2016-07-27 10:41 ` [tip:perf/urgent] perf s390: Fix " tip-bot for Song Shan Gong 2 siblings, 2 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-07-26 19:50 UTC (permalink / raw) To: Song Shan Gong; +Cc: jolsa, dsahern, linux-kernel, borntraeger Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: > At preset, when creating module's map, perf gets 'start' address by parsing > '/proc/modules', but it's module base address, isn't the start address of > '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and > 'PLT' relocations before '.text' section. So there exists an offset between > module base address and '.text' section, which will incur wrong symbol > resolution for modules. I'll apply this as it fixes the problem for you and we need to get fixes in ASAP to get this into 4.8, but why can't we just use your method for all arches and get rid of this arch__ hook? I.e. if I look here in my x86_64 notebook I see: [acme@jouet linux]$ cat /sys/module/tun/sections/.text 0xffffffffc0af2000 [acme@jouet linux]$ grep tun /proc/modules tun 28672 4 vhost_net, Live 0xffffffffc0af2000 [acme@jouet linux]$ So I could as well use what is in /sys/module/tun/sections/.text instead of reading it from /proc/modules and, in s390, reading it from /sys/module/tun/sections/.text. Do you see any problem with using this approach for _all_ arches? - Arnaldo > Fix this bug by getting 'start' address of module's map from parsing > '/sys/module/[module name]/sections/.text', not from '/proc/modules'. > > Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> > Acked-by: Jiri Olsa <jolsa@kernel.org> > --- > tools/perf/arch/s390/util/Build | 2 ++ > tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ > tools/perf/util/machine.c | 8 ++++++++ > tools/perf/util/machine.h | 1 + > 4 files changed, 30 insertions(+) > create mode 100644 tools/perf/arch/s390/util/machine.c > > diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build > index 8a61372..5bd7b92 100644 > --- a/tools/perf/arch/s390/util/Build > +++ b/tools/perf/arch/s390/util/Build > @@ -2,3 +2,5 @@ libperf-y += header.o > libperf-y += kvm-stat.o > > libperf-$(CONFIG_DWARF) += dwarf-regs.o > + > +libperf-y += machine.o > diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c > new file mode 100644 > index 0000000..b9a95a1 > --- /dev/null > +++ b/tools/perf/arch/s390/util/machine.c > @@ -0,0 +1,19 @@ > +#include <unistd.h> > +#include <stdio.h> > +#include <string.h> > +#include "util.h" > +#include "machine.h" > +#include "api/fs/fs.h" > + > +int arch__fix_module_text_start(u64 *start, const char *name) > +{ > + char path[PATH_MAX]; > + > + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", > + (int)strlen(name) - 2, name + 1); > + > + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) > + return -1; > + > + return 0; > +} > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index b177218..97cc9f0 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) > > return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); > } > +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, > + const char *name __maybe_unused) > +{ > + return 0; > +} > > static int machine__create_module(void *arg, const char *name, u64 start) > { > struct machine *machine = arg; > struct map *map; > > + if (arch__fix_module_text_start(&start, name) < 0) > + return -1; > + > map = machine__findnew_module_map(machine, start, name); > if (map == NULL) > return -1; > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h > index 41ac9cf..20739f7 100644 > --- a/tools/perf/util/machine.h > +++ b/tools/perf/util/machine.h > @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, > > struct map *machine__findnew_module_map(struct machine *machine, u64 start, > const char *filename); > +int arch__fix_module_text_start(u64 *start, const char *name); > > int __machine__load_kallsyms(struct machine *machine, const char *filename, > enum map_type type, bool no_kcore, symbol_filter_t filter); > -- > 2.3.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-26 19:50 ` Arnaldo Carvalho de Melo @ 2016-07-26 20:14 ` Christian Borntraeger 2016-07-26 20:29 ` Arnaldo Carvalho de Melo 2016-07-27 10:05 ` Songshan Gong 1 sibling, 1 reply; 17+ messages in thread From: Christian Borntraeger @ 2016-07-26 20:14 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Song Shan Gong; +Cc: jolsa, dsahern, linux-kernel On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: > Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: >> At preset, when creating module's map, perf gets 'start' address by parsing >> '/proc/modules', but it's module base address, isn't the start address of >> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and >> 'PLT' relocations before '.text' section. So there exists an offset between >> module base address and '.text' section, which will incur wrong symbol >> resolution for modules. > > I'll apply this as it fixes the problem for you and we need to get fixes > in ASAP to get this into 4.8, but why can't we just use your method for > all arches and get rid of this arch__ hook? I.e. if I look here in my > x86_64 notebook I see: > > [acme@jouet linux]$ cat /sys/module/tun/sections/.text > 0xffffffffc0af2000 > [acme@jouet linux]$ grep tun /proc/modules > tun 28672 4 vhost_net, Live 0xffffffffc0af2000 > [acme@jouet linux]$ > > So I could as well use what is in /sys/module/tun/sections/.text instead > of reading it from /proc/modules and, in s390, reading it from > /sys/module/tun/sections/.text. > > Do you see any problem with using this approach for _all_ arches? I think it should work well for _all_ arches but it will probably be hard to test this without help. I wouldn't be surprised if other architectures than s390 actually have the same issue, so doing this for everybody might atually fix this somewhere else. > > - Arnaldo > >> Fix this bug by getting 'start' address of module's map from parsing >> '/sys/module/[module name]/sections/.text', not from '/proc/modules'. >> >> Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> >> Acked-by: Jiri Olsa <jolsa@kernel.org> >> --- >> tools/perf/arch/s390/util/Build | 2 ++ >> tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ >> tools/perf/util/machine.c | 8 ++++++++ >> tools/perf/util/machine.h | 1 + >> 4 files changed, 30 insertions(+) >> create mode 100644 tools/perf/arch/s390/util/machine.c >> >> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build >> index 8a61372..5bd7b92 100644 >> --- a/tools/perf/arch/s390/util/Build >> +++ b/tools/perf/arch/s390/util/Build >> @@ -2,3 +2,5 @@ libperf-y += header.o >> libperf-y += kvm-stat.o >> >> libperf-$(CONFIG_DWARF) += dwarf-regs.o >> + >> +libperf-y += machine.o >> diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c >> new file mode 100644 >> index 0000000..b9a95a1 >> --- /dev/null >> +++ b/tools/perf/arch/s390/util/machine.c >> @@ -0,0 +1,19 @@ >> +#include <unistd.h> >> +#include <stdio.h> >> +#include <string.h> >> +#include "util.h" >> +#include "machine.h" >> +#include "api/fs/fs.h" >> + >> +int arch__fix_module_text_start(u64 *start, const char *name) >> +{ >> + char path[PATH_MAX]; >> + >> + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", >> + (int)strlen(name) - 2, name + 1); >> + >> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) >> + return -1; >> + >> + return 0; >> +} >> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c >> index b177218..97cc9f0 100644 >> --- a/tools/perf/util/machine.c >> +++ b/tools/perf/util/machine.c >> @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) >> >> return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); >> } >> +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, >> + const char *name __maybe_unused) >> +{ >> + return 0; >> +} >> >> static int machine__create_module(void *arg, const char *name, u64 start) >> { >> struct machine *machine = arg; >> struct map *map; >> >> + if (arch__fix_module_text_start(&start, name) < 0) >> + return -1; >> + >> map = machine__findnew_module_map(machine, start, name); >> if (map == NULL) >> return -1; >> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h >> index 41ac9cf..20739f7 100644 >> --- a/tools/perf/util/machine.h >> +++ b/tools/perf/util/machine.h >> @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, >> >> struct map *machine__findnew_module_map(struct machine *machine, u64 start, >> const char *filename); >> +int arch__fix_module_text_start(u64 *start, const char *name); >> >> int __machine__load_kallsyms(struct machine *machine, const char *filename, >> enum map_type type, bool no_kcore, symbol_filter_t filter); >> -- >> 2.3.0 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-26 20:14 ` Christian Borntraeger @ 2016-07-26 20:29 ` Arnaldo Carvalho de Melo 2016-07-27 9:24 ` Michael Ellerman ` (2 more replies) 0 siblings, 3 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-07-26 20:29 UTC (permalink / raw) To: Christian Borntraeger Cc: Song Shan Gong, jolsa, dsahern, linux-kernel, Michael Ellerman Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: > On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: > > Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: > >> At preset, when creating module's map, perf gets 'start' address by parsing > >> '/proc/modules', but it's module base address, isn't the start address of > >> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and > >> 'PLT' relocations before '.text' section. So there exists an offset between > >> module base address and '.text' section, which will incur wrong symbol > >> resolution for modules. > > > > I'll apply this as it fixes the problem for you and we need to get fixes > > in ASAP to get this into 4.8, but why can't we just use your method for > > all arches and get rid of this arch__ hook? I.e. if I look here in my > > x86_64 notebook I see: > > > > [acme@jouet linux]$ cat /sys/module/tun/sections/.text > > 0xffffffffc0af2000 > > [acme@jouet linux]$ grep tun /proc/modules > > tun 28672 4 vhost_net, Live 0xffffffffc0af2000 > > [acme@jouet linux]$ > > > > So I could as well use what is in /sys/module/tun/sections/.text instead > > of reading it from /proc/modules and, in s390, reading it from > > /sys/module/tun/sections/.text. > > > > Do you see any problem with using this approach for _all_ arches? > > I think it should work well for _all_ arches but it will probably be > hard to test this without help. Well, we could check for the cases we don't know, i.e. read from both and warn about cases where it is different, except for s390 where we now which is the right one to pick. > I wouldn't be surprised if other architectures than s390 actually have > the same issue, so doing this for everybody might atually fix this somewhere > else. Would be nice to get info from other arch people, Michael, how this goes on ppc? - Arnaldo > > > > - Arnaldo > > > >> Fix this bug by getting 'start' address of module's map from parsing > >> '/sys/module/[module name]/sections/.text', not from '/proc/modules'. > >> > >> Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> > >> Acked-by: Jiri Olsa <jolsa@kernel.org> > >> --- > >> tools/perf/arch/s390/util/Build | 2 ++ > >> tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ > >> tools/perf/util/machine.c | 8 ++++++++ > >> tools/perf/util/machine.h | 1 + > >> 4 files changed, 30 insertions(+) > >> create mode 100644 tools/perf/arch/s390/util/machine.c > >> > >> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build > >> index 8a61372..5bd7b92 100644 > >> --- a/tools/perf/arch/s390/util/Build > >> +++ b/tools/perf/arch/s390/util/Build > >> @@ -2,3 +2,5 @@ libperf-y += header.o > >> libperf-y += kvm-stat.o > >> > >> libperf-$(CONFIG_DWARF) += dwarf-regs.o > >> + > >> +libperf-y += machine.o > >> diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c > >> new file mode 100644 > >> index 0000000..b9a95a1 > >> --- /dev/null > >> +++ b/tools/perf/arch/s390/util/machine.c > >> @@ -0,0 +1,19 @@ > >> +#include <unistd.h> > >> +#include <stdio.h> > >> +#include <string.h> > >> +#include "util.h" > >> +#include "machine.h" > >> +#include "api/fs/fs.h" > >> + > >> +int arch__fix_module_text_start(u64 *start, const char *name) > >> +{ > >> + char path[PATH_MAX]; > >> + > >> + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", > >> + (int)strlen(name) - 2, name + 1); > >> + > >> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) > >> + return -1; > >> + > >> + return 0; > >> +} > >> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > >> index b177218..97cc9f0 100644 > >> --- a/tools/perf/util/machine.c > >> +++ b/tools/perf/util/machine.c > >> @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) > >> > >> return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); > >> } > >> +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, > >> + const char *name __maybe_unused) > >> +{ > >> + return 0; > >> +} > >> > >> static int machine__create_module(void *arg, const char *name, u64 start) > >> { > >> struct machine *machine = arg; > >> struct map *map; > >> > >> + if (arch__fix_module_text_start(&start, name) < 0) > >> + return -1; > >> + > >> map = machine__findnew_module_map(machine, start, name); > >> if (map == NULL) > >> return -1; > >> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h > >> index 41ac9cf..20739f7 100644 > >> --- a/tools/perf/util/machine.h > >> +++ b/tools/perf/util/machine.h > >> @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, > >> > >> struct map *machine__findnew_module_map(struct machine *machine, u64 start, > >> const char *filename); > >> +int arch__fix_module_text_start(u64 *start, const char *name); > >> > >> int __machine__load_kallsyms(struct machine *machine, const char *filename, > >> enum map_type type, bool no_kcore, symbol_filter_t filter); > >> -- > >> 2.3.0 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-26 20:29 ` Arnaldo Carvalho de Melo @ 2016-07-27 9:24 ` Michael Ellerman 2016-07-27 13:05 ` Arnaldo Carvalho de Melo 2016-07-27 10:10 ` Songshan Gong 2016-07-27 10:49 ` Songshan Gong 2 siblings, 1 reply; 17+ messages in thread From: Michael Ellerman @ 2016-07-27 9:24 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Christian Borntraeger, anton Cc: Song Shan Gong, jolsa, dsahern, linux-kernel Arnaldo Carvalho de Melo <acme@kernel.org> writes: > Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: >> On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: >> > Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: >> >> At preset, when creating module's map, perf gets 'start' address by parsing >> >> '/proc/modules', but it's module base address, isn't the start address of >> >> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and >> >> 'PLT' relocations before '.text' section. So there exists an offset between >> >> module base address and '.text' section, which will incur wrong symbol >> >> resolution for modules. >> > >> > I'll apply this as it fixes the problem for you and we need to get fixes >> > in ASAP to get this into 4.8, but why can't we just use your method for >> > all arches and get rid of this arch__ hook? I.e. if I look here in my >> > x86_64 notebook I see: >> > >> > [acme@jouet linux]$ cat /sys/module/tun/sections/.text >> > 0xffffffffc0af2000 >> > [acme@jouet linux]$ grep tun /proc/modules >> > tun 28672 4 vhost_net, Live 0xffffffffc0af2000 >> > [acme@jouet linux]$ >> > >> > So I could as well use what is in /sys/module/tun/sections/.text instead >> > of reading it from /proc/modules and, in s390, reading it from >> > /sys/module/tun/sections/.text. >> > >> > Do you see any problem with using this approach for _all_ arches? >> >> I think it should work well for _all_ arches but it will probably be >> hard to test this without help. > > Well, we could check for the cases we don't know, i.e. read from both > and warn about cases where it is different, except for s390 where we now > which is the right one to pick. > >> I wouldn't be surprised if other architectures than s390 actually have >> the same issue, so doing this for everybody might atually fix this somewhere >> else. > > Would be nice to get info from other arch people, Michael, how this goes > on ppc? It doesn't look like this is a problem on powerpc - at least I haven't heard of it. Looking at a system I have here, for all modules (26) the value in /proc/modules matches the .text section in /sys. So I think using /sys should be fine for us. cheers ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-27 9:24 ` Michael Ellerman @ 2016-07-27 13:05 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-07-27 13:05 UTC (permalink / raw) To: Michael Ellerman Cc: Christian Borntraeger, anton, Song Shan Gong, jolsa, dsahern, linux-kernel Em Wed, Jul 27, 2016 at 07:24:26PM +1000, Michael Ellerman escreveu: > Arnaldo Carvalho de Melo <acme@kernel.org> writes: > > Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: > >> On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: > >> > So I could as well use what is in /sys/module/tun/sections/.text instead > >> > of reading it from /proc/modules and, in s390, reading it from > >> > /sys/module/tun/sections/.text. > >> > Do you see any problem with using this approach for _all_ arches? > >> I think it should work well for _all_ arches but it will probably be > >> hard to test this without help. > > Well, we could check for the cases we don't know, i.e. read from both > > and warn about cases where it is different, except for s390 where we now > > which is the right one to pick. > >> I wouldn't be surprised if other architectures than s390 actually have > >> the same issue, so doing this for everybody might atually fix this somewhere > >> else. > > Would be nice to get info from other arch people, Michael, how this goes > > on ppc? > It doesn't look like this is a problem on powerpc - at least I haven't > heard of it. > Looking at a system I have here, for all modules (26) the value in > /proc/modules matches the .text section in /sys. > So I think using /sys should be fine for us. Thanks for checking. - Arnaldo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-26 20:29 ` Arnaldo Carvalho de Melo 2016-07-27 9:24 ` Michael Ellerman @ 2016-07-27 10:10 ` Songshan Gong 2016-07-27 10:49 ` Songshan Gong 2 siblings, 0 replies; 17+ messages in thread From: Songshan Gong @ 2016-07-27 10:10 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Christian Borntraeger Cc: jolsa, dsahern, linux-kernel, Michael Ellerman 在 7/27/2016 4:29 AM, Arnaldo Carvalho de Melo 写道: > Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: >> On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: >>> Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: >>>> At preset, when creating module's map, perf gets 'start' address by parsing >>>> '/proc/modules', but it's module base address, isn't the start address of >>>> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and >>>> 'PLT' relocations before '.text' section. So there exists an offset between >>>> module base address and '.text' section, which will incur wrong symbol >>>> resolution for modules. >>> >>> I'll apply this as it fixes the problem for you and we need to get fixes >>> in ASAP to get this into 4.8, but why can't we just use your method for >>> all arches and get rid of this arch__ hook? I.e. if I look here in my >>> x86_64 notebook I see: >>> >>> [acme@jouet linux]$ cat /sys/module/tun/sections/.text >>> 0xffffffffc0af2000 >>> [acme@jouet linux]$ grep tun /proc/modules >>> tun 28672 4 vhost_net, Live 0xffffffffc0af2000 >>> [acme@jouet linux]$ >>> >>> So I could as well use what is in /sys/module/tun/sections/.text instead >>> of reading it from /proc/modules and, in s390, reading it from >>> /sys/module/tun/sections/.text. >>> >>> Do you see any problem with using this approach for _all_ arches? >> >> I think it should work well for _all_ arches but it will probably be >> hard to test this without help. > > Well, we could check for the cases we don't know, i.e. read from both > and warn about cases where it is different, except for s390 where we now > which is the right one to pick. Good idea! > >> I wouldn't be surprised if other architectures than s390 actually have >> the same issue, so doing this for everybody might atually fix this somewhere >> else. > > Would be nice to get info from other arch people, Michael, how this goes > on ppc? > > - Arnaldo > >>> >>> - Arnaldo >>> >>>> Fix this bug by getting 'start' address of module's map from parsing >>>> '/sys/module/[module name]/sections/.text', not from '/proc/modules'. >>>> >>>> Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> >>>> Acked-by: Jiri Olsa <jolsa@kernel.org> >>>> --- >>>> tools/perf/arch/s390/util/Build | 2 ++ >>>> tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ >>>> tools/perf/util/machine.c | 8 ++++++++ >>>> tools/perf/util/machine.h | 1 + >>>> 4 files changed, 30 insertions(+) >>>> create mode 100644 tools/perf/arch/s390/util/machine.c >>>> >>>> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build >>>> index 8a61372..5bd7b92 100644 >>>> --- a/tools/perf/arch/s390/util/Build >>>> +++ b/tools/perf/arch/s390/util/Build >>>> @@ -2,3 +2,5 @@ libperf-y += header.o >>>> libperf-y += kvm-stat.o >>>> >>>> libperf-$(CONFIG_DWARF) += dwarf-regs.o >>>> + >>>> +libperf-y += machine.o >>>> diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c >>>> new file mode 100644 >>>> index 0000000..b9a95a1 >>>> --- /dev/null >>>> +++ b/tools/perf/arch/s390/util/machine.c >>>> @@ -0,0 +1,19 @@ >>>> +#include <unistd.h> >>>> +#include <stdio.h> >>>> +#include <string.h> >>>> +#include "util.h" >>>> +#include "machine.h" >>>> +#include "api/fs/fs.h" >>>> + >>>> +int arch__fix_module_text_start(u64 *start, const char *name) >>>> +{ >>>> + char path[PATH_MAX]; >>>> + >>>> + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", >>>> + (int)strlen(name) - 2, name + 1); >>>> + >>>> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) >>>> + return -1; >>>> + >>>> + return 0; >>>> +} >>>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c >>>> index b177218..97cc9f0 100644 >>>> --- a/tools/perf/util/machine.c >>>> +++ b/tools/perf/util/machine.c >>>> @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) >>>> >>>> return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); >>>> } >>>> +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, >>>> + const char *name __maybe_unused) >>>> +{ >>>> + return 0; >>>> +} >>>> >>>> static int machine__create_module(void *arg, const char *name, u64 start) >>>> { >>>> struct machine *machine = arg; >>>> struct map *map; >>>> >>>> + if (arch__fix_module_text_start(&start, name) < 0) >>>> + return -1; >>>> + >>>> map = machine__findnew_module_map(machine, start, name); >>>> if (map == NULL) >>>> return -1; >>>> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h >>>> index 41ac9cf..20739f7 100644 >>>> --- a/tools/perf/util/machine.h >>>> +++ b/tools/perf/util/machine.h >>>> @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, >>>> >>>> struct map *machine__findnew_module_map(struct machine *machine, u64 start, >>>> const char *filename); >>>> +int arch__fix_module_text_start(u64 *start, const char *name); >>>> >>>> int __machine__load_kallsyms(struct machine *machine, const char *filename, >>>> enum map_type type, bool no_kcore, symbol_filter_t filter); >>>> -- >>>> 2.3.0 >>> > -- SongShan Gong ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-26 20:29 ` Arnaldo Carvalho de Melo 2016-07-27 9:24 ` Michael Ellerman 2016-07-27 10:10 ` Songshan Gong @ 2016-07-27 10:49 ` Songshan Gong 2016-07-27 13:08 ` Arnaldo Carvalho de Melo 2 siblings, 1 reply; 17+ messages in thread From: Songshan Gong @ 2016-07-27 10:49 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Christian Borntraeger Cc: jolsa, dsahern, linux-kernel, Michael Ellerman 在 7/27/2016 4:29 AM, Arnaldo Carvalho de Melo 写道: > Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: >> On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: >>> Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: >>>> At preset, when creating module's map, perf gets 'start' address by parsing >>>> '/proc/modules', but it's module base address, isn't the start address of >>>> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and >>>> 'PLT' relocations before '.text' section. So there exists an offset between >>>> module base address and '.text' section, which will incur wrong symbol >>>> resolution for modules. >>> >>> I'll apply this as it fixes the problem for you and we need to get fixes >>> in ASAP to get this into 4.8, but why can't we just use your method for >>> all arches and get rid of this arch__ hook? I.e. if I look here in my >>> x86_64 notebook I see: >>> >>> [acme@jouet linux]$ cat /sys/module/tun/sections/.text >>> 0xffffffffc0af2000 >>> [acme@jouet linux]$ grep tun /proc/modules >>> tun 28672 4 vhost_net, Live 0xffffffffc0af2000 >>> [acme@jouet linux]$ >>> >>> So I could as well use what is in /sys/module/tun/sections/.text instead >>> of reading it from /proc/modules and, in s390, reading it from >>> /sys/module/tun/sections/.text. >>> >>> Do you see any problem with using this approach for _all_ arches? >> >> I think it should work well for _all_ arches but it will probably be >> hard to test this without help. > > Well, we could check for the cases we don't know, i.e. read from both > and warn about cases where it is different, except for s390 where we now > which is the right one to pick. > One question: how to get arch info except machine->env->arch? It seems that machine->env->arch could be NULL sometimes. >> I wouldn't be surprised if other architectures than s390 actually have >> the same issue, so doing this for everybody might atually fix this somewhere >> else. > > Would be nice to get info from other arch people, Michael, how this goes > on ppc? > > - Arnaldo > >>> >>> - Arnaldo >>> >>>> Fix this bug by getting 'start' address of module's map from parsing >>>> '/sys/module/[module name]/sections/.text', not from '/proc/modules'. >>>> >>>> Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> >>>> Acked-by: Jiri Olsa <jolsa@kernel.org> >>>> --- >>>> tools/perf/arch/s390/util/Build | 2 ++ >>>> tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ >>>> tools/perf/util/machine.c | 8 ++++++++ >>>> tools/perf/util/machine.h | 1 + >>>> 4 files changed, 30 insertions(+) >>>> create mode 100644 tools/perf/arch/s390/util/machine.c >>>> >>>> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build >>>> index 8a61372..5bd7b92 100644 >>>> --- a/tools/perf/arch/s390/util/Build >>>> +++ b/tools/perf/arch/s390/util/Build >>>> @@ -2,3 +2,5 @@ libperf-y += header.o >>>> libperf-y += kvm-stat.o >>>> >>>> libperf-$(CONFIG_DWARF) += dwarf-regs.o >>>> + >>>> +libperf-y += machine.o >>>> diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c >>>> new file mode 100644 >>>> index 0000000..b9a95a1 >>>> --- /dev/null >>>> +++ b/tools/perf/arch/s390/util/machine.c >>>> @@ -0,0 +1,19 @@ >>>> +#include <unistd.h> >>>> +#include <stdio.h> >>>> +#include <string.h> >>>> +#include "util.h" >>>> +#include "machine.h" >>>> +#include "api/fs/fs.h" >>>> + >>>> +int arch__fix_module_text_start(u64 *start, const char *name) >>>> +{ >>>> + char path[PATH_MAX]; >>>> + >>>> + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", >>>> + (int)strlen(name) - 2, name + 1); >>>> + >>>> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) >>>> + return -1; >>>> + >>>> + return 0; >>>> +} >>>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c >>>> index b177218..97cc9f0 100644 >>>> --- a/tools/perf/util/machine.c >>>> +++ b/tools/perf/util/machine.c >>>> @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) >>>> >>>> return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); >>>> } >>>> +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, >>>> + const char *name __maybe_unused) >>>> +{ >>>> + return 0; >>>> +} >>>> >>>> static int machine__create_module(void *arg, const char *name, u64 start) >>>> { >>>> struct machine *machine = arg; >>>> struct map *map; >>>> >>>> + if (arch__fix_module_text_start(&start, name) < 0) >>>> + return -1; >>>> + >>>> map = machine__findnew_module_map(machine, start, name); >>>> if (map == NULL) >>>> return -1; >>>> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h >>>> index 41ac9cf..20739f7 100644 >>>> --- a/tools/perf/util/machine.h >>>> +++ b/tools/perf/util/machine.h >>>> @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, >>>> >>>> struct map *machine__findnew_module_map(struct machine *machine, u64 start, >>>> const char *filename); >>>> +int arch__fix_module_text_start(u64 *start, const char *name); >>>> >>>> int __machine__load_kallsyms(struct machine *machine, const char *filename, >>>> enum map_type type, bool no_kcore, symbol_filter_t filter); >>>> -- >>>> 2.3.0 >>> > -- SongShan Gong ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-27 10:49 ` Songshan Gong @ 2016-07-27 13:08 ` Arnaldo Carvalho de Melo 2016-07-28 2:01 ` Songshan Gong 0 siblings, 1 reply; 17+ messages in thread From: Arnaldo Carvalho de Melo @ 2016-07-27 13:08 UTC (permalink / raw) To: Songshan Gong Cc: Christian Borntraeger, jolsa, dsahern, linux-kernel, Michael Ellerman Em Wed, Jul 27, 2016 at 06:49:48PM +0800, Songshan Gong escreveu: > > > 在 7/27/2016 4:29 AM, Arnaldo Carvalho de Melo 写道: > > Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: > > > On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: > > > > Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: > > > > > At preset, when creating module's map, perf gets 'start' address by parsing > > > > > '/proc/modules', but it's module base address, isn't the start address of > > > > > '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and > > > > > 'PLT' relocations before '.text' section. So there exists an offset between > > > > > module base address and '.text' section, which will incur wrong symbol > > > > > resolution for modules. > > > > > > > > I'll apply this as it fixes the problem for you and we need to get fixes > > > > in ASAP to get this into 4.8, but why can't we just use your method for > > > > all arches and get rid of this arch__ hook? I.e. if I look here in my > > > > x86_64 notebook I see: > > > > > > > > [acme@jouet linux]$ cat /sys/module/tun/sections/.text > > > > 0xffffffffc0af2000 > > > > [acme@jouet linux]$ grep tun /proc/modules > > > > tun 28672 4 vhost_net, Live 0xffffffffc0af2000 > > > > [acme@jouet linux]$ > > > > > > > > So I could as well use what is in /sys/module/tun/sections/.text instead > > > > of reading it from /proc/modules and, in s390, reading it from > > > > /sys/module/tun/sections/.text. > > > > > > > > Do you see any problem with using this approach for _all_ arches? > > > > > > I think it should work well for _all_ arches but it will probably be > > > hard to test this without help. > > > > Well, we could check for the cases we don't know, i.e. read from both > > and warn about cases where it is different, except for s390 where we now > > which is the right one to pick. > > > One question: how to get arch info except machine->env->arch? It seems that > machine->env->arch could be NULL sometimes. That is not what you want to look at, as it is related to perf.data, which may not be related to what you want, which is the running machine. For that use uname() -> utsname.machine. But then, why would you need it for reading /sys/module/*/sections/.text? The arch name isn't there :-) - Arnaldo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-27 13:08 ` Arnaldo Carvalho de Melo @ 2016-07-28 2:01 ` Songshan Gong 0 siblings, 0 replies; 17+ messages in thread From: Songshan Gong @ 2016-07-28 2:01 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Christian Borntraeger, jolsa, dsahern, linux-kernel, Michael Ellerman 在 7/27/2016 9:08 PM, Arnaldo Carvalho de Melo 写道: > Em Wed, Jul 27, 2016 at 06:49:48PM +0800, Songshan Gong escreveu: >> >> >> 在 7/27/2016 4:29 AM, Arnaldo Carvalho de Melo 写道: >>> Em Tue, Jul 26, 2016 at 10:14:18PM +0200, Christian Borntraeger escreveu: >>>> On 07/26/2016 09:50 PM, Arnaldo Carvalho de Melo wrote: >>>>> Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: >>>>>> At preset, when creating module's map, perf gets 'start' address by parsing >>>>>> '/proc/modules', but it's module base address, isn't the start address of >>>>>> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and >>>>>> 'PLT' relocations before '.text' section. So there exists an offset between >>>>>> module base address and '.text' section, which will incur wrong symbol >>>>>> resolution for modules. >>>>> >>>>> I'll apply this as it fixes the problem for you and we need to get fixes >>>>> in ASAP to get this into 4.8, but why can't we just use your method for >>>>> all arches and get rid of this arch__ hook? I.e. if I look here in my >>>>> x86_64 notebook I see: >>>>> >>>>> [acme@jouet linux]$ cat /sys/module/tun/sections/.text >>>>> 0xffffffffc0af2000 >>>>> [acme@jouet linux]$ grep tun /proc/modules >>>>> tun 28672 4 vhost_net, Live 0xffffffffc0af2000 >>>>> [acme@jouet linux]$ >>>>> >>>>> So I could as well use what is in /sys/module/tun/sections/.text instead >>>>> of reading it from /proc/modules and, in s390, reading it from >>>>> /sys/module/tun/sections/.text. >>>>> >>>>> Do you see any problem with using this approach for _all_ arches? >>>> >>>> I think it should work well for _all_ arches but it will probably be >>>> hard to test this without help. >>> >>> Well, we could check for the cases we don't know, i.e. read from both >>> and warn about cases where it is different, except for s390 where we now >>> which is the right one to pick. >>> >> One question: how to get arch info except machine->env->arch? It seems that >> machine->env->arch could be NULL sometimes. > > That is not what you want to look at, as it is related to perf.data, > which may not be related to what you want, which is the running machine. > > For that use uname() -> utsname.machine. > > But then, why would you need it for reading /sys/module/*/sections/.text? > The arch name isn't there :-) Yes, it's no use for this patch. Thanks a lot. > > - Arnaldo > -- SongShan Gong ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-26 19:50 ` Arnaldo Carvalho de Melo 2016-07-26 20:14 ` Christian Borntraeger @ 2016-07-27 10:05 ` Songshan Gong 1 sibling, 0 replies; 17+ messages in thread From: Songshan Gong @ 2016-07-27 10:05 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: jolsa, dsahern, linux-kernel, borntraeger 在 7/27/2016 3:50 AM, Arnaldo Carvalho de Melo 写道: > Em Thu, Jul 21, 2016 at 11:10:51AM +0800, Song Shan Gong escreveu: >> At preset, when creating module's map, perf gets 'start' address by parsing >> '/proc/modules', but it's module base address, isn't the start address of >> '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and >> 'PLT' relocations before '.text' section. So there exists an offset between >> module base address and '.text' section, which will incur wrong symbol >> resolution for modules. > > I'll apply this as it fixes the problem for you and we need to get fixes > in ASAP to get this into 4.8, but why can't we just use your method for > all arches and get rid of this arch__ hook? I.e. if I look here in my > x86_64 notebook I see: > > [acme@jouet linux]$ cat /sys/module/tun/sections/.text > 0xffffffffc0af2000 > [acme@jouet linux]$ grep tun /proc/modules > tun 28672 4 vhost_net, Live 0xffffffffc0af2000 > [acme@jouet linux]$ > > So I could as well use what is in /sys/module/tun/sections/.text instead > of reading it from /proc/modules and, in s390, reading it from > /sys/module/tun/sections/.text. > I also think it should be OK for most archs. > Do you see any problem with using this approach for _all_ arches? > > - Arnaldo > >> Fix this bug by getting 'start' address of module's map from parsing >> '/sys/module/[module name]/sections/.text', not from '/proc/modules'. >> >> Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> >> Acked-by: Jiri Olsa <jolsa@kernel.org> >> --- >> tools/perf/arch/s390/util/Build | 2 ++ >> tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ >> tools/perf/util/machine.c | 8 ++++++++ >> tools/perf/util/machine.h | 1 + >> 4 files changed, 30 insertions(+) >> create mode 100644 tools/perf/arch/s390/util/machine.c >> >> diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build >> index 8a61372..5bd7b92 100644 >> --- a/tools/perf/arch/s390/util/Build >> +++ b/tools/perf/arch/s390/util/Build >> @@ -2,3 +2,5 @@ libperf-y += header.o >> libperf-y += kvm-stat.o >> >> libperf-$(CONFIG_DWARF) += dwarf-regs.o >> + >> +libperf-y += machine.o >> diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c >> new file mode 100644 >> index 0000000..b9a95a1 >> --- /dev/null >> +++ b/tools/perf/arch/s390/util/machine.c >> @@ -0,0 +1,19 @@ >> +#include <unistd.h> >> +#include <stdio.h> >> +#include <string.h> >> +#include "util.h" >> +#include "machine.h" >> +#include "api/fs/fs.h" >> + >> +int arch__fix_module_text_start(u64 *start, const char *name) >> +{ >> + char path[PATH_MAX]; >> + >> + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", >> + (int)strlen(name) - 2, name + 1); >> + >> + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) >> + return -1; >> + >> + return 0; >> +} >> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c >> index b177218..97cc9f0 100644 >> --- a/tools/perf/util/machine.c >> +++ b/tools/perf/util/machine.c >> @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) >> >> return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); >> } >> +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, >> + const char *name __maybe_unused) >> +{ >> + return 0; >> +} >> >> static int machine__create_module(void *arg, const char *name, u64 start) >> { >> struct machine *machine = arg; >> struct map *map; >> >> + if (arch__fix_module_text_start(&start, name) < 0) >> + return -1; >> + >> map = machine__findnew_module_map(machine, start, name); >> if (map == NULL) >> return -1; >> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h >> index 41ac9cf..20739f7 100644 >> --- a/tools/perf/util/machine.h >> +++ b/tools/perf/util/machine.h >> @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, >> >> struct map *machine__findnew_module_map(struct machine *machine, u64 start, >> const char *filename); >> +int arch__fix_module_text_start(u64 *start, const char *name); >> >> int __machine__load_kallsyms(struct machine *machine, const char *filename, >> enum map_type type, bool no_kcore, symbol_filter_t filter); >> -- >> 2.3.0 > -- SongShan Gong ^ permalink raw reply [flat|nested] 17+ messages in thread
* [tip:perf/urgent] perf s390: Fix 'start' address of module's map 2016-07-21 3:10 ` [PATCH] s390/perf: fix " Song Shan Gong 2016-07-22 2:47 ` Songshan Gong 2016-07-26 19:50 ` Arnaldo Carvalho de Melo @ 2016-07-27 10:41 ` tip-bot for Song Shan Gong 2 siblings, 0 replies; 17+ messages in thread From: tip-bot for Song Shan Gong @ 2016-07-27 10:41 UTC (permalink / raw) To: linux-tip-commits Cc: tglx, gongss, mingo, acme, jolsa, dsahern, linux-kernel, borntraeger, hpa Commit-ID: 203d8a4aa6edf2c19206316d439ec5dae52ce581 Gitweb: http://git.kernel.org/tip/203d8a4aa6edf2c19206316d439ec5dae52ce581 Author: Song Shan Gong <gongss@linux.vnet.ibm.com> AuthorDate: Thu, 21 Jul 2016 11:10:51 +0800 Committer: Arnaldo Carvalho de Melo <acme@redhat.com> CommitDate: Tue, 26 Jul 2016 16:46:12 -0300 perf s390: Fix 'start' address of module's map At present, when creating module's map, perf gets 'start' address by parsing '/proc/modules', but it's the module base address, it isn't the start address of the '.text' section. In most arches, it's OK. But for s390, it places 'GOT' and 'PLT' relocations before '.text' section. So there exists an offset between module base address and '.text' section, which will incur wrong symbol resolution for modules. Fix this bug by getting 'start' address of module's map from parsing '/sys/module/[module name]/sections/.text', not from '/proc/modules'. Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> Acked-by: Jiri Olsa <jolsa@kernel.org> Cc: Christian Borntraeger <borntraeger@de.ibm.com> Cc: David Ahern <dsahern@gmail.com> Link: http://lkml.kernel.org/r/1469070651-6447-2-git-send-email-gongss@linux.vnet.ibm.com Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- tools/perf/arch/s390/util/Build | 2 ++ tools/perf/arch/s390/util/machine.c | 19 +++++++++++++++++++ tools/perf/util/machine.c | 8 ++++++++ tools/perf/util/machine.h | 1 + 4 files changed, 30 insertions(+) diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build index 8a61372..5bd7b92 100644 --- a/tools/perf/arch/s390/util/Build +++ b/tools/perf/arch/s390/util/Build @@ -2,3 +2,5 @@ libperf-y += header.o libperf-y += kvm-stat.o libperf-$(CONFIG_DWARF) += dwarf-regs.o + +libperf-y += machine.o diff --git a/tools/perf/arch/s390/util/machine.c b/tools/perf/arch/s390/util/machine.c new file mode 100644 index 0000000..b9a95a1 --- /dev/null +++ b/tools/perf/arch/s390/util/machine.c @@ -0,0 +1,19 @@ +#include <unistd.h> +#include <stdio.h> +#include <string.h> +#include "util.h" +#include "machine.h" +#include "api/fs/fs.h" + +int arch__fix_module_text_start(u64 *start, const char *name) +{ + char path[PATH_MAX]; + + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", + (int)strlen(name) - 2, name + 1); + + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) + return -1; + + return 0; +} diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index bc2cdbd..cb6388d 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -1093,12 +1093,20 @@ static int machine__set_modules_path(struct machine *machine) return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); } +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, + const char *name __maybe_unused) +{ + return 0; +} static int machine__create_module(void *arg, const char *name, u64 start) { struct machine *machine = arg; struct map *map; + if (arch__fix_module_text_start(&start, name) < 0) + return -1; + map = machine__findnew_module_map(machine, start, name); if (map == NULL) return -1; diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h index 41ac9cf..20739f7 100644 --- a/tools/perf/util/machine.h +++ b/tools/perf/util/machine.h @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, struct map *machine__findnew_module_map(struct machine *machine, u64 start, const char *filename); +int arch__fix_module_text_start(u64 *start, const char *name); int __machine__load_kallsyms(struct machine *machine, const char *filename, enum map_type type, bool no_kcore, symbol_filter_t filter); ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [RFC PATCH V3]s390/perf:fix 'start' address of module's map @ 2016-07-19 11:31 Song Shan Gong 2016-07-19 11:31 ` [PATCH] s390/perf: fix " Song Shan Gong 0 siblings, 1 reply; 17+ messages in thread From: Song Shan Gong @ 2016-07-19 11:31 UTC (permalink / raw) To: acme, jolsa; +Cc: borntraeger, dsahern, linux-kernel, Song Shan Gong Change log: >From V2: 1. remove redundancy string 'module_name'; >From V1: 1.change func name from 'fix__arch_module_baseaddr' to 'fix__arch_module_text_start'; 2.Parse '.text' start addr from 'sys' through 'sysfs__read_ull', not 'hex2u64()'; 2.Perfect code: check return value and allocated pointer by 'strdup'. Song Shan Gong (1): s390/perf: fix 'start' address of module's map tools/perf/arch/s390/util/Build | 2 ++ tools/perf/arch/s390/util/sym-handling.c | 27 +++++++++++++++++++++++++++ tools/perf/util/machine.c | 8 ++++++++ tools/perf/util/machine.h | 1 + 4 files changed, 38 insertions(+) create mode 100644 tools/perf/arch/s390/util/sym-handling.c -- 2.3.0 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH] s390/perf: fix 'start' address of module's map 2016-07-19 11:31 [RFC PATCH V3]s390/perf:fix " Song Shan Gong @ 2016-07-19 11:31 ` Song Shan Gong 2016-07-20 11:51 ` Jiri Olsa 0 siblings, 1 reply; 17+ messages in thread From: Song Shan Gong @ 2016-07-19 11:31 UTC (permalink / raw) To: acme, jolsa; +Cc: borntraeger, dsahern, linux-kernel, Song Shan Gong At preset, when creating module's map, perf gets 'start' address by parsing '/proc/modules', but it's module base address, isn't the start address of '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and 'PLT' relocations before '.text' section. So there exists an offset between module base address and '.text' section, which will incur wrong symbol resolution for modules. Fix this bug by getting 'start' address of module's map from parsing '/sys/module/[module name]/sections/.text', not from '/proc/modules'. Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> --- tools/perf/arch/s390/util/Build | 2 ++ tools/perf/arch/s390/util/sym-handling.c | 19 +++++++++++++++++++ tools/perf/util/machine.c | 8 ++++++++ tools/perf/util/machine.h | 1 + 4 files changed, 30 insertions(+) create mode 100644 tools/perf/arch/s390/util/sym-handling.c diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build index 8a61372..5e322ed 100644 --- a/tools/perf/arch/s390/util/Build +++ b/tools/perf/arch/s390/util/Build @@ -2,3 +2,5 @@ libperf-y += header.o libperf-y += kvm-stat.o libperf-$(CONFIG_DWARF) += dwarf-regs.o + +libperf-y += sym-handling.o diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c new file mode 100644 index 0000000..ebfc55d --- /dev/null +++ b/tools/perf/arch/s390/util/sym-handling.c @@ -0,0 +1,19 @@ +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> +#include "util.h" +#include "machine.h" +#include "api/fs/fs.h" + +int arch__fix_module_text_start(u64 *start, const char *name) +{ + char path[PATH_MAX]; + + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", + (int)strlen(name) - 2, name + 1); + + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) + return -1; + + return 0; +} diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index b177218..97cc9f0 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); } +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, + const char *name __maybe_unused) +{ + return 0; +} static int machine__create_module(void *arg, const char *name, u64 start) { struct machine *machine = arg; struct map *map; + if (arch__fix_module_text_start(&start, name) < 0) + return -1; + map = machine__findnew_module_map(machine, start, name); if (map == NULL) return -1; diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h index 41ac9cf..20739f7 100644 --- a/tools/perf/util/machine.h +++ b/tools/perf/util/machine.h @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, struct map *machine__findnew_module_map(struct machine *machine, u64 start, const char *filename); +int arch__fix_module_text_start(u64 *start, const char *name); int __machine__load_kallsyms(struct machine *machine, const char *filename, enum map_type type, bool no_kcore, symbol_filter_t filter); -- 2.3.0 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] s390/perf: fix 'start' address of module's map 2016-07-19 11:31 ` [PATCH] s390/perf: fix " Song Shan Gong @ 2016-07-20 11:51 ` Jiri Olsa 0 siblings, 0 replies; 17+ messages in thread From: Jiri Olsa @ 2016-07-20 11:51 UTC (permalink / raw) To: Song Shan Gong; +Cc: acme, jolsa, borntraeger, dsahern, linux-kernel On Tue, Jul 19, 2016 at 07:31:58PM +0800, Song Shan Gong wrote: > At preset, when creating module's map, perf gets 'start' address by parsing > '/proc/modules', but it's module base address, isn't the start address of > '.text' section. In most archs, it's OK. But for s390, it places 'GOT' and > 'PLT' relocations before '.text' section. So there exists an offset between > module base address and '.text' section, which will incur wrong symbol > resolution for modules. > > Fix this bug by getting 'start' address of module's map from parsing > '/sys/module/[module name]/sections/.text', not from '/proc/modules'. > > Signed-off-by: Song Shan Gong <gongss@linux.vnet.ibm.com> > --- > tools/perf/arch/s390/util/Build | 2 ++ > tools/perf/arch/s390/util/sym-handling.c | 19 +++++++++++++++++++ > tools/perf/util/machine.c | 8 ++++++++ > tools/perf/util/machine.h | 1 + > 4 files changed, 30 insertions(+) > create mode 100644 tools/perf/arch/s390/util/sym-handling.c > > diff --git a/tools/perf/arch/s390/util/Build b/tools/perf/arch/s390/util/Build > index 8a61372..5e322ed 100644 > --- a/tools/perf/arch/s390/util/Build > +++ b/tools/perf/arch/s390/util/Build > @@ -2,3 +2,5 @@ libperf-y += header.o > libperf-y += kvm-stat.o > > libperf-$(CONFIG_DWARF) += dwarf-regs.o > + > +libperf-y += sym-handling.o > diff --git a/tools/perf/arch/s390/util/sym-handling.c b/tools/perf/arch/s390/util/sym-handling.c > new file mode 100644 > index 0000000..ebfc55d > --- /dev/null > +++ b/tools/perf/arch/s390/util/sym-handling.c I wonder we should rather put this into arch/s390/util/machine.c other than that: Acked-by: Jiri Olsa <jolsa@kernel.org> thanks, jirka > @@ -0,0 +1,19 @@ > +#include <stdio.h> > +#include <stdlib.h> > +#include <unistd.h> > +#include "util.h" > +#include "machine.h" > +#include "api/fs/fs.h" > + > +int arch__fix_module_text_start(u64 *start, const char *name) > +{ > + char path[PATH_MAX]; > + > + snprintf(path, PATH_MAX, "module/%.*s/sections/.text", > + (int)strlen(name) - 2, name + 1); > + > + if (sysfs__read_ull(path, (unsigned long long *)start) < 0) > + return -1; > + > + return 0; > +} > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index b177218..97cc9f0 100644 > --- a/tools/perf/util/machine.c > +++ b/tools/perf/util/machine.c > @@ -1091,12 +1091,20 @@ static int machine__set_modules_path(struct machine *machine) > > return map_groups__set_modules_path_dir(&machine->kmaps, modules_path, 0); > } > +int __weak arch__fix_module_text_start(u64 *start __maybe_unused, > + const char *name __maybe_unused) > +{ > + return 0; > +} > > static int machine__create_module(void *arg, const char *name, u64 start) > { > struct machine *machine = arg; > struct map *map; > > + if (arch__fix_module_text_start(&start, name) < 0) > + return -1; > + > map = machine__findnew_module_map(machine, start, name); > if (map == NULL) > return -1; > diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h > index 41ac9cf..20739f7 100644 > --- a/tools/perf/util/machine.h > +++ b/tools/perf/util/machine.h > @@ -216,6 +216,7 @@ struct symbol *machine__find_kernel_function_by_name(struct machine *machine, > > struct map *machine__findnew_module_map(struct machine *machine, u64 start, > const char *filename); > +int arch__fix_module_text_start(u64 *start, const char *name); > > int __machine__load_kallsyms(struct machine *machine, const char *filename, > enum map_type type, bool no_kcore, symbol_filter_t filter); > -- > 2.3.0 > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2016-07-28 2:01 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-21 3:10 [RFC PATCH V4]s390/perf:fix 'start' address of module's map Song Shan Gong 2016-07-21 3:10 ` [PATCH] s390/perf: fix " Song Shan Gong 2016-07-22 2:47 ` Songshan Gong 2016-07-26 6:54 ` Jiri Olsa 2016-07-26 19:50 ` Arnaldo Carvalho de Melo 2016-07-26 20:14 ` Christian Borntraeger 2016-07-26 20:29 ` Arnaldo Carvalho de Melo 2016-07-27 9:24 ` Michael Ellerman 2016-07-27 13:05 ` Arnaldo Carvalho de Melo 2016-07-27 10:10 ` Songshan Gong 2016-07-27 10:49 ` Songshan Gong 2016-07-27 13:08 ` Arnaldo Carvalho de Melo 2016-07-28 2:01 ` Songshan Gong 2016-07-27 10:05 ` Songshan Gong 2016-07-27 10:41 ` [tip:perf/urgent] perf s390: Fix " tip-bot for Song Shan Gong -- strict thread matches above, loose matches on Subject: below -- 2016-07-19 11:31 [RFC PATCH V3]s390/perf:fix " Song Shan Gong 2016-07-19 11:31 ` [PATCH] s390/perf: fix " Song Shan Gong 2016-07-20 11:51 ` Jiri Olsa
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.