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

Groups > linux.kernel > #1350824 > unrolled thread

[tip:perf/core] perf tools: Fix locale handling in pmu parsing

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

  [tip:perf/core] perf tools: Fix locale handling in pmu parsing tip-bot for Jiri Olsa <tipbot@zytor.com> - 2016-03-05 09:20 +0100
    Re: [tip:perf/core] perf tools: Fix locale handling in pmu parsing Ingo Molnar <mingo@kernel.org> - 2016-03-08 14:30 +0100
      [PATCH][perf/core] perf tools: Omit unnecessary cast in  perf_pmu__parse_scale Jiri Olsa <jolsa@redhat.com> - 2016-03-08 19:50 +0100
        Re: [PATCH][perf/core] perf tools: Omit unnecessary cast in  perf_pmu__parse_scale Arnaldo Carvalho de Melo <acme@redhat.com> - 2016-03-09 14:50 +0100
        [tip:perf/core] perf tools: Omit unnecessary cast in  perf_pmu__parse_scale tip-bot for Jiri Olsa <tipbot@zytor.com> - 2016-03-11 09:50 +0100

#1350824 — [tip:perf/core] perf tools: Fix locale handling in pmu parsing

Commit-ID:  f9a5978ac4ede901fa73d7c28ae1c5d89bc2a46a
Gitweb:     http://git.kernel.org/tip/f9a5978ac4ede901fa73d7c28ae1c5d89bc2a46a
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Thu, 3 Mar 2016 10:53:48 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Thu, 3 Mar 2016 11:04:54 -0300

perf tools: Fix locale handling in pmu parsing

Ingo reported regression on display format of big numbers, which is
missing separators (in default perf stat output).

 triton:~/tip> perf stat -a sleep 1
         ...
         127008602      cycles                    #    0.011 GHz
         279538533      stalled-cycles-frontend   #  220.09% frontend cycles idle
         119213269      instructions              #    0.94  insn per cycle

This is caused by recent change:

  perf stat: Check existence of frontend/backed stalled cycles

that added call to pmu_have_event, that subsequently calls
perf_pmu__parse_scale, which has a bug in locale handling.

The lc string returned from setlocale, that we use to store old locale
value, may be allocated in static storage. Getting a dynamic copy to
make it survive another setlocale call.

  $ perf stat ls
         ...
         2,360,602      cycles                    #    3.080 GHz
         2,703,090      instructions              #    1.15  insn per cycle
           546,031      branches                  #  712.511 M/sec

Committer note:

Since the patch introducing the regression didn't made to perf/core,
move it to just before where the regression was introduced, so that we
don't break bisection for this feature.

Reported-by: Ingo Molnar <mingo@redhat.com>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20160303095348.GA24511@krava.redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/pmu.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index ce61f79..d8cd038 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -124,6 +124,17 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	lc = setlocale(LC_NUMERIC, NULL);
 
 	/*
+	 * The lc string may be allocated in static storage,
+	 * so get a dynamic copy to make it survive setlocale
+	 * call below.
+	 */
+	lc = strdup(lc);
+	if (!lc) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	/*
 	 * force to C locale to ensure kernel
 	 * scale string is converted correctly.
 	 * kernel uses default C locale.
@@ -135,6 +146,8 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	/* restore locale */
 	setlocale(LC_NUMERIC, lc);
 
+	free((char *) lc);
+
 	ret = 0;
 error:
 	close(fd);

[toc] | [next] | [standalone]

#1353037

* tip-bot for Jiri Olsa <tipbot@zytor.com> wrote:

> @@ -135,6 +146,8 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
>  	/* restore locale */
>  	setlocale(LC_NUMERIC, lc);
>  
> +	free((char *) lc);
> +

Btw., minor side note: why does 'lc' have to be case to 'char *'?

In the kernel kfree() takes 'const void *':

  include/linux/slab.h:void kfree(const void *);

