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@xxxxxxxxxxxxxxx>
> > ---
> >   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