[PATCH 0/2] perf tools: Add +field argument support for --field/--sort options

[PATCH 0/2] perf tools: Add +field argument support for --field/--sort options

[Jiri Olsa]

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 <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
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(-)

[PATCH 1/2] perf tools: Add +field argument support for --field option

[Jiri Olsa]

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

  $ perf report
  Samples: 10  of event 'cycles', Event count (approx.): 4463799
  Overhead  Command  Shared Object      Symbol
    32.40%  ls       [kernel.kallsyms]  [k] filemap_fault
    28.19%  ls       [kernel.kallsyms]  [k] get_page_from_freelist
    23.38%  ls       [kernel.kallsyms]  [k] enqueue_entity
    15.04%  ls       [kernel.kallsyms]  [k] mmap_region

  $ perf report -F +period,sample
  Samples: 10  of event 'cycles', Event count (approx.): 4463799
  Overhead        Period       Samples  Command  Shared Object      Symbol
    32.40%       1446493             1  ls       [kernel.kallsyms]  [k] filemap_fault
    28.19%       1258486             1  ls       [kernel.kallsyms]  [k] get_page_from_freelist
    23.38%       1043754             1  ls       [kernel.kallsyms]  [k] enqueue_entity
    15.04%        671160             1  ls       [kernel.kallsyms]  [k] mmap_region

Works in general for commands using --field option.

Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/ui/hist.c   |  4 ++--
 tools/perf/util/sort.c | 24 +++++++++++++++++++-----
 tools/perf/util/sort.h |  1 +
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 75eb6ac821f8..2af18376b077 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -452,7 +452,7 @@ void perf_hpp__init(void)
 	/*
 	 * If user specified field order, no need to setup default fields.
 	 */
