linux-kernel - Re: [PATCH 1/9] perf report: Fix --total-cycles --stdio output error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM9d7cj1hs-Y9RYPEi050hXYRJ-4Y62KvKb1hf_oPfaa+Y3LLg@mail.gmail.com>
Date: Fri, 2 Aug 2024 13:25:27 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: kan.liang@...ux.intel.com
Cc: acme@...nel.org, irogers@...gle.com, peterz@...radead.org, 
	mingo@...nel.org, linux-kernel@...r.kernel.org, adrian.hunter@...el.com, 
	ak@...ux.intel.com, eranian@...gle.com
Subject: Re: [PATCH 1/9] perf report: Fix --total-cycles --stdio output error

On Wed, Jul 3, 2024 at 1:03 PM <kan.liang@...ux.intel.com> wrote:
>
> From: Kan Liang <kan.liang@...ux.intel.com>
>
> The --total-cycles may output wrong information with the --stdio.
>
> For example,
>   perf record -e "{cycles,instructions}",cache-misses -b sleep 1
>   perf report --total-cycles --stdio
>
> The total cycles output of {cycles,instructions} and cache-misses are
> almost the same.
>
>  # Samples: 938  of events 'anon group { cycles, instructions }'
>  # Event count (approx.): 938
>  #
>  # Sampled Cycles%  Sampled Cycles  Avg Cycles%  Avg Cycles
>  # ...............  ..............  ...........  ..........
>   ..................................................>
>  #
>            11.19%            2.6K        0.10%          21
>                           [perf_iterate_ctx+48 -> >
>             5.79%            1.4K        0.45%          97
>             [__intel_pmu_enable_all.constprop.0+80 -> __intel_>
>             5.11%            1.2K        0.33%          71
>                              [native_write_msr+0 ->>
>
>  # Samples: 293  of event 'cache-misses'
>  # Event count (approx.): 293
>  #
>  # Sampled Cycles%  Sampled Cycles  Avg Cycles%  Avg Cycles
>                                                   [>
>  # ...............  ..............  ...........  ..........
>    ..................................................>
>  #
>            11.19%            2.6K        0.13%          21
>                           [perf_iterate_ctx+48 -> >
>             5.79%            1.4K        0.59%          97
> [__intel_pmu_enable_all.constprop.0+80 -> __intel_>
>             5.11%            1.2K        0.43%          71
>                              [native_write_msr+0 ->>
>
> With the symbol_conf.event_group, the perf report should only report the
> block information of the leader event in a group.
> However, the current implementation retrieves the next event's block
> information, rather than the next group leader's block information.
>
> Make sure the index is updated even if the event is skipped.
>
> With the patch,
>
>  # Samples: 293  of event 'cache-misses'
>  # Event count (approx.): 293
>  #
>  # Sampled Cycles%  Sampled Cycles  Avg Cycles%  Avg Cycles
>                                                   [>
>  # ...............  ..............  ...........  ..........
>    ..................................................>
>  #
>            37.98%            9.0K        4.05%         299
>    [perf_event_addr_filters_exec+0 -> perf_event_a>
>            11.19%            2.6K        0.28%          21
>                           [perf_iterate_ctx+48 -> >
>             5.79%            1.4K        1.32%          97
> [__intel_pmu_enable_all.constprop.0+80 -> __intel_>
>
> Fixes: 6f7164fa231a ("perf report: Sort by sampled cycles percent per block for stdio")
> Signed-off-by: Kan Liang <kan.liang@...ux.intel.com>

Acked-by: Namhyung Kim <namhyung@...nel.org>

Thanks,
Namhyung

> ---
>  tools/perf/builtin-report.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 9718770facb5..b9f22c5321da 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -565,6 +565,7 @@ static int evlist__tty_browse_hists(struct evlist *evlist, struct report *rep, c
>                 struct hists *hists = evsel__hists(pos);
>                 const char *evname = evsel__name(pos);
>
> +               i++;
>                 if (symbol_conf.event_group && !evsel__is_group_leader(pos))
>                         continue;
>
> @@ -574,7 +575,7 @@ static int evlist__tty_browse_hists(struct evlist *evlist, struct report *rep, c
>                 hists__fprintf_nr_sample_events(hists, rep, evname, stdout);
>
>                 if (rep->total_cycles_mode) {
> -                       report__browse_block_hists(&rep->block_reports[i++].hist,
> +                       report__browse_block_hists(&rep->block_reports[i - 1].hist,
>                                                    rep->min_percent, pos, NULL);
>                         continue;
>                 }
> --
> 2.38.1
>