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

Groups > linux.kernel > #1438118 > unrolled thread

[PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

Started bySong Shan Gong <gongss@linux.vnet.ibm.com>
First post2016-07-07 03:50 +0200
Last post2016-07-08 17:30 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Song Shan Gong <gongss@linux.vnet.ibm.com> - 2016-07-07 03:50 +0200
    Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Songshan Gong <gongss@linux.vnet.ibm.com> - 2016-07-08 04:20 +0200
      Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-08 17:30 +0200
    Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-08 17:20 +0200
    Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-08 17:30 +0200
    Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-08 17:30 +0200
    Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-08 17:30 +0200

#1438118 — [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map

FromSong Shan Gong <gongss@linux.vnet.ibm.com>
Date2016-07-07 03:50 +0200
Subject[PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
Message-ID<rS6BX-av-3@gated-at.bofh.it>
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 | 49 ++++++++++++++++++++++++++++++++
 tools/perf/util/machine.c                |  6 ++++
 tools/perf/util/machine.h                |  2 ++
 4 files changed, 59 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..efe2a50
--- /dev/null
+++ b/tools/perf/arch/s390/util/sym-handling.c
@@ -0,0 +1,49 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include "symbol.h"
+#include "map.h"
+#include "util.h"
+#include "machine.h"
+
+int arch__fix_module_baseaddr(struct machine *machine,
+		u64 *start, const char *name)
+{
+	char path[PATH_MAX];
+	char *module_name = strdup(name);
+	int len = strlen(module_name);
+	FILE *file;
+	int err = 0;
+	u64 text_start;
+	char *line = NULL;
+	size_t n;
+	char *sep;
+
+	module_name[len - 1] = '\0';
+	module_name += 1;
+	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
+				machine->root_dir, module_name);
+	file = fopen(path, "r");
+	if (file == NULL)
+		return -1;
+
+	len = getline(&line, &n, file);
+	if (len < 0) {
+		err = -1;
+		goto out;
+	}
+	line[--len] = '\0'; /* \n */
+	sep = strrchr(line, 'x');
+	if (sep == NULL) {
+		err = -1;
+		goto out;
+	}
+	hex2u64(sep + 1, &text_start);
+
+	*start = text_start;
+out:
+	free(line);
+	fclose(file);
+	free(module_name - 1);
+	return err;
+}
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b177218..e5c2721 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1091,12 +1091,18 @@ 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_baseaddr(struct machine *machine __maybe_unused,
+				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;
 
+	arch__fix_module_baseaddr(machine, &start, name);
 	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..da7b6c0 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -216,6 +216,8 @@ 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_baseaddr(struct machine *machine, 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

[toc] | [next] | [standalone]

#1439067

FromSongshan Gong <gongss@linux.vnet.ibm.com>
Date2016-07-08 04:20 +0200
Message-ID<rStyx-6V1-7@gated-at.bofh.it>
In reply to#1438118

在 7/7/2016 9:49 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>
> ---
>  tools/perf/arch/s390/util/Build          |  2 ++
>  tools/perf/arch/s390/util/sym-handling.c | 49 ++++++++++++++++++++++++++++++++
>  tools/perf/util/machine.c                |  6 ++++
>  tools/perf/util/machine.h                |  2 ++
>  4 files changed, 59 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..efe2a50
> --- /dev/null
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -0,0 +1,49 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "symbol.h"
> +#include "map.h"
> +#include "util.h"
> +#include "machine.h"
> +
> +int arch__fix_module_baseaddr(struct machine *machine,
> +		u64 *start, const char *name)
> +{
> +	char path[PATH_MAX];
> +	char *module_name = strdup(name);
> +	int len = strlen(module_name);
> +	FILE *file;
> +	int err = 0;
> +	u64 text_start;
> +	char *line = NULL;
> +	size_t n;
> +	char *sep;
> +
> +	module_name[len - 1] = '\0';
> +	module_name += 1;
> +	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
> +				machine->root_dir, module_name);
> +	file = fopen(path, "r");
> +	if (file == NULL)
> +		return -1;
> +
> +	len = getline(&line, &n, file);
> +	if (len < 0) {
> +		err = -1;
> +		goto out;
> +	}
> +	line[--len] = '\0'; /* \n */
> +	sep = strrchr(line, 'x');
> +	if (sep == NULL) {
> +		err = -1;
> +		goto out;
> +	}
> +	hex2u64(sep + 1, &text_start);
> +
> +	*start = text_start;
> +out:
> +	free(line);
> +	fclose(file);
> +	free(module_name - 1);
> +	return err;
> +}
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b177218..e5c2721 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1091,12 +1091,18 @@ 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_baseaddr(struct machine *machine __maybe_unused,
> +				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;
>
> +	arch__fix_module_baseaddr(machine, &start, name);

As the description says, I would change the function name to 
'arch__fix_module_text_start';

>  	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..da7b6c0 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -216,6 +216,8 @@ 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_baseaddr(struct machine *machine, 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

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

#1439578

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-08 17:30 +0200
Message-ID<rSFT4-6xS-27@gated-at.bofh.it>
In reply to#1439067
On Fri, Jul 08, 2016 at 10:17:30AM +0800, Songshan Gong wrote:

SNIP

> > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> > index b177218..e5c2721 100644
> > --- a/tools/perf/util/machine.c
> > +++ b/tools/perf/util/machine.c
> > @@ -1091,12 +1091,18 @@ 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_baseaddr(struct machine *machine __maybe_unused,
> > +				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;
> > 
> > +	arch__fix_module_baseaddr(machine, &start, name);
> 
> As the description says, I would change the function name to
> 'arch__fix_module_text_start';

both of them sound good to me

jirka

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

#1439561

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-08 17:20 +0200
Message-ID<rSFJn-6un-11@gated-at.bofh.it>
In reply to#1438118
On Thu, Jul 07, 2016 at 09:49:36AM +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'.

cool, does this fix the 'perf test 1' for s390? that'd be great

I'll send few coments shortly

thanks,
jirka

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

#1439570

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-08 17:30 +0200
Message-ID<rSFT4-6xS-11@gated-at.bofh.it>
In reply to#1438118
On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:

SNIP

> +	char *line = NULL;
> +	size_t n;
> +	char *sep;
> +
> +	module_name[len - 1] = '\0';
> +	module_name += 1;
> +	snprintf(path, PATH_MAX, "%s/sys/module/%s/sections/.text",
> +				machine->root_dir, module_name);
> +	file = fopen(path, "r");
> +	if (file == NULL)
> +		return -1;
> +
> +	len = getline(&line, &n, file);
> +	if (len < 0) {
> +		err = -1;
> +		goto out;
> +	}
> +	line[--len] = '\0'; /* \n */
> +	sep = strrchr(line, 'x');
> +	if (sep == NULL) {
> +		err = -1;
> +		goto out;
> +	}
> +	hex2u64(sep + 1, &text_start);

we have following functions in tools/lib/api/fs to read
single number from file, which I assume you do above:

int sysfs__read_int(const char *entry, int *value);
int sysfs__read_ull(const char *entry, unsigned long long *value);

please check if you could use some of them,
we could add some more generic one if needed

thanks,
jirka

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

#1439572

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-08 17:30 +0200
Message-ID<rSFT4-6xS-29@gated-at.bofh.it>
In reply to#1438118
On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:

SNIP

> +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..efe2a50
> --- /dev/null
> +++ b/tools/perf/arch/s390/util/sym-handling.c
> @@ -0,0 +1,49 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include "symbol.h"
> +#include "map.h"
> +#include "util.h"
> +#include "machine.h"
> +
> +int arch__fix_module_baseaddr(struct machine *machine,
> +		u64 *start, const char *name)
> +{
> +	char path[PATH_MAX];
> +	char *module_name = strdup(name);

we have a rule to check on every allocated pointer,
pelase check module_name != NULL before using it

> +	int len = strlen(module_name);
> +	FILE *file;
> +	int err = 0;
> +	u64 text_start;
> +	char *line = NULL;
> +	size_t n;
> +	char *sep;
> +
> +	module_name[len - 1] = '\0';
> +	module_name += 1;

hum, why do increase the pointer?

thanks,
jirka

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

#1439581

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-08 17:30 +0200
Message-ID<rSFT4-6xS-31@gated-at.bofh.it>
In reply to#1438118
On Thu, Jul 07, 2016 at 09:49:36AM +0800, Song Shan Gong wrote:

SNIP

> +	hex2u64(sep + 1, &text_start);
> +
> +	*start = text_start;
> +out:
> +	free(line);
> +	fclose(file);
> +	free(module_name - 1);
> +	return err;
> +}
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b177218..e5c2721 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1091,12 +1091,18 @@ 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_baseaddr(struct machine *machine __maybe_unused,
> +				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;
>  
> +	arch__fix_module_baseaddr(machine, &start, name);

you should check for its return value and fail if needed

jirka

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web