-	if (field_order)
+	if (is_strict_order(field_order))
 		return;
 
 	if (symbol_conf.cumulate_callchain) {
@@ -519,7 +519,7 @@ void perf_hpp__column_disable(unsigned col)
 
 void perf_hpp__cancel_cumulate(void)
 {
-	if (field_order)
+	if (is_strict_order(field_order))
 		return;
 
 	perf_hpp__column_disable(PERF_HPP__OVERHEAD_ACC);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index b4a805e5e440..1958637cf136 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1453,7 +1453,7 @@ static int __setup_sorting(void)
 	int ret = 0;
 
 	if (sort_keys == NULL) {
-		if (field_order) {
+		if (is_strict_order(field_order)) {
 			/*
 			 * If user specified field order but no sort order,
 			 * we'll honor it and not add default sort orders.
@@ -1639,23 +1639,36 @@ static void reset_dimensions(void)
 		memory_sort_dimensions[i].taken = 0;
 }
 
+bool is_strict_order(const char *order)
+{
+	return order && (*order != '+');
+}
+
 static int __setup_output_field(void)
 {
-	char *tmp, *tok, *str;
-	int ret = 0;
+	char *tmp, *tok, *str, *strp;
+	int ret = -EINVAL;
 
 	if (field_order == NULL)
 		return 0;
 
 	reset_dimensions();
 
-	str = strdup(field_order);
+	strp = str = strdup(field_order);
 	if (str == NULL) {
 		error("Not enough memory to setup output fields");
 		return -ENOMEM;
 	}
 
-	for (tok = strtok_r(str, ", ", &tmp);
+	if (!is_strict_order(field_order))
+		strp++;
+
+	if (!strlen(strp)) {
+		error("Invalid --fields key: `+'");
+		goto out;
+	}
+
+	for (tok = strtok_r(strp, ", ", &tmp);
 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
 		ret = output_field_add(tok);
 		if (ret == -EINVAL) {
@@ -1667,6 +1680,7 @@ static int __setup_output_field(void)
 		}
 	}
 
+out:
 	free(str);
 	return ret;
 }
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 041f0c9cea2b..c03e4ff8beff 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -218,4 +218,5 @@ void perf_hpp__set_elide(int idx, bool elide);
 
 int report_parse_ignore_callees_opt(const struct option *opt, const char *arg, int unset);
 
+bool is_strict_order(const char *order);
 #endif	/* __PERF_SORT_H */
-- 
1.8.3.1

[PATCH 2/2] perf tools: Add +field argument support for --sort option

[Jiri Olsa]

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 <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/sort.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1958637cf136..b3d7dc1837ec 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1446,12 +1446,38 @@ static const char *get_default_sort_order(void)
 	return default_sort_orders[sort__mode];
 }
 
+static int setup_sort_order(void)
+{
+#define BUF_MAX 4096
+	static char buf[BUF_MAX];
+
+	if (!sort_order || is_strict_order(sort_order))
+		return 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;
+	return 0;
+#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)) {
 			/*
-- 
1.8.3.1

Re: [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options

[Arnaldo Carvalho de Melo]

Em Fri, Aug 22, 2014 at 03:58:37PM +0200, Jiri Olsa escreveu:
> 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

Really cool feature, (lightly) tested and applied,

Thanks,

- Arnaldo
 
> 
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> 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(-)

Re: [PATCH 2/2] perf tools: Add +field argument support for --sort option

[Arnaldo Carvalho de Melo]

Em Fri, Aug 22, 2014 at 03:58:39PM +0200, Jiri Olsa escreveu:
> 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 <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
>  tools/perf/util/sort.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 1958637cf136..b3d7dc1837ec 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -1446,12 +1446,38 @@ static const char *get_default_sort_order(void)
>  	return default_sort_orders[sort__mode];
>  }
>  
> +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.

> +	static char buf[BUF_MAX];
> +
> +	if (!sort_order || is_strict_order(sort_order))
> +		return 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;

> +	return 0;


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

I applied the --fields one.

- Arnaldo

> +#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)) {
>  			/*
> -- 
> 1.8.3.1

Re: [PATCH 2/2] perf tools: Add +field argument support for --sort option

[Jiri Olsa]

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

SNIP

> >  }
> >  
> > +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.
> 
> > +	static char buf[BUF_MAX];
> > +
> > +	if (!sort_order || is_strict_order(sort_order))
> > +		return 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;
> 	}

ok, will check the asprintf

> 		
> 	sort_order = new_sort_order;
> 
> > +	return 0;
> 
> 
> Also please fix the error message, there is a cut'n'paste error there :-)

aargh right ;-)

I'll send v2

thanks,
jirka

[PATCHv2] perf tools: Add +field argument support for --sort option

[Jiri Olsa]

On Fri, Aug 22, 2014 at 05:23:55PM +0200, Jiri Olsa wrote:
> On Fri, Aug 22, 2014 at 12:16:51PM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > >  }
> > >  
> > > +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.
> > 
> > > +	static char buf[BUF_MAX];
> > > +
> > > +	if (!sort_order || is_strict_order(sort_order))
> > > +		return 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;
> > 	}
> 
> ok, will check the asprintf
> 
> > 		
> > 	sort_order = new_sort_order;
> > 
> > > +	return 0;
> > 
> > 
> > Also please fix the error message, there is a cut'n'paste error there :-)
> 
> aargh right ;-)
> 
> I'll send v2

attached v2 with changes suggested above:
  - use dynamic memory instead static buffer
  - fix error message typo

thanks,
jirka


---
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 <acme@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/sort.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1958637cf136..289df9d1e65a 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1446,12 +1446,47 @@ static const char *get_default_sort_order(void)
 	return default_sort_orders[sort__mode];
 }
 
+static int setup_sort_order(void)
+{
+	char *new_sort_order;
+
+	/*
+	 * Append '+'-prefixed sort order to the default sort
+	 * order string.
+	 */
+	if (!sort_order || is_strict_order(sort_order))
+		return 0;
+
+	if (sort_order[1] == '\0') {
+		error("Invalid --sort key: `+'");
+		return -EINVAL;
+	}
+
+	/*
+	 * We allocate new sort_order string, but we never free it,
+	 * because it's checked over the rest of the code.
+	 */
+	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;
+	return 0;
+}
+
 static int __setup_sorting(void)
 {
 	char *tmp, *tok, *str;
-	const char *sort_keys = sort_order;
+	const char *sort_keys;
 	int ret = 0;
 
+	ret = setup_sort_order();
+	if (ret)
+		return ret;
+
+	sort_keys = sort_order;
 	if (sort_keys == NULL) {
 		if (is_strict_order(field_order)) {
 			/*
-- 
1.8.3.1

[tip:perf/core] perf tools: Add +field argument support for --field option

[tip-bot for Jiri Olsa]

Commit-ID:  2f3f9bcf000b2043a480e7cc0cae582559fb0f13
Gitweb:     http://git.kernel.org/tip/2f3f9bcf000b2043a480e7cc0cae582559fb0f13
Author:     Jiri Olsa <jolsa@kernel.org>
AuthorDate: Fri, 22 Aug 2014 15:58:38 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Sun, 24 Aug 2014 08:11:19 -0300

perf tools: Add +field argument support for --field option

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

  $ perf report
  Samples: 10  of event 'cycles', Event count (approx.): 4463799
  Overhead  Command  Shared Object      Symbol
    32.40%  ls       [kernel.kallsyms]  [k] filemap_fault
    28.19%  ls       [kernel.kallsyms]  [k] get_page_from_freelist
    23.38%  ls       [kernel.kallsyms]  [k] enqueue_entity
    15.04%  ls       [kernel.kallsyms]  [k] mmap_region

  $ perf report -F +period,sample
  Samples: 10  of event 'cycles', Event count (approx.): 4463799
  Overhead        Period       Samples  Command  Shared Object      Symbol
    32.40%       1446493             1  ls       [kernel.kallsyms]  [k] filemap_fault
    28.19%       1258486             1  ls       [kernel.kallsyms]  [k] get_page_from_freelist
    23.38%       1043754             1  ls       [kernel.kallsyms]  [k] enqueue_entity
    15.04%        671160             1  ls       [kernel.kallsyms]  [k] mmap_region

Works in general for commands using --field option.

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/1408715919-25990-2-git-send-email-jolsa@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/ui/hist.c   |  4 ++--
 tools/perf/util/sort.c | 24 +++++++++++++++++++-----
 tools/perf/util/sort.h |  1 +
 3 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c
index 75eb6ac..2af1837 100644
--- a/tools/perf/ui/hist.c
+++ b/tools/perf/ui/hist.c
@@ -452,7 +452,7 @@ void perf_hpp__init(void)
 	/*
 	 * If user specified field order, no need to setup default fields.
 	 */
-	if (field_order)
+	if (is_strict_order(field_order))
 		return;
 
 	if (symbol_conf.cumulate_callchain) {
@@ -519,7 +519,7 @@ void perf_hpp__column_disable(unsigned col)
 
 void perf_hpp__cancel_cumulate(void)
 {
-	if (field_order)
+	if (is_strict_order(field_order))
 		return;
 
 	perf_hpp__column_disable(PERF_HPP__OVERHEAD_ACC);
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index b4a805e..1958637 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1453,7 +1453,7 @@ static int __setup_sorting(void)
 	int ret = 0;
 
 	if (sort_keys == NULL) {
-		if (field_order) {
+		if (is_strict_order(field_order)) {
 			/*
 			 * If user specified field order but no sort order,
 			 * we'll honor it and not add default sort orders.
@@ -1639,23 +1639,36 @@ static void reset_dimensions(void)
 		memory_sort_dimensions[i].taken = 0;
 }
 
+bool is_strict_order(const char *order)
+{
+	return order && (*order != '+');
+}
+
 static int __setup_output_field(void)
 {
-	char *tmp, *tok, *str;
-	int ret = 0;
+	char *tmp, *tok, *str, *strp;
+	int ret = -EINVAL;
 
 	if (field_order == NULL)
 		return 0;
 
 	reset_dimensions();
 
-	str = strdup(field_order);
+	strp = str = strdup(field_order);
 	if (str == NULL) {
 		error("Not enough memory to setup output fields");
 		return -ENOMEM;
 	}
 
-	for (tok = strtok_r(str, ", ", &tmp);
+	if (!is_strict_order(field_order))
+		strp++;
+
+	if (!strlen(strp)) {
+		error("Invalid --fields key: `+'");
+		goto out;
+	}
+
+	for (tok = strtok_r(strp, ", ", &tmp);
 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
 		ret = output_field_add(tok);
 		if (ret == -EINVAL) {
@@ -1667,6 +1680,7 @@ static int __setup_output_field(void)
 		}
 	}
 
+out:
 	free(str);
 	return ret;
 }
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 041f0c9..c03e4ff 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -218,4 +218,5 @@ void perf_hpp__set_elide(int idx, bool elide);
 
 int report_parse_ignore_callees_opt(const struct option *opt, const char *arg, int unset);
 
+bool is_strict_order(const char *order);
 #endif	/* __PERF_SORT_H */

Re: [PATCHv2] perf tools: Add +field argument support for --sort option

[Namhyung Kim]

Hi Jiri,

On Sat, 23 Aug 2014 14:59:48 +0200, Jiri Olsa wrote:
> 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 <acme@kernel.org>
> Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Jean Pihet <jean.pihet@linaro.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung

[tip:perf/core] perf tools: Add +field argument support for --sort option

[tip-bot for Jiri Olsa]

Commit-ID:  1a1c0ffb2adb2d2ce7bb9c4dfd2935ba345cf2c2
Gitweb:     http://git.kernel.org/tip/1a1c0ffb2adb2d2ce7bb9c4dfd2935ba345cf2c2
Author:     Jiri Olsa <jolsa@redhat.com>
AuthorDate: Sat, 23 Aug 2014 14:59:48 +0200
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 17 Sep 2014 17:08:07 -0300

perf tools: Add +field argument support for --sort option

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.

v2 with changes suggested:
  - Use dynamic memory instead static buffer
  - Fix error message typo

Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Corey Ashford <cjashfor@linux.vnet.ibm.com>
Cc: David Ahern <dsahern@gmail.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jean Pihet <jean.pihet@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Link: http://lkml.kernel.org/r/20140823125948.GA1193@krava.brq.redhat.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/sort.c | 37 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 1958637..289df9d 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -1446,12 +1446,47 @@ static const char *get_default_sort_order(void)
 	return default_sort_orders[sort__mode];
 }
 
+static int setup_sort_order(void)
+{
+	char *new_sort_order;
+
+	/*
+	 * Append '+'-prefixed sort order to the default sort
+	 * order string.
+	 */
+	if (!sort_order || is_strict_order(sort_order))
+		return 0;
+
+	if (sort_order[1] == '\0') {
+		error("Invalid --sort key: `+'");
+		return -EINVAL;
+	}
+
+	/*
+	 * We allocate new sort_order string, but we never free it,
+	 * because it's checked over the rest of the code.
+	 */
+	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;
+	return 0;
+}
+
 static int __setup_sorting(void)
 {
 	char *tmp, *tok, *str;
-	const char *sort_keys = sort_order;
+	const char *sort_keys;
 	int ret = 0;
 
+	ret = setup_sort_order();
+	if (ret)
+		return ret;
+
+	sort_keys = sort_order;
 	if (sort_keys == NULL) {
 		if (is_strict_order(field_order)) {
 			/*