Skip to contentSkip to conversation

perf report: Fix some option handling on --stdio

PATCH In review Namhyung Kim 3 participants 4 messages May 13, 2015 – May 15, 2015 perf events

lore
Share & tools
lore

Namhyung Kim

author maintainer
#0 lore raw

PATCH 1/1 perf report: Fix some option handling on --stdio

There's a bug that perf report sometimes ignore some options on --stdio output. This bug is triggered only if a related config variable is set. For example, let's assume we have a following config file.

  $ cat ~/.perfconfig
  [call-graph]
    print-type = graph
  [hist]
    percentage = absolute

Then, following perf config will not honor some options.

  $ perf record -ag sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.199 MB perf.data (77 samples) ]
  $ perf report -g none --stdio
  # To display the perf.data header info, please use --header/--header-only options.
  #
  # Samples: 77  of event 'cycles'
  # Event count (approx.): 25425383
  #
  # Overhead  Command          Shared Object            Symbol
  # ........  ...............  .......................  ..............
  #
      16.34%  swapper          [kernel.vmlinux]         [k] intel_idle
                      |
                      ---intel_idle
                         cpuidle_enter_state
                         cpuidle_enter
                         cpu_startup_entry
   ...

With '-g none' option, it should not show callchains, but it still shows callchains. However it works as expected on --tui output.

Similarly, '--percentage relative' option is not work and still shows a absolute percentage values.

Looking at the source, I found that those setting were overwritten by config variables when setup_pager() called. The setup_pager() is to start a pager process so that it can manage long lines of output on the stdio mode. But as it calls the perf_config() after parsing arguments, the settings were overwritten regardless of command line options.

The reason it calls perf_config() is to find the 'pager_program' which might be set by a config variable, I guess. However current perf code does not provide the config variable for it, so it's just meaningless IMHO. Eliminating the call makes the option working as expected.

---
 tools/perf/util/cache.h       | 1 -
 tools/perf/util/environment.c | 1 -
 tools/perf/util/pager.c       | 5 -----
 3 files changed, 7 deletions(-)
tools/perf/util/cache.h +0 -1
index fbcca21d66ab..c861373aaed3 100644
@@ -30,7 +30,6 @@ extern const char *perf_config_dirname(const char *, const char *);
 
 /* pager.c */
 extern void setup_pager(void);
-extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
 
tools/perf/util/environment.c +0 -1
index 275b0ee345f5..7405123692f1 100644
@@ -5,5 +5,4 @@
  */
 #include "cache.h"
 
-const char *pager_program;
 int pager_use_color = 1;
tools/perf/util/pager.c +0 -5
index 31ee02d4e988..53ef006a951c 100644
@@ -50,11 +50,6 @@ void setup_pager(void)
 
 	if (!isatty(1))
 		return;
-	if (!pager) {
-		if (!pager_program)
-			perf_config(perf_default_config, NULL);
-		pager = pager_program;
-	}
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!(pager || access("/usr/bin/pager", X_OK)))
Signature · 1 line
2.4.0
#1 lore raw

Em Thu, May 14, 2015 at 12:03:26AM +0900, Namhyung Kim escreveu:

Quoted 3 lines

There's a bug that perf report sometimes ignore some options on --stdio output. This bug is triggered only if a related config variable is set. For example, let's assume we have a following config file.

Testing, please next time indicate against what branch this is to be applied, as I thought about perf/core, but it doesn't apply there, so tried perf/urgent, where it applies.

- Arnaldo

Quoted 94 lines
  $ cat ~/.perfconfig
  [call-graph]
    print-type = graph
  [hist]
    percentage = absolute

Then, following perf config will not honor some options.

  $ perf record -ag sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.199 MB perf.data (77 samples) ]
  $ perf report -g none --stdio
  # To display the perf.data header info, please use --header/--header-only options.
  #
  # Samples: 77  of event 'cycles'
  # Event count (approx.): 25425383
  #
  # Overhead  Command          Shared Object            Symbol
  # ........  ...............  .......................  ..............
  #
      16.34%  swapper          [kernel.vmlinux]         [k] intel_idle
                      |
                      ---intel_idle
                         cpuidle_enter_state
                         cpuidle_enter
                         cpu_startup_entry
   ...

With '-g none' option, it should not show callchains, but it still shows callchains. However it works as expected on --tui output.

Similarly, '--percentage relative' option is not work and still shows a absolute percentage values.

Looking at the source, I found that those setting were overwritten by config variables when setup_pager() called. The setup_pager() is to start a pager process so that it can manage long lines of output on the stdio mode. But as it calls the perf_config() after parsing arguments, the settings were overwritten regardless of command line options.

The reason it calls perf_config() is to find the 'pager_program' which might be set by a config variable, I guess. However current perf code does not provide the config variable for it, so it's just meaningless IMHO. Eliminating the call makes the option working as expected.

---
 tools/perf/util/cache.h       | 1 -
 tools/perf/util/environment.c | 1 -
 tools/perf/util/pager.c       | 5 -----
 3 files changed, 7 deletions(-)
