perf tools: Add +field argument support for --field/--sort options

Adding support to add field(s) to default sort order via using the '+' prefix, like for report:

  $ perf report
  Samples: 2K of event 'cycles', Event count (approx.): 882172583
  Overhead  Command          Shared Object                Symbol
     7.39%  swapper          [kernel.kallsyms]            [k] intel_idle
     1.97%  firefox          libpthread-2.17.so           [.] pthread_mutex_lock
     1.39%  firefox          [snd_hda_intel]              [k] azx_get_position
     1.11%  firefox          libpthread-2.17.so           [.] pthread_mutex_unlock

  $ perf report -s +cpu
  Samples: 2K of event 'cycles', Event count (approx.): 882172583
  Overhead  Command          Shared Object                Symbol                      CPU
     2.89%  swapper          [kernel.kallsyms]            [k] intel_idle              000
     2.61%  swapper          [kernel.kallsyms]            [k] intel_idle              002
     1.20%  swapper          [kernel.kallsyms]            [k] intel_idle              001
     0.82%  firefox          libpthread-2.17.so           [.] pthread_mutex_lock      002

Works in general for commands using --sort option.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/sort.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

tools/perf/util/sort.c +0 -0

index 1958637cf136..b3d7dc1837ec 100644

tools/perf/util/sort.c +5 -0

+	static char buf[BUF_MAX];
+
+	if (!sort_order || is_strict_order(sort_order))
+		return 0;
+

tools/perf/util/sort.c +8 -1

+#undef BUF_MAX
+}
+
 static int __setup_sorting(void)
 {
 	char *tmp, *tok, *str;
-	const char *sort_keys = sort_order;
+	const char *sort_keys;
 	int ret = 0;
 
+	if (setup_sort_order())
+		return -EINVAL;
+
+	sort_keys = sort_order;
 	if (sort_keys == NULL) {
 		if (is_strict_order(field_order)) {
 			/*

Signature · 1 line

1.8.3.1

tools/perf/util/sort.c static const char *get_default_sort_order(void) · line 1451

Quoted 5 lines

}

tools/perf/util/sort.c +3 -0

+static int setup_sort_order(void)
+{
+#define BUF_MAX 4096

This is an arbitrary value, and you will use this just here, so you could just have used a number and later used sizeof(buf).

Anyway, what bothers me is the use of yet another static buffer.

The thing to use here is asprintf, that will do it all for you, format _and_ allocate a buffer the size you need.

Quoted 5 lines

tools/perf/util/sort.c +5 -0

+	static char buf[BUF_MAX];
+
+	if (!sort_order || is_strict_order(sort_order))
+		return 0;
+

tools/perf/util/sort.c static const char *get_default_sort_order(void) · line 1466

Quoted 10 lines

tools/perf/util/sort.c +10 -0

+	if (!strlen(sort_order + 1)) {
+		error("Invalid --fields key: `+'");
+		return -EINVAL;
+	}
+
+	scnprintf(buf, BUF_MAX, "%s,%s",
+		  get_default_sort_order(),
+		  sort_order + 1);
+
+	sort_order = buf;

I.e. it would be better to have this as:

	char *new_sort_order;

	if (sort_order[1] == '\0') {
		error("Invalid --fields key: `+'");
		return -EINVAL;
	}

	if (asprintf(&new_sort_order, "%s,%s",
		     get_default_sort_order(), sort_order + 1) < 0) {
		error("Not enough memory to set up --sort");
		return -ENOMEM;
	}

		
	sort_order = new_sort_order;

Review

Quoted 1 line

(quoted diff) +1 -0

+	return 0;

Also please fix the error message, there is a cut'n'paste error there :-)

On Fri, Aug 22, 2014 at 12:16:51PM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

Quoted 32 lines

Review

Quoted 5 lines

}

(quoted diff) +3 -0

+static int setup_sort_order(void)
+{
+#define BUF_MAX 4096

This is an arbitrary value, and you will use this just here, so you could just have used a number and later used sizeof(buf).

Anyway, what bothers me is the use of yet another static buffer.

The thing to use here is asprintf, that will do it all for you, format _and_ allocate a buffer the size you need.

Quoted 5 lines

(quoted diff) +5 -0

+	static char buf[BUF_MAX];
+
+	if (!sort_order || is_strict_order(sort_order))
+		return 0;
+

Review

Quoted 10 lines

(quoted diff) +10 -0

+	if (!strlen(sort_order + 1)) {
+		error("Invalid --fields key: `+'");
+		return -EINVAL;
+	}
+
+	scnprintf(buf, BUF_MAX, "%s,%s",
+		  get_default_sort_order(),
+		  sort_order + 1);
+
+	sort_order = buf;

I.e. it would be better to have this as:

Quoted 12 lines

	char *new_sort_order;

	if (sort_order[1] == '\0') {
		error("Invalid --fields key: `+'");
		return -EINVAL;
	}

	if (asprintf(&new_sort_order, "%s,%s",
		     get_default_sort_order(), sort_order + 1) < 0) {
		error("Not enough memory to set up --sort");
		return -ENOMEM;
	}

ok, will check the asprintf

Quoted 3 lines

		
	sort_order = new_sort_order;

Quoted 4 lines

Review

Quoted 1 line

(quoted diff) +1 -0

+	return 0;

Also please fix the error message, there is a cut'n'paste error there :-)

aargh right ;-)

I'll send v2

Adding support to add field(s) to default sort order via using the '+' prefix, like for report:

  $ perf report
  Samples: 2K of event 'cycles', Event count (approx.): 882172583
  Overhead  Command          Shared Object                Symbol
     7.39%  swapper          [kernel.kallsyms]            [k] intel_idle
     1.97%  firefox          libpthread-2.17.so           [.] pthread_mutex_lock
     1.39%  firefox          [snd_hda_intel]              [k] azx_get_position
     1.11%  firefox          libpthread-2.17.so           [.] pthread_mutex_unlock

  $ perf report -s +cpu
  Samples: 2K of event 'cycles', Event count (approx.): 882172583
  Overhead  Command          Shared Object                Symbol                      CPU
     2.89%  swapper          [kernel.kallsyms]            [k] intel_idle              000
     2.61%  swapper          [kernel.kallsyms]            [k] intel_idle              002
     1.20%  swapper          [kernel.kallsyms]            [k] intel_idle              001
     0.82%  firefox          libpthread-2.17.so           [.] pthread_mutex_lock      002

Works in general for commands using --sort option.

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

hi,
adding support to add field(s) to default field and sort orders via using the '+' prefix, like for report:

  $ perf report -F +period,sample
  $ perf report -s +cpu

thanks,
jirka

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Corey Ashford <[email protected]>

Cc David Ahern <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jean Pihet <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Jiri Olsa <[email protected]>

---
Jiri Olsa (2):
perf tools: Add +field argument support for --field option perf tools: Add +field argument support for --sort option

 tools/perf/ui/hist.c   |  4 ++--
 tools/perf/util/sort.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++------
 tools/perf/util/sort.h |  1 +
 3 files changed, 49 insertions(+), 8 deletions(-)