linux-kernel - Re: [PATCH] perf report: Fix input reload/switch functionality

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] [day] [month] [year] [list]

Message-ID: <CACT4Y+ayg0oo0sX54DuJ9n+L4MJJU5LsEJtEDS13Zu7ifOyq3g@mail.gmail.com>
Date: Fri, 10 Jan 2025 13:19:16 +0100
From: Dmitry Vyukov <dvyukov@...gle.com>
To: James Clark <james.clark@...aro.org>
Cc: linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org, 
	namhyung@...nel.org, irogers@...gle.com
Subject: Re: [PATCH] perf report: Fix input reload/switch functionality

On Thu, 9 Jan 2025 at 15:50, James Clark <james.clark@...aro.org> wrote:
> On 08/01/2025 3:35 pm, Dmitry Vyukov wrote:
> > On Wed, 8 Jan 2025 at 16:23, James Clark <james.clark@...aro.org> wrote:
> >>
> >> On 08/01/2025 6:36 am, Dmitry Vyukov wrote:
> >>> Currently the code checks that there is no "ipc" in the sort order
> >>> and add an ipc string. This will always error out on the second pass
> >>> after input reload/switch, since the sort order already contains "ipc".
> >>> Do the ipc check/fixup only on the first pass.
> >>>
> >>
> >> Hi Dmitry,
> >>
> >> A reproducer with before and after behavior might be helpful for the review.
> >>
> >> It might be unrelated to your change here, but the input switch thing
> >> didn't seem to do anything for me. If I record two files, open the first
> >> one, press S and select the second file nothing seems to change. I
> >> assumed it would show the other file but nothing changes?
> >>
> >>    $ perf record -- true
> >>    $ perf record -o 2.data -- ls
> >>    $ perf report
> >>    S key
> >>    load 2.data
> >
> > Yes, sure. This needs "--sort symbol":
> >
> > perf report --sort symbol
> >
> > then press 's', select file, and it terminates.
> >
> > Affects fewer cases then I initially assumed, since I happened to run
> > with "--sort symbol" when I discovered it.
> >
> >
>
> Ah yeah I can reproduce the bug with --sort, which is fixed by the
> patch. But 's' doesn't actually reload a new histogram for me, I just
> get the original file again. Maybe I'm misunderstanding what 's' is
> supposed to do?

As far as I understand, 's' allows you to select and load a new
profile w/o existing (fwiw, if you select a different file). If the
file has changed, I guess you can also reload it and see the changes.


> >>> Signed-off-by: Dmitry Vyukov <dvyukov@...gle.com>
> >>> Fixes: ec6ae74fe8f0 ("perf report: Display average IPC and IPC coverage per symbol")
> >>   > Cc: Namhyung Kim <namhyung@...nel.org>> Cc: Ian Rogers
> >> <irogers@...gle.com>
> >>> Cc: linux-perf-users@...r.kernel.org
> >>> Cc: linux-kernel@...r.kernel.org
> >>> ---
> >>>    tools/perf/builtin-report.c | 30 ++++++++++++++++--------------
> >>>    1 file changed, 16 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> >>> index 048c91960ba91..42d7dfdf07d9b 100644
> >>> --- a/tools/perf/builtin-report.c
> >>> +++ b/tools/perf/builtin-report.c
> >>> @@ -1721,22 +1721,24 @@ int cmd_report(int argc, const char **argv)
> >>>                symbol_conf.annotate_data_sample = true;
> >>>        }
> >>>
> >>> -     if (sort_order && strstr(sort_order, "ipc")) {
> >>> -             parse_options_usage(report_usage, options, "s", 1);
> >>> -             goto error;
> >>> -     }
> >>> -
> >>> -     if (sort_order && strstr(sort_order, "symbol")) {
> >>> -             if (sort__mode == SORT_MODE__BRANCH) {
> >>> -                     snprintf(sort_tmp, sizeof(sort_tmp), "%s,%s",
> >>> -                              sort_order, "ipc_lbr");
> >>> -                     report.symbol_ipc = true;
> >>> -             } else {
> >>> -                     snprintf(sort_tmp, sizeof(sort_tmp), "%s,%s",
> >>> -                              sort_order, "ipc_null");
> >>> +     if (last_key != K_SWITCH_INPUT_DATA) {
> >>> +             if (sort_order && strstr(sort_order, "ipc")) {
> >>> +                     parse_options_usage(report_usage, options, "s", 1);
> >>> +                     goto error;
> >>>                }
> >>>
> >>> -             sort_order = sort_tmp;
> >>> +             if (sort_order && strstr(sort_order, "symbol")) {
> >>> +                     if (sort__mode == SORT_MODE__BRANCH) {
> >>> +                             snprintf(sort_tmp, sizeof(sort_tmp), "%s,%s",
> >>> +                                      sort_order, "ipc_lbr");
> >>> +                             report.symbol_ipc = true;
> >>> +                     } else {
> >>> +                             snprintf(sort_tmp, sizeof(sort_tmp), "%s,%s",
> >>> +                                      sort_order, "ipc_null");
> >>> +                     }
> >>> +
> >>> +                     sort_order = sort_tmp;
> >>> +             }
> >>>        }
> >>>
> >>>        if ((last_key != K_SWITCH_INPUT_DATA && last_key != K_RELOAD) &&
> >>>
> >>> base-commit: 09a0fa92e5b45e99cf435b2fbf5ebcf889cf8780
> >>
>