* [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options @ 2014-08-22 13:58 Jiri Olsa 2014-08-22 13:58 ` [PATCH 1/2] perf tools: Add +field argument support for --field option Jiri Olsa ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Jiri Olsa @ 2014-08-22 13:58 UTC (permalink / raw) To: linux-kernel Cc: Arnaldo Carvalho de Melo, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra, 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(-) ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] perf tools: Add +field argument support for --field option 2014-08-22 13:58 [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Jiri Olsa @ 2014-08-22 13:58 ` Jiri Olsa 2014-08-24 15:00 ` [tip:perf/core] " tip-bot for Jiri Olsa 2014-08-22 13:58 ` [PATCH 2/2] perf tools: Add +field argument support for --sort option Jiri Olsa 2014-08-22 15:02 ` [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Arnaldo Carvalho de Melo 2 siblings, 1 reply; 10+ messages in thread From: Jiri Olsa @ 2014-08-22 13:58 UTC (permalink / raw) To: linux-kernel Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [tip:perf/core] perf tools: Add +field argument support for --field option 2014-08-22 13:58 ` [PATCH 1/2] perf tools: Add +field argument support for --field option Jiri Olsa @ 2014-08-24 15:00 ` tip-bot for Jiri Olsa 0 siblings, 0 replies; 10+ messages in thread From: tip-bot for Jiri Olsa @ 2014-08-24 15:00 UTC (permalink / raw) To: linux-tip-commits Cc: acme, linux-kernel, paulus, hpa, mingo, jolsa, a.p.zijlstra, jean.pihet, namhyung, fweisbec, dsahern, tglx, cjashfor 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 */ ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] perf tools: Add +field argument support for --sort option 2014-08-22 13:58 [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Jiri Olsa 2014-08-22 13:58 ` [PATCH 1/2] perf tools: Add +field argument support for --field option Jiri Olsa @ 2014-08-22 13:58 ` Jiri Olsa 2014-08-22 15:16 ` Arnaldo Carvalho de Melo 2014-08-22 15:02 ` [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Arnaldo Carvalho de Melo 2 siblings, 1 reply; 10+ messages in thread From: Jiri Olsa @ 2014-08-22 13:58 UTC (permalink / raw) To: linux-kernel Cc: Jiri Olsa, Arnaldo Carvalho de Melo, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf tools: Add +field argument support for --sort option 2014-08-22 13:58 ` [PATCH 2/2] perf tools: Add +field argument support for --sort option Jiri Olsa @ 2014-08-22 15:16 ` Arnaldo Carvalho de Melo 2014-08-22 15:23 ` Jiri Olsa 0 siblings, 1 reply; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2014-08-22 15:16 UTC (permalink / raw) To: Jiri Olsa Cc: linux-kernel, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf tools: Add +field argument support for --sort option 2014-08-22 15:16 ` Arnaldo Carvalho de Melo @ 2014-08-22 15:23 ` Jiri Olsa 2014-08-23 12:59 ` [PATCHv2] " Jiri Olsa 0 siblings, 1 reply; 10+ messages in thread From: Jiri Olsa @ 2014-08-22 15:23 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, linux-kernel, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCHv2] perf tools: Add +field argument support for --sort option 2014-08-22 15:23 ` Jiri Olsa @ 2014-08-23 12:59 ` Jiri Olsa 2014-08-26 8:02 ` Namhyung Kim 2014-09-19 5:18 ` [tip:perf/core] " tip-bot for Jiri Olsa 0 siblings, 2 replies; 10+ messages in thread From: Jiri Olsa @ 2014-08-23 12:59 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Jiri Olsa, linux-kernel, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCHv2] perf tools: Add +field argument support for --sort option 2014-08-23 12:59 ` [PATCHv2] " Jiri Olsa @ 2014-08-26 8:02 ` Namhyung Kim 2014-09-19 5:18 ` [tip:perf/core] " tip-bot for Jiri Olsa 1 sibling, 0 replies; 10+ messages in thread From: Namhyung Kim @ 2014-08-26 8:02 UTC (permalink / raw) To: Jiri Olsa Cc: Arnaldo Carvalho de Melo, Jiri Olsa, linux-kernel, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Paul Mackerras, Peter Zijlstra 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [tip:perf/core] perf tools: Add +field argument support for --sort option 2014-08-23 12:59 ` [PATCHv2] " Jiri Olsa 2014-08-26 8:02 ` Namhyung Kim @ 2014-09-19 5:18 ` tip-bot for Jiri Olsa 1 sibling, 0 replies; 10+ messages in thread From: tip-bot for Jiri Olsa @ 2014-09-19 5:18 UTC (permalink / raw) To: linux-tip-commits Cc: acme, linux-kernel, paulus, hpa, mingo, jolsa, a.p.zijlstra, jean.pihet, namhyung, jolsa, fweisbec, dsahern, tglx, cjashfor 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)) { /* ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options 2014-08-22 13:58 [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Jiri Olsa 2014-08-22 13:58 ` [PATCH 1/2] perf tools: Add +field argument support for --field option Jiri Olsa 2014-08-22 13:58 ` [PATCH 2/2] perf tools: Add +field argument support for --sort option Jiri Olsa @ 2014-08-22 15:02 ` Arnaldo Carvalho de Melo 2 siblings, 0 replies; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2014-08-22 15:02 UTC (permalink / raw) To: Jiri Olsa Cc: linux-kernel, Corey Ashford, David Ahern, Frederic Weisbecker, Ingo Molnar, Jean Pihet, Namhyung Kim, Paul Mackerras, Peter Zijlstra 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(-) ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-09-19 5:19 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-22 13:58 [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Jiri Olsa 2014-08-22 13:58 ` [PATCH 1/2] perf tools: Add +field argument support for --field option Jiri Olsa 2014-08-24 15:00 ` [tip:perf/core] " tip-bot for Jiri Olsa 2014-08-22 13:58 ` [PATCH 2/2] perf tools: Add +field argument support for --sort option Jiri Olsa 2014-08-22 15:16 ` Arnaldo Carvalho de Melo 2014-08-22 15:23 ` Jiri Olsa 2014-08-23 12:59 ` [PATCHv2] " Jiri Olsa 2014-08-26 8:02 ` Namhyung Kim 2014-09-19 5:18 ` [tip:perf/core] " tip-bot for Jiri Olsa 2014-08-22 15:02 ` [PATCH 0/2] perf tools: Add +field argument support for --field/--sort options Arnaldo Carvalho de Melo
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.