[PATCH v2 1/3] perf util: Move block_pair_cmp to block-info

[PATCH v2 1/3] perf util: Move block_pair_cmp to block-info

[Jin Yao]

block_pair_cmp() is a function which is used to compare
two blocks. Moving it from builtin-diff.c to block-info.c
to let it be used by other builtins.

In block_pair_cmp, there is a minor change. It checks valid
for map, dso and sym first. If they are invalid, we will not
compare the address because the address might not make sense.

 v2:
 ---
 New patch created in v2

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-diff.c    | 17 -----------------
 tools/perf/util/block-info.c | 23 +++++++++++++++++++++++
 tools/perf/util/block-info.h |  2 ++
 3 files changed, 25 insertions(+), 17 deletions(-)

diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
index f8b6ae557d8b..5ff1e21082cb 100644
--- a/tools/perf/builtin-diff.c
+++ b/tools/perf/builtin-diff.c
@@ -572,23 +572,6 @@ static void init_block_hist(struct block_hist *bh)
 	bh->valid = true;
 }
 
-static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
-{
-	struct block_info *bi_a = a->block_info;
-	struct block_info *bi_b = b->block_info;
-	int cmp;
-
-	if (!bi_a->sym || !bi_b->sym)
-		return -1;
-
-	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
-
-	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
-		return 0;
-
-	return -1;
-}
-
 static struct hist_entry *get_block_pair(struct hist_entry *he,
 					 struct hists *hists_pair)
 {
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index c4b030bf6ec2..18a445938681 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -475,3 +475,26 @@ float block_info__total_cycles_percent(struct hist_entry *he)
 
 	return 0.0;
 }
+
+int block_pair_cmp(struct hist_entry *pair, struct hist_entry *he)
+{
+	struct block_info *bi_p = pair->block_info;
+	struct block_info *bi_h = he->block_info;
+	struct map_symbol *ms_p = &pair->ms;
+	struct map_symbol *ms_h = &he->ms;
+	int cmp;
+
+	if (!ms_p->map || !ms_p->map->dso || !ms_p->sym ||
+	    !ms_h->map || !ms_h->map->dso || !ms_h->sym) {
+		return -1;
+	}
+
+	cmp = strcmp(ms_p->sym->name, ms_h->sym->name);
+	if (cmp)
+		return -1;
+
+	if ((bi_p->start == bi_h->start) && (bi_p->end == bi_h->end))
+		return 0;
+
+	return -1;
+}
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index bef0d75e9819..bfa22c59195d 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -76,4 +76,6 @@ int report__browse_block_hists(struct block_hist *bh, float min_percent,
 
 float block_info__total_cycles_percent(struct hist_entry *he);
 
+int block_pair_cmp(struct hist_entry *pair, struct hist_entry *he);
+
 #endif /* __PERF_BLOCK_H */
-- 
2.17.1

[PATCH v2 2/3] perf util: Flexible to set block info output formats

[Jin Yao]

Currently we use a predefined array to set the
block info output formats, it's fixed and inflexible.

This patch adds two parameters "block_hpps" and "nr_hpps"
in block_info__create_report and other static functions,
in order to let user decide which columns to report and
with specified report ordering. It should be more flexible.

Buffers will be allocated to contain the new fmts, of course,
we need to release them before perf exits.

 v2:
 ---
 New patch created in v2.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-report.c  | 25 +++++++++++---
 tools/perf/util/block-info.c | 64 ++++++++++++++++++++++++++----------
 tools/perf/util/block-info.h | 12 +++++--
 3 files changed, 76 insertions(+), 25 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index de988589d99b..81ae1f862d11 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -104,6 +104,7 @@ struct report {
 	bool			symbol_ipc;
 	bool			total_cycles_mode;
 	struct block_report	*block_reports;
+	int			nr_block_reports;
 };
 
 static int report__config(const char *var, const char *value, void *cb)
@@ -503,7 +504,7 @@ static int perf_evlist__tui_block_hists_browse(struct evlist *evlist,
 		ret = report__browse_block_hists(&rep->block_reports[i++].hist,
 						 rep->min_percent, pos,
 						 &rep->session->header.env,
-						 &rep->annotation_opts);
+						 &rep->annotation_opts, true);
 		if (ret != 0)
 			return ret;
 	}
