[PATCH v5 0/4] perf: Refactor the block info implementation

[PATCH v5 0/4] perf: Refactor the block info implementation

[Jin Yao]

This patch series refactors the block info functionalities to let them
be used by other builtins and allow setting the output fmts flexibly.

It also supports the 'Sampled Cycles%' and 'Avg Cycles%' printed in
colors.

 v5:
 ---
 Only change the patch "perf util: Flexible to set block info output formats".
 Other patches are not changed.

Jin Yao (4):
  perf util: Move block_pair_cmp to block-info
  perf util: Validate map/dso/sym before comparing blocks
  perf util: Flexible to set block info output formats
  perf util: Support color ops to print block percents in color

 tools/perf/builtin-diff.c    | 17 -------
 tools/perf/builtin-report.c  | 21 ++++++--
 tools/perf/util/block-info.c | 99 ++++++++++++++++++++++++++----------
 tools/perf/util/block-info.h |  9 +++-
 4 files changed, 99 insertions(+), 47 deletions(-)

-- 
2.17.1

[PATCH v5 1/4] 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 can be used by other builtins.

 v4/v5:
 ------
 No change.

 v3:
 ---
 Separate it from original patch for good tracking.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/builtin-diff.c    | 17 -----------------
 tools/perf/util/block-info.c | 17 +++++++++++++++++
 tools/perf/util/block-info.h |  2 ++
 3 files changed, 19 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..f0f38bdd496a 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -475,3 +475,20 @@ float block_info__total_cycles_percent(struct hist_entry *he)
 
 	return 0.0;
 }
+
+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;
+}
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index bef0d75e9819..4fa91eeae92e 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 *a, struct hist_entry *b);
+
 #endif /* __PERF_BLOCK_H */
-- 
2.17.1

[PATCH v5 2/4] perf util: Validate map/dso/sym before comparing blocks

[Jin Yao]

block_pair_cmp() is a function which is used to compare
two blocks. This patch checks the validity of map, dso and
sym before comparing blocks.

If they are invalid, we will not compare the address because
the address might not make sense.

Another change is it returns cmp if sym->name is not equal.

This patch uses "strcmp(ms_p->sym->name, ms_h->sym->name)" is
because we have checked ms->sym yet, we don't need an additional
checking for bi->sym.

 v4/v5:
 ------
 No change.

 v3:
 ---
 Separate from original patch.
 Return cmp if it's not 0.

Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
---
 tools/perf/util/block-info.c | 18 ++++++++++++------
 tools/perf/util/block-info.h |  2 +-
 2 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/block-info.c b/tools/perf/util/block-info.c
index f0f38bdd496a..2d0929aa0a65 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -476,18 +476,24 @@ float block_info__total_cycles_percent(struct hist_entry *he)
 	return 0.0;
 }
 
-int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
+int block_pair_cmp(struct hist_entry *pair, struct hist_entry *he)
 {
-	struct block_info *bi_a = a->block_info;
-	struct block_info *bi_b = b->block_info;
+	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 (!bi_a->sym || !bi_b->sym)
+	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(bi_a->sym->name, bi_b->sym->name);
+	cmp = strcmp(ms_p->sym->name, ms_h->sym->name);
+	if (cmp)
+		return cmp;
 
-	if ((!cmp) && (bi_a->start == bi_b->start) && (bi_a->end == bi_b->end))
+	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 4fa91eeae92e..bfa22c59195d 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -76,6 +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 *a, struct hist_entry *b);
+int block_pair_cmp(struct hist_entry *pair, struct hist_entry *he);
 
 #endif /* __PERF_BLOCK_H */
-- 
2.17.1

[PATCH v5 3/4] 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.

 v5:
 ---
 Don't need to call hists__delete_entries() after printing the
 block hists, because we will release the block hists in
 block_info__free_report().

 v4:
 ---
 Keep fmts in struct block_report as an array, instead of
 allocaing it. That's we don't need the release code.

 v3:
 ---
 No change.

 v2:
 ---
 New patch created in v2.

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

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 9483b3f0cae3..1ffc95c0e138 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)
@@ -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;
 	}
@@ -1551,8 +1563,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 2d0929aa0a65..5eb23c58f194 100644
--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -376,33 +376,41 @@ 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);
+	if (nr_hpps > PERF_HPP_REPORT__BLOCK_MAX_INDEX)
+		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 +419,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,13 +444,23 @@ 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);
+
+	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)
@@ -451,13 +472,11 @@ 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);
 		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);
 		return ret;
 	default:
 		return -1;
diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
index bfa22c59195d..0e01c63b09a2 100644
--- a/tools/perf/util/block-info.h
+++ b/tools/perf/util/block-info.h
@@ -45,6 +45,7 @@ struct block_report {
 	struct block_hist	hist;
 	u64			cycles;
 	struct block_fmt	fmts[PERF_HPP_REPORT__BLOCK_MAX_INDEX];
+	int			nr_fmts;
 };
 
 struct block_hist;
@@ -68,7 +69,11 @@ 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,
-- 
2.17.1

[PATCH v5 4/4] 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

 v3/v4/v5:
 ---------
 No change

 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 5eb23c58f194..fe2cee3e3068 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 v5 0/4] perf: Refactor the block info implementation

[Jiri Olsa]

