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

[PATCH v4 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:
 ---
 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 v4 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:
 ---
 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 v4 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.

 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  | 25 ++++++++++++---
 tools/perf/util/block-info.c | 60 ++++++++++++++++++++++++++----------
 tools/perf/util/block-info.h | 10 ++++--
 3 files changed, 71 insertions(+), 24 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 2d0929aa0a65..266b2ba3ccad 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,16 +444,27 @@ 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)
+			       struct annotation_options *annotation_opts,
+			       bool release)
 {
 	int ret;
 
@@ -451,13 +473,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..77a19e83d146 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,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 v4 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:
 ---
 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 266b2ba3ccad..54eba9decc9b 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 v4 3/4] perf util: Flexible to set block info output formats

[Jiri Olsa]

On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:

SNIP

>  }
>  
> +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)
> +			       struct annotation_options *annotation_opts,
> +			       bool release)
>  {
>  	int ret;
>  
> @@ -451,13 +473,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;

I don't understand this change.. why do you relase it in here?
there's release call in block_info__free_report you just added...

jirka

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

[Jiri Olsa]

On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:

SNIP

> +			       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)

hum, should be '>=' above.. ?

jirka

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

[Jin, Yao]

On 1/20/2020 5:47 PM, Jiri Olsa wrote:
> On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:
> 
> SNIP
> 
>>   }
>>   
>> +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)
>> +			       struct annotation_options *annotation_opts,
>> +			       bool release)
>>   {
>>   	int ret;
>>   
>> @@ -451,13 +473,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;
> 
> I don't understand this change.. why do you relase it in here?
> there's release call in block_info__free_report you just added...
> 
> jirka
> 

Yes, I have released the hists entries in block_info__free_report(). I 
don't need to keep the release code here. Thanks so much for pointing 
this out.

Thanks
Jin Yao

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

[Jin, Yao]

On 1/20/2020 5:47 PM, Jiri Olsa wrote:
> On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:
> 
> SNIP
> 
>> +			       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)
> 
> hum, should be '>=' above.. ?
> 
> jirka
> 

'=' should be OK.

enum {
	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,
	PERF_HPP_REPORT__BLOCK_MAX_INDEX
};

PERF_HPP_REPORT__BLOCK_MAX_INDEX is 6.

If nr_hpps is 6, for example, block_hpps[] is,

		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,
		};

		block_info__create_report(session->evlist,
					  rep->total_cycles,
                                           block_hpps, 6,
                                           &rep->nr_block_reports);

That should be legal.

Thanks
Jin Yao

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

[Jin, Yao]

On 1/20/2020 11:00 PM, Jin, Yao wrote:
> 
> 
> On 1/20/2020 5:47 PM, Jiri Olsa wrote:
>> On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:
>>
>> SNIP
>>
>>> +                   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)
>>
>> hum, should be '>=' above.. ?
>>
>> jirka
>>
> 
> '=' should be OK.
> 
> enum {
>      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,
>      PERF_HPP_REPORT__BLOCK_MAX_INDEX
> };
> 
> PERF_HPP_REPORT__BLOCK_MAX_INDEX is 6.
> 
> If nr_hpps is 6, for example, block_hpps[] is,
> 
>          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,
>          };
> 
>          block_info__create_report(session->evlist,
>                        rep->total_cycles,
>                                            block_hpps, 6,
>                                            &rep->nr_block_reports);
> 
> That should be legal.
> 
> Thanks
> Jin Yao

Hi Jiri,

Does this explanation make sense?

Thanks
Jin Yao

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

[Jiri Olsa]

On Mon, Jan 27, 2020 at 08:31:23PM +0800, Jin, Yao wrote:
> 
> 
> On 1/20/2020 11:00 PM, Jin, Yao wrote:
> > 
> > 
> > On 1/20/2020 5:47 PM, Jiri Olsa wrote:
> > > On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:
> > > 
> > > SNIP
> > > 
> > > > +                   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)
> > > 
> > > hum, should be '>=' above.. ?
> > > 
> > > jirka
> > > 
> > 
> > '=' should be OK.
> > 
> > enum {
> >      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,
> >      PERF_HPP_REPORT__BLOCK_MAX_INDEX
> > };
> > 
> > PERF_HPP_REPORT__BLOCK_MAX_INDEX is 6.
> > 
> > If nr_hpps is 6, for example, block_hpps[] is,
> > 
> >          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,
> >          };
> > 
> >          block_info__create_report(session->evlist,
> >                        rep->total_cycles,
> >                                            block_hpps, 6,
> >                                            &rep->nr_block_reports);
> > 
> > That should be legal.
> > 
> > Thanks
> > Jin Yao
> 
> Hi Jiri,
> 
> Does this explanation make sense?

ok, make sense

thanks,
jirka

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

[Jin, Yao]

On 1/28/2020 5:21 PM, Jiri Olsa wrote:
> On Mon, Jan 27, 2020 at 08:31:23PM +0800, Jin, Yao wrote:
>>
>>
>> On 1/20/2020 11:00 PM, Jin, Yao wrote:
>>>
>>>
>>> On 1/20/2020 5:47 PM, Jiri Olsa wrote:
>>>> On Thu, Jan 16, 2020 at 03:29:03AM +0800, Jin Yao wrote:
>>>>
>>>> SNIP
>>>>
>>>>> +                   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)
>>>>
>>>> hum, should be '>=' above.. ?
>>>>
>>>> jirka
>>>>
>>>
>>> '=' should be OK.
>>>
>>> enum {
>>>       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,
>>>       PERF_HPP_REPORT__BLOCK_MAX_INDEX
>>> };
>>>
>>> PERF_HPP_REPORT__BLOCK_MAX_INDEX is 6.
>>>
>>> If nr_hpps is 6, for example, block_hpps[] is,
>>>
>>>           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,
>>>           };
>>>
>>>           block_info__create_report(session->evlist,
>>>                         rep->total_cycles,
>>>                                             block_hpps, 6,
>>>                                             &rep->nr_block_reports);
>>>
>>> That should be legal.
>>>
>>> Thanks
>>> Jin Yao
>>
>> Hi Jiri,
>>
>> Does this explanation make sense?
> 
> ok, make sense
> 
> thanks,
> jirka
> 

Thanks Jiri!

I will post v5 which only contains the change of dropping the 
unnecessary release code.

Thanks
Jin Yao