@@ -536,7 +537,7 @@ static int perf_evlist__tty_browse_hists(struct evlist *evlist,
 		if (rep->total_cycles_mode) {
 			report__browse_block_hists(&rep->block_reports[i++].hist,
 						   rep->min_percent, pos,
-						   NULL, NULL);
+						   NULL, NULL, true);
 			continue;
 		}
 
@@ -966,8 +967,19 @@ static int __cmd_report(struct report *rep)
 	report__output_resort(rep);
 
 	if (rep->total_cycles_mode) {
+		int block_hpps[6] = {
+			PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_PCT,
+			PERF_HPP_REPORT__BLOCK_LBR_CYCLES,
+			PERF_HPP_REPORT__BLOCK_CYCLES_PCT,
+			PERF_HPP_REPORT__BLOCK_AVG_CYCLES,
+			PERF_HPP_REPORT__BLOCK_RANGE,
+			PERF_HPP_REPORT__BLOCK_DSO,
+		};
+
 		rep->block_reports = block_info__create_report(session->evlist,
-							       rep->total_cycles);
+							       rep->total_cycles,
+							       block_hpps, 6,
+							       &rep->nr_block_reports);
 		if (!rep->block_reports)
 			return -1;
 	}
@@ -1543,8 +1555,11 @@ int cmd_report(int argc, const char **argv)
 		zfree(&report.ptime_range);
 	}
 
-	if (report.block_reports)
-		zfree(&report.block_reports);
+	if (report.block_reports) {
+		block_info__free_report(report.block_reports,
+					report.nr_block_reports);
+		report.block_reports = NULL;
+	}
 
 	zstd_fini(&(session->zstd_data));
 	perf_session__delete(session);
diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index 18a445938681..0818db17b3f2 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -376,33 +376,42 @@ static void hpp_register(struct block_fmt *block_fmt, int idx,
 }
 
 static void register_block_columns(struct perf_hpp_list *hpp_list,
-				   struct block_fmt *block_fmts)
+				   struct block_fmt *block_fmts,
+				   int *block_hpps, int nr_hpps)
 {
-	for (int i = 0; i < PERF_HPP_REPORT__BLOCK_MAX_INDEX; i++)
-		hpp_register(&block_fmts[i], i, hpp_list);
+	for (int i = 0; i < nr_hpps; i++)
+		hpp_register(&block_fmts[i], block_hpps[i], hpp_list);
 }
 
-static void init_block_hist(struct block_hist *bh, struct block_fmt *block_fmts)
+static void init_block_hist(struct block_hist *bh, struct block_fmt *block_fmts,
+			    int *block_hpps, int nr_hpps)
 {
 	__hists__init(&bh->block_hists, &bh->block_list);
 	perf_hpp_list__init(&bh->block_list);
 	bh->block_list.nr_header_lines = 1;
 
-	register_block_columns(&bh->block_list, block_fmts);
+	register_block_columns(&bh->block_list, block_fmts,
+			       block_hpps, nr_hpps);
 
-	perf_hpp_list__register_sort_field(&bh->block_list,
-		&block_fmts[PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_PCT].fmt);
+	/* Sort by the first fmt */
+	perf_hpp_list__register_sort_field(&bh->block_list, &block_fmts[0].fmt);
 }
 