tools/perf/util/cache.h +0 -1
index fbcca21d66ab..c861373aaed3 100644
@@ -30,7 +30,6 @@ extern const char *perf_config_dirname(const char *, const char *);
 
 /* pager.c */
 extern void setup_pager(void);
-extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
 
tools/perf/util/environment.c +0 -1
index 275b0ee345f5..7405123692f1 100644
@@ -5,5 +5,4 @@
  */
 #include "cache.h"
 
-const char *pager_program;
 int pager_use_color = 1;
tools/perf/util/pager.c +0 -5
index 31ee02d4e988..53ef006a951c 100644
@@ -50,11 +50,6 @@ void setup_pager(void)
 
 	if (!isatty(1))
 		return;
-	if (!pager) {
-		if (!pager_program)
-			perf_config(perf_default_config, NULL);
-		pager = pager_program;
-	}
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!(pager || access("/usr/bin/pager", X_OK)))
Signature · 1 line
2.4.0
author maintainer
#2 lore raw

Hi Arnaldo,

Quoted 8 lines · Arnaldo Carvalho de Melo

Em Thu, May 14, 2015 at 12:03:26AM +0900, Namhyung Kim escreveu:

Quoted 3 lines

There's a bug that perf report sometimes ignore some options on --stdio output. This bug is triggered only if a related config variable is set. For example, let's assume we have a following config file.

Testing, please next time indicate against what branch this is to be applied, as I thought about perf/core, but it doesn't apply there, so tried perf/urgent, where it applies.

Oh, sorry about that. I actually intended to apply it to the perf/core but forgot to rebase to the current code. It's fortunate that it can be applied to perf/urgent cleanly though. :)

I'll add an indication next time if patch should go to other branch than perf/core.

Thanks,
Namhyung

#3 lore raw
In reply to Namhyung Kim
perf report: Fix some option handling on --stdio
Commit-ID:  4fd113b5ce803da0b8fa0494513bedfdf2feb483
Gitweb:     http://git.kernel.org/tip/4fd113b5ce803da0b8fa0494513bedfdf2feb483
Author:     Namhyung Kim <[email protected]>
AuthorDate: Thu, 14 May 2015 00:03:26 +0900
Committer:  Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Thu, 14 May 2015 10:05:22 -0300

perf report: Fix some option handling on --stdio

There's a bug that perf report sometimes ignore some options on --stdio output. This bug is triggered only if a related config variable is set. For example, let's assume we have a following config file.

  $ cat ~/.perfconfig
  [call-graph]
    print-type = graph
  [hist]
    percentage = absolute

Then, following perf config will not honor some options.

  $ perf record -ag sleep 1
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.199 MB perf.data (77 samples) ]
  $ perf report -g none --stdio
  # To display the perf.data header info, please use --header/--header-only options.
  #
  # Samples: 77  of event 'cycles'
  # Event count (approx.): 25425383
  #
  # Overhead  Command          Shared Object            Symbol
  # ........  ...............  .......................  ..............
  #
      16.34%  swapper          [kernel.vmlinux]         [k] intel_idle
                      |
                      ---intel_idle
                         cpuidle_enter_state
                         cpuidle_enter
                         cpu_startup_entry
   ...

With '-g none' option, it should not show callchains, but it still shows callchains. However it works as expected on --tui output.

Similarly, '--percentage relative' option is not work and still shows a absolute percentage values.

Looking at the source, I found that those setting were overwritten by config variables when setup_pager() called. The setup_pager() is to start a pager process so that it can manage long lines of output on the stdio mode. But as it calls the perf_config() after parsing arguments, the settings were overwritten regardless of command line options.

The reason it calls perf_config() is to find the 'pager_program' which might be set by a config variable, I guess. However current perf code does not provide the config variable for it, so it's just meaningless IMHO. Eliminating the call makes the option working as expected.

---
 tools/perf/util/cache.h       | 1 -
 tools/perf/util/environment.c | 1 -
 tools/perf/util/pager.c       | 5 -----
 3 files changed, 7 deletions(-)
tools/perf/util/cache.h +0 -1
index fbcca21..c861373 100644
@@ -30,7 +30,6 @@ extern const char *perf_config_dirname(const char *, const char *);
 
 /* pager.c */
 extern void setup_pager(void);
-extern const char *pager_program;
 extern int pager_in_use(void);
 extern int pager_use_color;
 
tools/perf/util/environment.c +0 -1
index 275b0ee..7405123 100644
@@ -5,5 +5,4 @@
  */
 #include "cache.h"
 
-const char *pager_program;
 int pager_use_color = 1;
tools/perf/util/pager.c +0 -5
index 31ee02d..53ef006 100644
@@ -50,11 +50,6 @@ void setup_pager(void)
 
 	if (!isatty(1))
 		return;
-	if (!pager) {
-		if (!pager_program)
-			perf_config(perf_default_config, NULL);
-		pager = pager_program;
-	}
 	if (!pager)
 		pager = getenv("PAGER");
 	if (!(pager || access("/usr/bin/pager", X_OK)))