which will accept all pointer types. That avoids unnecessary and fragile type 
casts.

Thanks,

	Ingo

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

#1353313 — [PATCH][perf/core] perf tools: Omit unnecessary cast in perf_pmu__parse_scale

On Tue, Mar 08, 2016 at 02:23:40PM +0100, Ingo Molnar wrote:
> 
> * tip-bot for Jiri Olsa <tipbot@zytor.com> wrote:
> 
> > @@ -135,6 +146,8 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
> >  	/* restore locale */
> >  	setlocale(LC_NUMERIC, lc);
> >  
> > +	free((char *) lc);
> > +
> 
> Btw., minor side note: why does 'lc' have to be case to 'char *'?
> 
> In the kernel kfree() takes 'const void *':
> 
>   include/linux/slab.h:void kfree(const void *);
> 
> which will accept all pointer types. That avoids unnecessary and fragile type 
> casts.

libc free takes only non const pointers:

util/pmu.c: In function ‘perf_pmu__parse_scale’:
util/pmu.c:149:7: error: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
  free(lc);
       ^
In file included from /home/jolsa/kernel/linux-perf/tools/include/linux/kernel.h:6:0,
                 from /home/jolsa/kernel/linux-perf/tools/include/linux/list.h:6,
                 from util/pmu.c:1:
/usr/include/stdlib.h:483:13: note: expected ‘void *’ but argument is of type ‘const char *’
 extern void free (void *__ptr) __THROW;


but we could actually make it char* from the beginning..
for some reason I thought setlocale returns const ptr,
and I did not check.. fix attached 

jirka


---
There's no need to used const char pointer,
we can used char pointer from the beginning
and omit unnecessary cast.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index d8cd038baed2..adef23b1352e 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -98,7 +98,7 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	char scale[128];
 	int fd, ret = -1;
 	char path[PATH_MAX];
-	const char *lc;
+	char *lc;
 
 	snprintf(path, PATH_MAX, "%s/%s.scale", dir, name);
 
@@ -146,7 +146,7 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	/* restore locale */
 	setlocale(LC_NUMERIC, lc);
 
-	free((char *) lc);
+	free(lc);
 
 	ret = 0;
 error:
-- 
2.4.3

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

#1354132 — Re: [PATCH][perf/core] perf tools: Omit unnecessary cast in perf_pmu__parse_scale

Em Tue, Mar 08, 2016 at 07:42:30PM +0100, Jiri Olsa escreveu:
> but we could actually make it char* from the beginning..
> for some reason I thought setlocale returns const ptr,
> and I did not check.. fix attached 

Thanks, applied.

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

#1355698 — [tip:perf/core] perf tools: Omit unnecessary cast in perf_pmu__parse_scale

Commit-ID:  ea8f75f981918c5946fc4029acdc86707fa901c1
Gitweb:     http://git.kernel.org/tip/ea8f75f981918c5946fc4029acdc86707fa901c1
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Tue, 8 Mar 2016 19:42:30 +0100
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 9 Mar 2016 10:42:22 -0300

perf tools: Omit unnecessary cast in perf_pmu__parse_scale

There's no need to use a const char pointer, we can used char pointer
from the beginning and omit the unnecessary cast.

Reported-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: David Ahern <dsahern@gmail.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20160308184230.GB7897@krava.redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/pmu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index d8cd038..adef23b 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -98,7 +98,7 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	char scale[128];
 	int fd, ret = -1;
 	char path[PATH_MAX];
-	const char *lc;
+	char *lc;
 
 	snprintf(path, PATH_MAX, "%s/%s.scale", dir, name);
 
@@ -146,7 +146,7 @@ static int perf_pmu__parse_scale(struct perf_pmu_alias *alias, char *dir, char *
 	/* restore locale */
 	setlocale(LC_NUMERIC, lc);
 
-	free((char *) lc);
+	free(lc);
 
 	ret = 0;
 error:

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web