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

Message-ID: <CACT4Y+bUrZHD5H+z5-+dKGbtBPO9EQNzBoOwYGhp7zHGrHYrQg@mail.gmail.com>
Date: Wed, 8 Jan 2025 16:35:15 +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 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.


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