On Tue, Jan 28, 2020 at 08:55:52PM +0800, Jin Yao wrote:
> This patch series refactors the block info functionalities to let them
> be used by other builtins and allow setting the output fmts flexibly.
> 
> It also supports the 'Sampled Cycles%' and 'Avg Cycles%' printed in
> colors.
> 
>  v5:
>  ---
>  Only change the patch "perf util: Flexible to set block info output formats".
>  Other patches are not changed.
> 
> Jin Yao (4):
>   perf util: Move block_pair_cmp to block-info
>   perf util: Validate map/dso/sym before comparing blocks
>   perf util: Flexible to set block info output formats
>   perf util: Support color ops to print block percents in color

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> 
>  tools/perf/builtin-diff.c    | 17 -------
>  tools/perf/builtin-report.c  | 21 ++++++--
>  tools/perf/util/block-info.c | 99 ++++++++++++++++++++++++++----------
>  tools/perf/util/block-info.h |  9 +++-
>  4 files changed, 99 insertions(+), 47 deletions(-)
> 
> -- 
> 2.17.1
> 

Re: [PATCH v5 1/4] perf util: Move block_pair_cmp to block-info

[Arnaldo Carvalho de Melo]

Em Tue, Jan 28, 2020 at 08:55:53PM +0800, Jin Yao escreveu:
> 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 can be used by other builtins.
> 
>  v4/v5:
>  ------
>  No change.
> 
>  v3:
>  ---
>  Separate it from original patch for good tracking.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/builtin-diff.c    | 17 -----------------
>  tools/perf/util/block-info.c | 17 +++++++++++++++++
>  tools/perf/util/block-info.h |  2 ++
>  3 files changed, 19 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..f0f38bdd496a 100644
> --- a/tools/perf/util/block-info.c
> +++ b/tools/perf/util/block-info.c
> @@ -475,3 +475,20 @@ float block_info__total_cycles_percent(struct hist_entry *he)
>  
>  	return 0.0;
>  }
> +
> +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)

First thing that came to mind was that hist_entry comparision functions
had been changed to return int64_t recently, when I went to look at it I
found this:

tools/perf/util/block-info.c

int64_t block_info__cmp(struct perf_hpp_fmt *fmt __maybe_unused,
                        struct hist_entry *left, struct hist_entry *right)
{
        struct block_info *bi_l = left->block_info;
        struct block_info *bi_r = right->block_info;
        int cmp;
.
.
.

Which look a bit more complete, can you check if that can be used
instead or explain why my quick analysis of this is b0rken?

Perhaps we can have a __block_info__cmp() that doesn't receive the
perf_hpp_fmt (that isn't even used above) so that the previous use of
block_pair_cmp() can be replaced with __block_info__cmp() instead?

Thanks,

- Arnaldo

> +{
> +	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;
> +}
> diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
> index bef0d75e9819..4fa91eeae92e 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 *a, struct hist_entry *b);
> +
>  #endif /* __PERF_BLOCK_H */
> -- 
> 2.17.1
> 

-- 

- Arnaldo

Re: [PATCH v5 1/4] perf util: Move block_pair_cmp to block-info

[Jin, Yao]

On 1/31/2020 4:32 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jan 28, 2020 at 08:55:53PM +0800, Jin Yao escreveu:
>> 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 can be used by other builtins.
>>
>>   v4/v5:
>>   ------
>>   No change.
>>
>>   v3:
>>   ---
>>   Separate it from original patch for good tracking.
>>
>> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
>> ---
>>   tools/perf/builtin-diff.c    | 17 -----------------
>>   tools/perf/util/block-info.c | 17 +++++++++++++++++
>>   tools/perf/util/block-info.h |  2 ++
>>   3 files changed, 19 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..f0f38bdd496a 100644
>> --- a/tools/perf/util/block-info.c
>> +++ b/tools/perf/util/block-info.c
>> @@ -475,3 +475,20 @@ float block_info__total_cycles_percent(struct hist_entry *he)
>>   
>>   	return 0.0;
>>   }
>> +
>> +int block_pair_cmp(struct hist_entry *a, struct hist_entry *b)
> 
> First thing that came to mind was that hist_entry comparision functions
> had been changed to return int64_t recently, when I went to look at it I
> found this:
> 
> tools/perf/util/block-info.c
> 
> int64_t block_info__cmp(struct perf_hpp_fmt *fmt __maybe_unused,
>                          struct hist_entry *left, struct hist_entry *right)
> {
>          struct block_info *bi_l = left->block_info;
>          struct block_info *bi_r = right->block_info;
>          int cmp;
> .
> .
> .
> 
> Which look a bit more complete, can you check if that can be used
> instead or explain why my quick analysis of this is b0rken?
> 
> Perhaps we can have a __block_info__cmp() that doesn't receive the
> perf_hpp_fmt (that isn't even used above) so that the previous use of
> block_pair_cmp() can be replaced with __block_info__cmp() instead?
> 
> Thanks,
> 
> - Arnaldo
> 

Hi Arnaldo,

I think it's a good idea to use __block_info__cmp() to replace the 
block_pair_cmp(). The block_info__cmp() is more comprehensive than 
block_pair_cmp() and we only need a bit change such as,

--- a/tools/perf/util/block-info.c
+++ b/tools/perf/util/block-info.c
@@ -74,7 +74,7 @@ int64_t block_info__cmp(struct perf_hpp_fmt *fmt 
__maybe_unused,

         if (!bi_l->sym || !bi_r->sym) {
                 if (!bi_l->sym && !bi_r->sym)
-                       return 0;
+                       return -1;
                 else if (!bi_l->sym)
                         return -1;
                 else

It returns -1 if both syms are NULL.

I will update the patchset.

Thanks
Jin Yao

>> +{
>> +	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;
>> +}
>> diff --git a/tools/perf/util/block-info.h b/tools/perf/util/block-info.h
>> index bef0d75e9819..4fa91eeae92e 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 *a, struct hist_entry *b);
>> +
>>   #endif /* __PERF_BLOCK_H */
>> -- 
>> 2.17.1
>>
>