Re: [PATCH v2 4/5] perf tools: Support dynamic sort keys for -F/--fields

From: Namhyung Kim
Date: Tue Jan 05 2016 - 18:57:18 EST

Next message: Namhyung Kim: "Re: [PATCH v2 5/5] perf evlist: Add -T/--trace option to show trace fields"
Previous message: Steinar H. Gunderson: "Re: Does vm_operations_struct require a .owner field?"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH v2 4/5] perf tools: Support dynamic sort keys for -F/--fields"
Next in thread: Namhyung Kim: "[PATCH v2 3/5] perf tools: Add document for dynamic sort keys"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Jan 05, 2016 at 07:16:43PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 05, 2016 at 07:58:37PM +0900, Namhyung Kim escreveu:
> > Now dynamic sort keys are supported for tracepoint events, add it to
> > output fields too.
>
> Must be the end of the day, but:
>
> [root@zoo ~]# perf report -h -F -s
>
> Usage: perf report [<options>]
>
> -F, --fields <key[,keys...]>
> output field(s): overhead, period, sample plus all of sort keys
> -s, --sort <key[,key2...]>
> sort by key(s): pid, comm, dso, symbol, parent, cpu, srcline, ... Please refer the man page for the complete list.
>
> [root@zoo ~]#
>
> So, with this file, for instance:
>
> [root@zoo ~]# perf evlist -v
> sched:sched_switch: type: 2, size: 112, config: 0x10b, { sample_period, sample_freq }: 1, sample_type: IP|TID|TIME|CPU|PERIOD|RAW, disabled: 1, inherit: 1, mmap: 1, comm: 1, task: 1, sample_id_all: 1, exclude_guest: 1, mmap2: 1, comm_exec: 1
> [root@zoo ~]#
>
> I thought that these would be equivalent:
>
> [root@zoo ~]# perf report -s prev_comm,next_comm | head -20 | tail -15
> # Samples: 27K of event 'sched:sched_switch'
> # Event count (approx.): 27997
> #
> # Overhead prev_comm next_comm
> # ........ ................ ................
> #
> 10.97% firefox swapper/0
> 10.97% swapper/0 firefox
> 10.23% Xorg.bin swapper/2
> 10.23% swapper/2 Xorg.bin
> 10.20% firefox swapper/3
> 10.20% swapper/3 firefox
> 5.75% swapper/1 Xorg.bin
> 5.74% Xorg.bin swapper/1
> 3.93% Xorg.bin swapper/0
> [root@zoo ~]#
>
> [root@zoo ~]# perf report -s prev_comm,next_comm -F overhead | head -20 | tail -15
> # Samples: 27K of event 'sched:sched_switch'
> # Event count (approx.): 27997
> #
> # Overhead prev_comm next_comm
> # ........ ................ ................
> #
> 0.05% xchat swapper/1
> 0.11% xchat swapper/0
> 0.01% watchdog/3 swapper/3
> 0.01% watchdog/2 swapper/2
> 0.01% watchdog/1 swapper/1
> 0.01% watchdog/0 swapper/0
> 0.01% virsh swapper/2
> 0.08% tor swapper/3
> 0.02% tor swapper/1
> [root@zoo ~]#
>
> What am I doing wrong or didn't understood about those options?

Yes, this is subtle. The --sort option didn't really sort data but
just aggregate it. By adding -F/--fields option, the aggregation and
sorting can be separated. Behavior of the -s/--sort option is kept if
it's not used with -F/--fields (for backward compatibility). However
if it used with -F/--fields, it has the original meaning of sorting.

IOW, 'perf report -s prev_comm,next_comm' is actually same as
'perf report -F overhead,prev_comm,next_comm -s overhead'.

You run 'perf report -F overhead -s prev_com,next_comm', so it sorts
the data using alphabetical order of prev_comm and then next_comm (But
it should be reversed, will fix).

Please see the description of the original patch posting.

https://lkml.org/lkml/2014/5/19/84

Thanks,
Namhyung

