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

[Namhyung Kim <namhyung@kernel.org>]

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@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> >  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