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

Groups > linux.kernel > #1440483 > unrolled thread

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

Started bySongshan Gong <gongss@linux.vnet.ibm.com>
First post2016-07-11 13:10 +0200
Last post2016-07-15 15:30 +0200
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Songshan Gong <gongss@linux.vnet.ibm.com> - 2016-07-11 13:10 +0200
    Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-11 14:10 +0200
      Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Songshan Gong <gongss@linux.vnet.ibm.com> - 2016-07-13 08:50 +0200
        Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-13 11:10 +0200
          Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Songshan Gong <gongss@linux.vnet.ibm.com> - 2016-07-15 09:50 +0200
            Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Jiri Olsa <jolsa@redhat.com> - 2016-07-15 10:30 +0200
              Re: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map Arnaldo Carvalho de Melo <acme@kernel.org> - 2016-07-15 15:30 +0200

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

FromSongshan Gong <gongss@linux.vnet.ibm.com>
Date2016-07-11 13:10 +0200
SubjectRe: [PATCH] [RFC V1]s390/perf: fix 'start' address of module's map
Message-ID<rTHg5-6eK-1@gated-at.bofh.it>

在 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

[toc] | [next] | [standalone]

#1440515

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-11 14:10 +0200
Message-ID<rTIca-6PO-9@gated-at.bofh.it>
In reply to#1440483
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;
 	}

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

#1442032

FromSongshan Gong <gongss@linux.vnet.ibm.com>
Date2016-07-13 08:50 +0200
Message-ID<rUm9z-7Nq-1@gated-at.bofh.it>
In reply to#1440515

在 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

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

#1442245

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-13 11:10 +0200
Message-ID<rUol4-VT-21@gated-at.bofh.it>
In reply to#1442032
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

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

#1444018

FromSongshan Gong <gongss@linux.vnet.ibm.com>
Date2016-07-15 09:50 +0200
Message-ID<rV62L-47o-27@gated-at.bofh.it>
In reply to#1442245

在 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

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

#1444050

FromJiri Olsa <jolsa@redhat.com>
Date2016-07-15 10:30 +0200
Message-ID<rV6Fr-4zR-15@gated-at.bofh.it>
In reply to#1444018
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

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

#1444316

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2016-07-15 15:30 +0200
Message-ID<rVblM-7q0-17@gated-at.bofh.it>
In reply to#1444050
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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web