>
> - Arnaldo
>
> > Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> > tools/perf/util/sort.c | 51 ++++++++++++++++++++++++++++++++------------------
> > 1 file changed, 33 insertions(+), 18 deletions(-)
> >
> > diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> > index a8a958827e39..07312ceed6eb 100644
> > --- a/tools/perf/util/sort.c
> > +++ b/tools/perf/util/sort.c
> > @@ -1908,7 +1908,7 @@ static struct perf_evsel *find_evsel(struct perf_evlist *evlist, char *event_nam
> >
> > static int __dynamic_dimension__add(struct perf_evsel *evsel,
> > struct format_field *field,
> > - bool raw_trace)
> > + bool raw_trace, bool is_sort_key)
> > {
> > struct hpp_dynamic_entry *hde;
> >
> > @@ -1918,18 +1918,24 @@ static int __dynamic_dimension__add(struct perf_evsel *evsel,
> >
> > hde->raw_trace = raw_trace;
> >
> > - perf_hpp__register_sort_field(&hde->hpp);
> > + if (is_sort_key)
> > + perf_hpp__register_sort_field(&hde->hpp);
> > + else
> > + perf_hpp__column_register(&hde->hpp);
> > +
> > return 0;
> > }
> >
> > -static int add_evsel_fields(struct perf_evsel *evsel, bool raw_trace)
> > +static int add_evsel_fields(struct perf_evsel *evsel, bool raw_trace,
> > + bool is_sort_key)
> > {
> > int ret;
> > struct format_field *field;
> >
> > field = evsel->tp_format->format.fields;
> > while (field) {
> > - ret = __dynamic_dimension__add(evsel, field, raw_trace);
> > + ret = __dynamic_dimension__add(evsel, field, raw_trace,
> > + is_sort_key);
> > if (ret < 0)
> > return ret;
> >
> > @@ -1938,7 +1944,8 @@ static int add_evsel_fields(struct perf_evsel *evsel, bool raw_trace)
> > return 0;
> > }
> >
> > -static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
> > +static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace,
> > + bool is_sort_key)
> > {
> > int ret;
> > struct perf_evsel *evsel;
> > @@ -1947,7 +1954,7 @@ static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
> > if (evsel->attr.type != PERF_TYPE_TRACEPOINT)
> > continue;
> >
> > - ret = add_evsel_fields(evsel, raw_trace);
> > + ret = add_evsel_fields(evsel, raw_trace, is_sort_key);
> > if (ret < 0)
> > return ret;
> > }
> > @@ -1955,7 +1962,8 @@ static int add_all_dynamic_fields(struct perf_evlist *evlist, bool raw_trace)
> > }
> >
> > static int add_all_matching_fields(struct perf_evlist *evlist,
> > - char *field_name, bool raw_trace)
> > + char *field_name, bool raw_trace,
> > + bool is_sort_key)
> > {
> > int ret = -ESRCH;
> > struct perf_evsel *evsel;
> > @@ -1969,14 +1977,16 @@ static int add_all_matching_fields(struct perf_evlist *evlist,
> > if (field == NULL)
> > continue;
> >
> > - ret = __dynamic_dimension__add(evsel, field, raw_trace);
> > + ret = __dynamic_dimension__add(evsel, field, raw_trace,
> > + is_sort_key);
> > if (ret < 0)
> > break;
> > }
> > return ret;
> > }
> >
> > -static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
> > +static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok,
> > + bool is_sort_key)
> > {
> > char *str, *event_name, *field_name, *opt_name;
> > struct perf_evsel *evsel;
> > @@ -2006,12 +2016,13 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
> > }
> >
> > if (!strcmp(field_name, "trace_fields")) {
> > - ret = add_all_dynamic_fields(evlist, raw_trace);
> > + ret = add_all_dynamic_fields(evlist, raw_trace, is_sort_key);
> > goto out;
> > }
> >
> > if (event_name == NULL) {
> > - ret = add_all_matching_fields(evlist, field_name, raw_trace);
> > + ret = add_all_matching_fields(evlist, field_name, raw_trace,
> > + is_sort_key);
> > goto out;
> > }
> >
> > @@ -2029,7 +2040,7 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
> > }
> >
> > if (!strcmp(field_name, "*")) {
> > - ret = add_evsel_fields(evsel, raw_trace);
> > + ret = add_evsel_fields(evsel, raw_trace, is_sort_key);
> > } else {
> > field = pevent_find_any_field(evsel->tp_format, field_name);
> > if (field == NULL) {
> > @@ -2038,7 +2049,8 @@ static int add_dynamic_entry(struct perf_evlist *evlist, const char *tok)
> > return -ENOENT;
> > }
> >
> > - ret = __dynamic_dimension__add(evsel, field, raw_trace);
> > + ret = __dynamic_dimension__add(evsel, field, raw_trace,
> > + is_sort_key);
> > }
> >
> > out:
> > @@ -2182,7 +2194,7 @@ static int sort_dimension__add(const char *tok,
> > return 0;
> > }
> >
> > - if (!add_dynamic_entry(evlist, tok))
> > + if (!add_dynamic_entry(evlist, tok, true))
> > return 0;
> >
> > return -ESRCH;
> > @@ -2387,7 +2399,7 @@ void sort__setup_elide(FILE *output)
> > }
> > }
> >
> > -static int output_field_add(char *tok)
> > +static int output_field_add(struct perf_evlist *evlist, char *tok)
> > {
> > unsigned int i;
> >
> > @@ -2427,6 +2439,9 @@ static int output_field_add(char *tok)
> > return __sort_dimension__add_output(sd);
> > }
> >
> > + if (!add_dynamic_entry(evlist, tok, false))
> > + return 0;
> > +
> > return -ESRCH;
> > }
> >
> > @@ -2452,7 +2467,7 @@ bool is_strict_order(const char *order)
> > return order && (*order != '+');
> > }
> >
> > -static int __setup_output_field(void)
> > +static int __setup_output_field(struct perf_evlist *evlist)
> > {
> > char *tmp, *tok, *str, *strp;
> > int ret = -EINVAL;
> > @@ -2476,7 +2491,7 @@ static int __setup_output_field(void)
> >
> > for (tok = strtok_r(strp, ", ", &tmp);
> > tok; tok = strtok_r(NULL, ", ", &tmp)) {
> > - ret = output_field_add(tok);
> > + ret = output_field_add(evlist, tok);
> > if (ret == -EINVAL) {
> > error("Invalid --fields key: `%s'", tok);
> > break;
> > @@ -2513,7 +2528,7 @@ int setup_sorting(struct perf_evlist *evlist)
> > if (sort__mode != SORT_MODE__DIFF)
> > perf_hpp__init();
> >
> > - err = __setup_output_field();
> > + err = __setup_output_field(evlist);
> > if (err < 0)
> > return err;
> >
> > --
> > 2.6.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Namhyung Kim: "Re: [PATCH v2 5/5] perf evlist: Add -T/--trace option to show trace fields"
Previous message: Steinar H. Gunderson: "Re: Does vm_operations_struct require a .owner field?"
In reply to: Arnaldo Carvalho de Melo: "Re: [PATCH v2 4/5] perf tools: Support dynamic sort keys for -F/--fields"
Next in thread: Namhyung Kim: "[PATCH v2 3/5] perf tools: Add document for dynamic sort keys"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]