* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-07 1:49 [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Song Shan Gong
@ 2016-07-08 2:17 ` Songshan Gong
2016-07-08 15:20 ` Jiri Olsa
2016-07-08 15:18 ` Jiri Olsa
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Songshan Gong @ 2016-07-08 2:17 UTC (permalink / raw)
To: acme, jolsa; +Cc: dsahern, linux-kernel
在 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-08 2:17 ` Songshan Gong
@ 2016-07-08 15:20 ` Jiri Olsa
0 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2016-07-08 15:20 UTC (permalink / raw)
To: Songshan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-07 1:49 [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Song Shan Gong
2016-07-08 2:17 ` Songshan Gong
@ 2016-07-08 15:18 ` Jiri Olsa
2016-07-11 8:11 ` Songshan Gong
2016-07-08 15:20 ` Jiri Olsa
` (2 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2016-07-08 15:18 UTC (permalink / raw)
To: Song Shan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-08 15:18 ` Jiri Olsa
@ 2016-07-11 8:11 ` Songshan Gong
2016-07-13 3:32 ` Songshan Gong
0 siblings, 1 reply; 16+ messages in thread
From: Songshan Gong @ 2016-07-11 8:11 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel
在 7/8/2016 11:18 PM, Jiri Olsa 写道:
> 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've checked, 'perf test 1' is still failed. But the test is for testing
whether 'vmlinux symtab matches kallsyms'. For vmlinux, it's built when
compiling linux-kernel, so there's no any symbol info about module. For
kallsyms, when loading, it also splits kernel symbols with module
symbols. But this patch is intended to fix wrong symbol resolution for
modules, so I think there's no relationship between this patch and the
testcase 'perf test 1'.
I also debuged the reason why 'perf test 1' fails, there are two reasons
at least:
1. perf gets kernel start address by finding the first no-zero value of
symbols {'_text', '_stext'} in /proc/kallsyms; for s390, it's always
'_stext', but actually, '_stext' is not the first symbol in s390, there
are other symbols before '_stext', for example 'iplstart'.
2. In addition, when loading by parsing /proc/kallsyms, if the kernel
map start value is a non-zero value getting from '_stext', because
'_text' is zero, it will not be included in kernel map; but for vmlinux,
it always add '_text' to kernel map whether '_text' is zero or not. So
if '_text' is zero, whether adding '_text' to kernel map, it's
non-consistent for loading by /proc/kallsyms and vmlinux.
I'll try to fix this bug later.
Thanks for your comments.
> I'll send few coments shortly
>
> thanks,
> jirka
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-11 8:11 ` Songshan Gong
@ 2016-07-13 3:32 ` Songshan Gong
0 siblings, 0 replies; 16+ messages in thread
From: Songshan Gong @ 2016-07-13 3:32 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel
I send this email to test the connection to linux-kernel maillist. Just
ignore.
在 7/11/2016 4:11 PM, Songshan Gong 写道:
>
>
> 在 7/8/2016 11:18 PM, Jiri Olsa 写道:
>> 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've checked, 'perf test 1' is still failed. But the test is for testing
> whether 'vmlinux symtab matches kallsyms'. For vmlinux, it's built when
> compiling linux-kernel, so there's no any symbol info about module. For
> kallsyms, when loading, it also splits kernel symbols with module
> symbols. But this patch is intended to fix wrong symbol resolution for
> modules, so I think there's no relationship between this patch and the
> testcase 'perf test 1'.
>
> I also debuged the reason why 'perf test 1' fails, there are two reasons
> at least:
> 1. perf gets kernel start address by finding the first no-zero value of
> symbols {'_text', '_stext'} in /proc/kallsyms; for s390, it's always
> '_stext', but actually, '_stext' is not the first symbol in s390, there
> are other symbols before '_stext', for example 'iplstart'.
> 2. In addition, when loading by parsing /proc/kallsyms, if the kernel
> map start value is a non-zero value getting from '_stext', because
> '_text' is zero, it will not be included in kernel map; but for vmlinux,
> it always add '_text' to kernel map whether '_text' is zero or not. So
> if '_text' is zero, whether adding '_text' to kernel map, it's
> non-consistent for loading by /proc/kallsyms and vmlinux.
>
> I'll try to fix this bug later.
>
> Thanks for your comments.
>
>> I'll send few coments shortly
>>
>> thanks,
>> jirka
>>
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-07 1:49 [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Song Shan Gong
2016-07-08 2:17 ` Songshan Gong
2016-07-08 15:18 ` Jiri Olsa
@ 2016-07-08 15:20 ` Jiri Olsa
2016-07-08 15:21 ` Jiri Olsa
2016-07-08 15:21 ` Jiri Olsa
4 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2016-07-08 15:20 UTC (permalink / raw)
To: Song Shan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-07 1:49 [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Song Shan Gong
` (2 preceding siblings ...)
2016-07-08 15:20 ` Jiri Olsa
@ 2016-07-08 15:21 ` Jiri Olsa
2016-07-08 15:21 ` Jiri Olsa
4 siblings, 0 replies; 16+ messages in thread
From: Jiri Olsa @ 2016-07-08 15:21 UTC (permalink / raw)
To: Song Shan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-07 1:49 [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Song Shan Gong
` (3 preceding siblings ...)
2016-07-08 15:21 ` Jiri Olsa
@ 2016-07-08 15:21 ` Jiri Olsa
2016-07-11 11:06 ` Songshan Gong
4 siblings, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2016-07-08 15:21 UTC (permalink / raw)
To: Song Shan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-08 15:21 ` Jiri Olsa
@ 2016-07-11 11:06 ` Songshan Gong
2016-07-11 12:01 ` Jiri Olsa
0 siblings, 1 reply; 16+ messages in thread
From: Songshan Gong @ 2016-07-11 11:06 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel
在 7/8/2016 11:21 PM, Jiri Olsa 写道:
> 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
It seems infeasible.
Each value in /sys/module/[module name]/sections/.text is a string like
"0x000003ff8130078\n".
But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is
based on decimal.
Maybe you can introduce a new argument indicating the value is based on
hex or decimal, or binary?
> thanks,
> jirka
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-11 11:06 ` Songshan Gong
@ 2016-07-11 12:01 ` Jiri Olsa
2016-07-13 6:39 ` Songshan Gong
0 siblings, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2016-07-11 12:01 UTC (permalink / raw)
To: Songshan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
SNIP
> >
> > 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
>
> It seems infeasible.
> Each value in /sys/module/[module name]/sections/.text is a string like
> "0x000003ff8130078\n".
> But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
> on decimal.
>
> Maybe you can introduce a new argument indicating the value is based on hex
> or decimal, or binary?
yea we could specify it directly and add something like:
int filename__read_ull(const char *filename, unsigned long long *value, int base)
plus some other higher layer helpers..
but I wonder if we could use the base 0 (like in the attached patch),
the man page says it should be able to detect the base
we'd need to check all the current usage to make sure nothing gets broken
jirka
---
diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
index 08556cf2c70d..d18ae548468a 100644
--- a/tools/lib/api/fs/fs.c
+++ b/tools/lib/api/fs/fs.c
@@ -292,7 +292,7 @@ int filename__read_ull(const char *filename, unsigned long long *value)
return -1;
if (read(fd, line, sizeof(line)) > 0) {
- *value = strtoull(line, NULL, 10);
+ *value = strtoull(line, NULL, 0);
if (*value != ULLONG_MAX)
err = 0;
}
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-11 12:01 ` Jiri Olsa
@ 2016-07-13 6:39 ` Songshan Gong
2016-07-13 9:07 ` Jiri Olsa
0 siblings, 1 reply; 16+ messages in thread
From: Songshan Gong @ 2016-07-13 6:39 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel
在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
>
> SNIP
>
>>>
>>> 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
>>
>> It seems infeasible.
>> Each value in /sys/module/[module name]/sections/.text is a string like
>> "0x000003ff8130078\n".
>> But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
>> on decimal.
>>
>> Maybe you can introduce a new argument indicating the value is based on hex
>> or decimal, or binary?
>
> yea we could specify it directly and add something like:
>
> int filename__read_ull(const char *filename, unsigned long long *value, int base)
>
> plus some other higher layer helpers..
>
> but I wonder if we could use the base 0 (like in the attached patch),
> the man page says it should be able to detect the base
>
> we'd need to check all the current usage to make sure nothing gets broken
>
> jirka
>
>
Since your patch havn't pushed to devel branch, my next version patch
will still use the origin method to parse value from /sys/.
Thanks.
> ---
> diff --git a/tools/lib/api/fs/fs.c b/tools/lib/api/fs/fs.c
> index 08556cf2c70d..d18ae548468a 100644
> --- a/tools/lib/api/fs/fs.c
> +++ b/tools/lib/api/fs/fs.c
> @@ -292,7 +292,7 @@ int filename__read_ull(const char *filename, unsigned long long *value)
> return -1;
>
> if (read(fd, line, sizeof(line)) > 0) {
> - *value = strtoull(line, NULL, 10);
> + *value = strtoull(line, NULL, 0);
> if (*value != ULLONG_MAX)
> err = 0;
> }
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-13 6:39 ` Songshan Gong
@ 2016-07-13 9:07 ` Jiri Olsa
2016-07-15 7:45 ` Songshan Gong
0 siblings, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2016-07-13 9:07 UTC (permalink / raw)
To: Songshan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
>
>
> 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> > On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
> >
> > SNIP
> >
> > > >
> > > > 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
> > >
> > > It seems infeasible.
> > > Each value in /sys/module/[module name]/sections/.text is a string like
> > > "0x000003ff8130078\n".
> > > But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
> > > on decimal.
> > >
> > > Maybe you can introduce a new argument indicating the value is based on hex
> > > or decimal, or binary?
> >
> > yea we could specify it directly and add something like:
> >
> > int filename__read_ull(const char *filename, unsigned long long *value, int base)
> >
> > plus some other higher layer helpers..
> >
> > but I wonder if we could use the base 0 (like in the attached patch),
> > the man page says it should be able to detect the base
> >
> > we'd need to check all the current usage to make sure nothing gets broken
> >
> > jirka
> >
> >
>
> Since your patch havn't pushed to devel branch, my next version patch will
> still use the origin method to parse value from /sys/.
I'll make/send the change during this week,
jirka
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-13 9:07 ` Jiri Olsa
@ 2016-07-15 7:45 ` Songshan Gong
2016-07-15 8:27 ` Jiri Olsa
0 siblings, 1 reply; 16+ messages in thread
From: Songshan Gong @ 2016-07-15 7:45 UTC (permalink / raw)
To: Jiri Olsa; +Cc: acme, jolsa, dsahern, linux-kernel
在 7/13/2016 5:07 PM, Jiri Olsa 写道:
> On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
>>
>>
>> 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
>>> On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
>>>
>>> SNIP
>>>
>>>>>
>>>>> 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
>>>>
>>>> It seems infeasible.
>>>> Each value in /sys/module/[module name]/sections/.text is a string like
>>>> "0x000003ff8130078\n".
>>>> But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
>>>> on decimal.
>>>>
>>>> Maybe you can introduce a new argument indicating the value is based on hex
>>>> or decimal, or binary?
>>>
>>> yea we could specify it directly and add something like:
>>>
>>> int filename__read_ull(const char *filename, unsigned long long *value, int base)
>>>
>>> plus some other higher layer helpers..
>>>
>>> but I wonder if we could use the base 0 (like in the attached patch),
>>> the man page says it should be able to detect the base
>>>
>>> we'd need to check all the current usage to make sure nothing gets broken
>>>
>>> jirka
>>>
>>>
>>
>> Since your patch havn't pushed to devel branch, my next version patch will
>> still use the origin method to parse value from /sys/.
>
> I'll make/send the change during this week,
>
Oh, could you remind me after you've done?
Thanks a lot.
Song Shan Gong
> jirka
>
--
SongShan Gong
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-15 7:45 ` Songshan Gong
@ 2016-07-15 8:27 ` Jiri Olsa
2016-07-15 13:23 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 16+ messages in thread
From: Jiri Olsa @ 2016-07-15 8:27 UTC (permalink / raw)
To: Songshan Gong; +Cc: acme, jolsa, dsahern, linux-kernel
On Fri, Jul 15, 2016 at 03:45:24PM +0800, Songshan Gong wrote:
>
>
> 在 7/13/2016 5:07 PM, Jiri Olsa 写道:
> > On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
> > >
> > >
> > > 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> > > > On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
> > > >
> > > > SNIP
> > > >
> > > > > >
> > > > > > 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
> > > > >
> > > > > It seems infeasible.
> > > > > Each value in /sys/module/[module name]/sections/.text is a string like
> > > > > "0x000003ff8130078\n".
> > > > > But the core function 'strtoull(line, NULL, 10)' in sysfs__read_ull is based
> > > > > on decimal.
> > > > >
> > > > > Maybe you can introduce a new argument indicating the value is based on hex
> > > > > or decimal, or binary?
> > > >
> > > > yea we could specify it directly and add something like:
> > > >
> > > > int filename__read_ull(const char *filename, unsigned long long *value, int base)
> > > >
> > > > plus some other higher layer helpers..
> > > >
> > > > but I wonder if we could use the base 0 (like in the attached patch),
> > > > the man page says it should be able to detect the base
> > > >
> > > > we'd need to check all the current usage to make sure nothing gets broken
> > > >
> > > > jirka
> > > >
> > > >
> > >
> > > Since your patch havn't pushed to devel branch, my next version patch will
> > > still use the origin method to parse value from /sys/.
> >
> > I'll make/send the change during this week,
> >
>
> Oh, could you remind me after you've done?
> Thanks a lot.
>
I posted it this morning, you're CC-ed
jirka
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
2016-07-15 8:27 ` Jiri Olsa
@ 2016-07-15 13:23 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 16+ messages in thread
From: Arnaldo Carvalho de Melo @ 2016-07-15 13:23 UTC (permalink / raw)
To: Songshan Gong; +Cc: Jiri Olsa, jolsa, dsahern, linux-kernel
Em Fri, Jul 15, 2016 at 10:27:43AM +0200, Jiri Olsa escreveu:
> On Fri, Jul 15, 2016 at 03:45:24PM +0800, Songshan Gong wrote:
> > 在 7/13/2016 5:07 PM, Jiri Olsa 写道:
> > > On Wed, Jul 13, 2016 at 02:39:13PM +0800, Songshan Gong wrote:
> > > > 在 7/11/2016 8:01 PM, Jiri Olsa 写道:
> > > > > On Mon, Jul 11, 2016 at 07:06:14PM +0800, Songshan Gong wrote:
> > > > Since your patch havn't pushed to devel branch, my next version patch will
> > > > still use the origin method to parse value from /sys/.
> > >
> > > I'll make/send the change during this week,
> > >
> >
> > Oh, could you remind me after you've done?
> > Thanks a lot.
> >
>
> I posted it this morning, you're CC-ed
I have just merged it, pushing now to acme/perf/core, thanks!
- Arnaldo
^ permalink raw reply [flat|nested] 16+ messages in thread