-static void process_block_report(struct hists *hists,
-				 struct block_report *block_report,
-				 u64 total_cycles)
+static int process_block_report(struct hists *hists,
+				struct block_report *block_report,
+				u64 total_cycles, int *block_hpps,
+				int nr_hpps)
 {
 	struct rb_node *next = rb_first_cached(&hists->entries);
 	struct block_hist *bh = &block_report->hist;
 	struct hist_entry *he;
 
-	init_block_hist(bh, block_report->fmts);
+	block_report->fmts = calloc(nr_hpps, sizeof(struct block_fmt));
+	if (!block_report->fmts)
+		return -1;
+
+	block_report->nr_fmts = nr_hpps;
+	init_block_hist(bh, block_report->fmts, block_hpps, nr_hpps);
 
 	while (next) {
 		he = rb_entry(next, struct hist_entry, rb_node);
@@ -411,16 +420,19 @@ static void process_block_report(struct hists *hists,
 		next = rb_next(&he->rb_node);
 	}
 
-	for (int i = 0; i < PERF_HPP_REPORT__BLOCK_MAX_INDEX; i++) {
+	for (int i = 0; i < nr_hpps; i++) {
 		block_report->fmts[i].total_cycles = total_cycles;
 		block_report->fmts[i].block_cycles = block_report->cycles;
 	}
 
 	hists__output_resort(&bh->block_hists, NULL);
+	return 0;
 }
 
 struct block_report *block_info__create_report(struct evlist *evlist,
-					       u64 total_cycles)
+					       u64 total_cycles,
+					       int *block_hpps, int nr_hpps,
+					       int *nr_reps)
 {
 	struct block_report *block_reports;
 	int nr_hists = evlist->core.nr_entries, i = 0;
@@ -433,16 +445,30 @@ struct block_report *block_info__create_report(struct evlist *evlist,
 	evlist__for_each_entry(evlist, pos) {
 		struct hists *hists = evsel__hists(pos);
 
-		process_block_report(hists, &block_reports[i], total_cycles);
+		process_block_report(hists, &block_reports[i], total_cycles,
+				     block_hpps, nr_hpps);
 		i++;
 	}
 
+	*nr_reps = nr_hists;
 	return block_reports;
 }
 
+void block_info__free_report(struct block_report *reps, int nr_reps)
+{
+	for (int i = 0; i < nr_reps; i++) {
+		hists__delete_entries(&reps[i].hist.block_hists);
+		if (reps[i].fmts)
+			free(reps[i].fmts);
+	}
+
+	free(reps);
+}
+
 int report__browse_block_hists(struct block_hist *bh, float min_percent,
 			       struct evsel *evsel, struct perf_env *env,
-			       struct annotation_options *annotation_opts)
+			       struct annotation_options *annotation_opts,
+			       bool release)
 {
 	int ret;
 
@@ -451,13 +477,17 @@ int report__browse_block_hists(struct block_hist *bh, float min_percent,
 		symbol_conf.report_individual_block = true;
 		hists__fprintf(&bh->block_hists, true, 0, 0, min_percent,
 			       stdout, true);
-		hists__delete_entries(&bh->block_hists);
+		if (release)
+			hists__delete_entries(&bh->block_hists);
+
 		return 0;
 	case 1:
 		symbol_conf.report_individual_block = true;
 		ret = block_hists_tui_browse(bh, evsel, min_percent,
 					     env, annotation_opts);
-		hists__delete_entries(&bh->block_hists);
+		if (release)
+			hists__delete_entries(&bh->block_hists);
+
 		return ret;
 	default:
 		return -1;
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index bfa22c59195d..0bf01e3a423d 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -44,7 +44,8 @@ enum {
 struct block_report {
 	struct block_hist	hist;
 	u64			cycles;
-	struct block_fmt	fmts[PERF_HPP_REPORT__BLOCK_MAX_INDEX];
+	struct block_fmt	*fmts;
+	int			nr_fmts;
 };
 
 struct block_hist;
@@ -68,11 +69,16 @@ int block_info__process_sym(struct hist_entry *he, struct block_hist *bh,
 			    u64 *block_cycles_aggr, u64 total_cycles);
 
 struct block_report *block_info__create_report(struct evlist *evlist,
-					       u64 total_cycles);
+					       u64 total_cycles,
+					       int *block_hpps, int nr_hpps,
+					       int *nr_reps);
+
+void block_info__free_report(struct block_report *reps, int nr_reps);
 
 int report__browse_block_hists(struct block_hist *bh, float min_percent,
 			       struct evsel *evsel, struct perf_env *env,
-			       struct annotation_options *annotation_opts);
+			       struct annotation_options *annotation_opts,
+			       bool release);
 
 float block_info__total_cycles_percent(struct hist_entry *he);
 
-- 
2.17.1

[PATCH v2 3/3] perf util: Support color ops to print block percents in color

[Jin Yao]

It would be nice to print the block percents with colors.

This patch supports the 'Sampled Cycles%' and 'Avg Cycles%'
printed in colors.

For example,

perf record -b ...
perf report --total-cycles or perf report --total-cycles --stdio

percent > 5%, colored in red
percent > 0.5%, colored in green
percent < 0.5%, default color

 v2:
 ---
 No functional change

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/block-info.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index 0818db17b3f2..60b3a0107177 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -185,6 +185,17 @@ static int block_column_width(struct perf_hpp_fmt *fmt,
 	return block_fmt->width;
 }
 
+static int color_pct(struct perf_hpp *hpp, int width, double pct)
+{
+#ifdef HAVE_SLANG_SUPPORT
+	if (use_browser) {
+		return __hpp__slsmg_color_printf(hpp, "%*.2f%%",
+						 width - 1, pct);
+	}
+#endif
+	return hpp_color_scnprintf(hpp, "%*.2f%%", width - 1, pct);
+}
+
 static int block_total_cycles_pct_entry(struct perf_hpp_fmt *fmt,
 					struct perf_hpp *hpp,
 					struct hist_entry *he)
@@ -192,14 +203,11 @@ static int block_total_cycles_pct_entry(struct perf_hpp_fmt *fmt,
 	struct block_fmt *block_fmt = container_of(fmt, struct block_fmt, fmt);
 	struct block_info *bi = he->block_info;
 	double ratio = 0.0;
-	char buf[16];
 
 	if (block_fmt->total_cycles)
 		ratio = (double)bi->cycles / (double)block_fmt->total_cycles;
 
-	sprintf(buf, "%.2f%%", 100.0 * ratio);
-
-	return scnprintf(hpp->buf, hpp->size, "%*s", block_fmt->width, buf);
+	return color_pct(hpp, block_fmt->width, 100.0 * ratio);
 }
 
 static int64_t block_total_cycles_pct_sort(struct perf_hpp_fmt *fmt,
@@ -252,16 +260,13 @@ static int block_cycles_pct_entry(struct perf_hpp_fmt *fmt,
 	struct block_info *bi = he->block_info;
 	double ratio = 0.0;
 	u64 avg;
-	char buf[16];
 
 	if (block_fmt->block_cycles && bi->num_aggr) {
 		avg = bi->cycles_aggr / bi->num_aggr;
 		ratio = (double)avg / (double)block_fmt->block_cycles;
 	}
 
-	sprintf(buf, "%.2f%%", 100.0 * ratio);
-
-	return scnprintf(hpp->buf, hpp->size, "%*s", block_fmt->width, buf);
+	return color_pct(hpp, block_fmt->width, 100.0 * ratio);
 }
 
 static int block_avg_cycles_entry(struct perf_hpp_fmt *fmt,
@@ -348,7 +353,7 @@ static void hpp_register(struct block_fmt *block_fmt, int idx,
 
 	switch (idx) {
 	case PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_PCT:
-		fmt->entry = block_total_cycles_pct_entry;
+		fmt->color = block_total_cycles_pct_entry;
 		fmt->cmp = block_info__cmp;
 		fmt->sort = block_total_cycles_pct_sort;
 		break;
@@ -356,7 +361,7 @@ static void hpp_register(struct block_fmt *block_fmt, int idx,
 		fmt->entry = block_cycles_lbr_entry;
 		break;
 	case PERF_HPP_REPORT__BLOCK_CYCLES_PCT:
-		fmt->entry = block_cycles_pct_entry;
+		fmt->color = block_cycles_pct_entry;
 		break;
 	case PERF_HPP_REPORT__BLOCK_AVG_CYCLES:
 		fmt->entry = block_avg_cycles_entry;
-- 
2.17.1

Re: [PATCH v2 1/3] perf util: Move block_pair_cmp to block-info

[Jiri Olsa]

On Tue, Jan 07, 2020 at 03:45:23AM +0800, Jin Yao wrote:
> block_pair_cmp() is a function which is used to compare
> two blocks. Moving it from builtin-diff.c to block-info.c
> to let it be used by other builtins.
> 
> In block_pair_cmp, there is a minor change. It checks valid
> for map, dso and sym first. If they are invalid, we will not
> compare the address because the address might not make sense.

please separate the change as well, it's hard to track
what you did when the whole function is moved

> 
>  v2:
>  ---
>  New patch created in v2
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-diff.c    | 17 -----------------
>  tools/perf/util/block-info.c | 23 +++++++++++++++++++++++
>  tools/perf/util/block-info.h |  2 ++
>  3 files changed, 25 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
> index f8b6ae557d8b..5ff1e21082cb 100644
> --- a/tools/perf/builtin-diff.c
> +++ b/tools/perf/builtin-diff.c
> @@ -572,23 +572,6 @@ static void init_block_hist(struct block_hist *bh)
>  	bh->valid = true;
>  }
>  
> -static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
> -{
> -	struct block_info *bi_a = a->block_info;
> -	struct block_info *bi_b = b->block_info;
> -	int cmp;
> -
> -	if (!bi_a->sym || !bi_b->sym)
> -		return -1;
> -
> -	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
> -
> -	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
> -		return 0;
> -
> -	return -1;
> -}
> -
>  static struct hist_entry *get_block_pair(struct hist_entry *he,
>  					 struct hists *hists_pair)
>  {
> diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
> index c4b030bf6ec2..18a445938681 100644
> --- a/tools/perf/util/block-info.c
> +++ b/tools/perf/util/block-info.c
> @@ -475,3 +475,26 @@ float block_info__total_cycles_percent(struct hist_entry *he)
>  
>  	return 0.0;
>  }
> +
> +int block_pair_cmp(struct hist_entry *pair, struct hist_entry *he)
> +{
> +	struct block_info *bi_p = pair->block_info;
> +	struct block_info *bi_h = he->block_info;
> +	struct map_symbol *ms_p = &pair->ms;
> +	struct map_symbol *ms_h = &he->ms;
> +	int cmp;
> +
> +	if (!ms_p->map || !ms_p->map->dso || !ms_p->sym ||
> +	    !ms_h->map || !ms_h->map->dso || !ms_h->sym) {
> +		return -1;
> +	}
> +
> +	cmp = strcmp(ms_p->sym->name, ms_h->sym->name);
> +	if (cmp)
> +		return -1;

should this return cmp? also you don't mention this change in the changelog

thanks,
jirka

Re: [PATCH v2 2/3] perf util: Flexible to set block info output formats

[Jiri Olsa]

On Tue, Jan 07, 2020 at 03:45:24AM +0800, Jin Yao wrote:
> Currently we use a predefined array to set the
> block info output formats, it's fixed and inflexible.
> 
> This patch adds two parameters "block_hpps" and "nr_hpps"
> in block_info__create_report and other static functions,
> in order to let user decide which columns to report and
> with specified report ordering. It should be more flexible.
> 
> Buffers will be allocated to contain the new fmts, of course,
> we need to release them before perf exits.
> 
>  v2:
>  ---
>  New patch created in v2.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-report.c  | 25 +++++++++++---
>  tools/perf/util/block-info.c | 64 ++++++++++++++++++++++++++----------
>  tools/perf/util/block-info.h | 12 +++++--
>  3 files changed, 76 insertions(+), 25 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index de988589d99b..81ae1f862d11 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -104,6 +104,7 @@ struct report {
>  	bool			symbol_ipc;
>  	bool			total_cycles_mode;
>  	struct block_report	*block_reports;
> +	int			nr_block_reports;
>  };
>  
>  static int report__config(const char *var, const char *value, void *cb)
> @@ -503,7 +504,7 @@ static int perf_evlist__tui_block_hists_browse(struct evlist *evlist,
>  		ret = report__browse_block_hists(&rep->block_reports[i++].hist,
>  						 rep->min_percent, pos,
>  						 &rep->session->header.env,
> -						 &rep->annotation_opts);
> +						 &rep->annotation_opts, true);
>  		if (ret != 0)
>  			return ret;
>  	}
> @@ -536,7 +537,7 @@ static int perf_evlist__tty_browse_hists(struct evlist *evlist,
>  		if (rep->total_cycles_mode) {
>  			report__browse_block_hists(&rep->block_reports[i++].hist,
>  						   rep->min_percent, pos,
> -						   NULL, NULL);
> +						   NULL, NULL, true);
>  			continue;
>  		}
>  
> @@ -966,8 +967,19 @@ static int __cmd_report(struct report *rep)
>  	report__output_resort(rep);
>  
>  	if (rep->total_cycles_mode) {
> +		int block_hpps[6] = {
> +			PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_PCT,
> +			PERF_HPP_REPORT__BLOCK_LBR_CYCLES,
> +			PERF_HPP_REPORT__BLOCK_CYCLES_PCT,
> +			PERF_HPP_REPORT__BLOCK_AVG_CYCLES,
> +			PERF_HPP_REPORT__BLOCK_RANGE,
> +			PERF_HPP_REPORT__BLOCK_DSO,
> +		};

I'd understand this change if there was another place in the code using
this, but it's not part of this patchset.. is it comming later?

thanks,
jirka

Re: [PATCH v2 1/3] perf util: Move block_pair_cmp to block-info

[Jin, Yao]

On 1/7/2020 5:57 PM, Jiri Olsa wrote:
> On Tue, Jan 07, 2020 at 03:45:23AM +0800, Jin Yao wrote:
>> block_pair_cmp() is a function which is used to compare
>> two blocks. Moving it from builtin-diff.c to block-info.c
>> to let it be used by other builtins.
>>
>> In block_pair_cmp, there is a minor change. It checks valid
>> for map, dso and sym first. If they are invalid, we will not
>> compare the address because the address might not make sense.
> 
> please separate the change as well, it's hard to track
> what you did when the whole function is moved
> 

Got it, thanks! I will separate this too.

>>
>>   v2:
>>   ---
>>   New patch created in v2
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-diff.c    | 17 -----------------
>>   tools/perf/util/block-info.c | 23 +++++++++++++++++++++++
>>   tools/perf/util/block-info.h |  2 ++
>>   3 files changed, 25 insertions(+), 17 deletions(-)
>>
>> diff --git a/tools/perf/builtin-diff.c b/tools/perf/builtin-diff.c
>> index f8b6ae557d8b..5ff1e21082cb 100644
>> --- a/tools/perf/builtin-diff.c
>> +++ b/tools/perf/builtin-diff.c
>> @@ -572,23 +572,6 @@ static void init_block_hist(struct block_hist *bh)
>>   	bh->valid = true;
>>   }
>>   
>> -static int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
>> -{
>> -	struct block_info *bi_a = a->block_info;
>> -	struct block_info *bi_b = b->block_info;
>> -	int cmp;
>> -
>> -	if (!bi_a->sym || !bi_b->sym)
>> -		return -1;
>> -
>> -	cmp = strcmp(bi_a->sym->name, bi_b->sym->name);
>> -
>> -	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
>> -		return 0;
>> -
>> -	return -1;
>> -}
>> -
>>   static struct hist_entry *get_block_pair(struct hist_entry *he,
>>   					 struct hists *hists_pair)
>>   {
>> diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
>> index c4b030bf6ec2..18a445938681 100644
>> --- a/tools/perf/util/block-info.c
>> +++ b/tools/perf/util/block-info.c
>> @@ -475,3 +475,26 @@ float block_info__total_cycles_percent(struct hist_entry *he)
>>   
>>   	return 0.0;
>>   }
>> +
>> +int block_pair_cmp(struct hist_entry *pair, struct hist_entry *he)
>> +{
>> +	struct block_info *bi_p = pair->block_info;
>> +	struct block_info *bi_h = he->block_info;
>> +	struct map_symbol *ms_p = &pair->ms;
>> +	struct map_symbol *ms_h = &he->ms;
>> +	int cmp;
>> +
>> +	if (!ms_p->map || !ms_p->map->dso || !ms_p->sym ||
>> +	    !ms_h->map || !ms_h->map->dso || !ms_h->sym) {
>> +		return -1;
>> +	}
>> +
>> +	cmp = strcmp(ms_p->sym->name, ms_h->sym->name);
>> +	if (cmp)
>> +		return -1;
> 
> should this return cmp? also you don't mention this change in the changelog
> 

Yes, return cmp should be OK.

It's changed from "strcmp(bi_a->sym->name, bi_b->sym->name)" to 
"strcmp(ms_p->sym->name, ms_h->sym->name)" is because we don't need an 
additional checking for bi_a->sym and bi_b->sym.

If we use "strcmp(bi_a->sym->name, bi_b->sym->name)" here, I think we'd 
better check the sym first. I will mention that in changelog.

Thanks
Jin Yao

> thanks,
> jirka
> 

Re: [PATCH v2 2/3] perf util: Flexible to set block info output formats

[Jin, Yao]

On 1/7/2020 6:06 PM, Jiri Olsa wrote:
> On Tue, Jan 07, 2020 at 03:45:24AM +0800, Jin Yao wrote:
>> Currently we use a predefined array to set the
>> block info output formats, it's fixed and inflexible.
>>
>> This patch adds two parameters "block_hpps" and "nr_hpps"
>> in block_info__create_report and other static functions,
>> in order to let user decide which columns to report and
>> with specified report ordering. It should be more flexible.
>>
>> Buffers will be allocated to contain the new fmts, of course,
>> we need to release them before perf exits.
>>
>>   v2:
>>   ---
>>   New patch created in v2.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-report.c  | 25 +++++++++++---
>>   tools/perf/util/block-info.c | 64 ++++++++++++++++++++++++++----------
>>   tools/perf/util/block-info.h | 12 +++++--
>>   3 files changed, 76 insertions(+), 25 deletions(-)
>>
>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>> index de988589d99b..81ae1f862d11 100644
>> --- a/tools/perf/builtin-report.c
>> +++ b/tools/perf/builtin-report.c
>> @@ -104,6 +104,7 @@ struct report {
>>   	bool			symbol_ipc;
>>   	bool			total_cycles_mode;
>>   	struct block_report	*block_reports;
>> +	int			nr_block_reports;
>>   };
>>   
>>   static int report__config(const char *var, const char *value, void *cb)
>> @@ -503,7 +504,7 @@ static int perf_evlist__tui_block_hists_browse(struct evlist *evlist,
>>   		ret = report__browse_block_hists(&rep->block_reports[i++].hist,
>>   						 rep->min_percent, pos,
>>   						 &rep->session->header.env,
>> -						 &rep->annotation_opts);
>> +						 &rep->annotation_opts, true);
>>   		if (ret != 0)
>>   			return ret;
>>   	}
>> @@ -536,7 +537,7 @@ static int perf_evlist__tty_browse_hists(struct evlist *evlist,
>>   		if (rep->total_cycles_mode) {
>>   			report__browse_block_hists(&rep->block_reports[i++].hist,
>>   						   rep->min_percent, pos,
>> -						   NULL, NULL);
>> +						   NULL, NULL, true);
>>   			continue;
>>   		}
>>   
>> @@ -966,8 +967,19 @@ static int __cmd_report(struct report *rep)
>>   	report__output_resort(rep);
>>   
>>   	if (rep->total_cycles_mode) {
>> +		int block_hpps[6] = {
>> +			PERF_HPP_REPORT__BLOCK_TOTAL_CYCLES_PCT,
>> +			PERF_HPP_REPORT__BLOCK_LBR_CYCLES,
>> +			PERF_HPP_REPORT__BLOCK_CYCLES_PCT,
>> +			PERF_HPP_REPORT__BLOCK_AVG_CYCLES,
>> +			PERF_HPP_REPORT__BLOCK_RANGE,
>> +			PERF_HPP_REPORT__BLOCK_DSO,
>> +		};
> 
> I'd understand this change if there was another place in the code using
> this, but it's not part of this patchset.. is it comming later?
> 

Yes, you guessed it. :)

I will have some follow-up patches, maybe post some weeks later.

Thanks
Jin Yao

> thanks,
> jirka
>