Re: [PATCH 1/3] perf tools: Fix period/freq terms setup

From: Stephane Eranian <eranian@google.com>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Jiri Olsa <jolsa@kernel.org>, lkml <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Namhyung Kim <namhyung@kernel.org>,
	David Ahern <dsahern@gmail.com>, Andi Kleen <ak@linux.intel.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	Peter Zijlstra <a.p.zijlstra@chello.nl>
Subject: Re: [PATCH 1/3] perf tools: Fix period/freq terms setup
Date: Wed, 7 Feb 2018 10:52:35 -0800	[thread overview]
Message-ID: <CABPqkBTXhL1U0HBbc2J0mdpGDiC2LG=Cm8MN+Pp_K=a_euDuUg@mail.gmail.com> (raw)
In-Reply-To: <20180206093553.GA8065@krava>

On Tue, Feb 6, 2018 at 1:35 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Mon, Feb 05, 2018 at 06:51:05PM -0800, Stephane Eranian wrote:
>
> SNIP
>
>> >
>> Looks like this is working then, great!
>>
>> Now, related to profiling and reporting. There is still an issue I
>> keep running into
>> with grouping. I want to sample on N events, where N > number of  hw counters.
>> Yet I want the same output as perf report --group, i.e., side-by-side
>> profiles as
>> opposed to showing me one event profile at a time (which is not very useful).
>>
>> You should not require events to belong to the same group to support this. Many
>> other tools support such output (e.g., VTUNE, Gooda). It is still very
>> valuable even
>> though events may not have been measured at the same time.
>>
>> Let me use a simple (and silly but portable) example.
>> Today if I do on  Intel x86:
>>
>>  $ perf record -e branches,branches,branches,branches,branches my_test
>>
>> And I do:
>>
>> $ perf report --group
>> It will show me 5 distinct profiles.
>>
>> I would like perf to show me a single profile where the 5 events are
>> side-by-side.
>>
>> Similar to what I get if I do instead:
>> $ perf record -e '{branches,branches,branches,branches}' my_test
>> $ perf report --group
>>
>> But here, I would have to ensure all events fits in a group to allow
>> the reporting
>> I want. So that would limit me to 4 events.
>>
>> I think perf report --group should work regardless of how the events
>> were grouped.
>> Is there already a way to work around this?
>
> no workaround.. please try attached patch, it seems
> to work for what you described
>
Works for me. That's great!
Thanks.

Tested-By: Stephane Eranian <eranian@google.com>

> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 4ad5dc649716..35a013992092 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -937,6 +937,7 @@ int cmd_report(int argc, const char **argv)
>                 "perf report [<options>]",
>                 NULL
>         };
> +       bool group_set = false;
>         struct report report = {
>                 .tool = {
>                         .sample          = process_sample_event,
> @@ -1056,7 +1057,7 @@ int cmd_report(int argc, const char **argv)
>                    "Specify disassembler style (e.g. -M intel for intel syntax)"),
>         OPT_BOOLEAN(0, "show-total-period", &symbol_conf.show_total_period,
>                     "Show a column with the sum of periods"),
> -       OPT_BOOLEAN(0, "group", &symbol_conf.event_group,
> +       OPT_BOOLEAN_SET(0, "group", &symbol_conf.event_group, &group_set,
>                     "Show event group information together"),
>         OPT_CALLBACK_NOOPT('b', "branch-stack", &branch_mode, "",
>                     "use branch records for per branch histogram filling",
> @@ -1173,6 +1174,9 @@ int cmd_report(int argc, const char **argv)
>         has_br_stack = perf_header__has_feat(&session->header,
>                                              HEADER_BRANCH_STACK);
>
> +       if (group_set && !session->evlist->nr_groups)
> +               perf_evlist__set_leader(session->evlist);
> +
>         if (itrace_synth_opts.last_branch)
>                 has_br_stack = true;
>