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

[James Clark]

On 08/01/2025 3:35 pm, Dmitry Vyukov wrote:
> On Wed, 8 Jan 2025 at 16:23, James Clark <james.clark@xxxxxxxxxx> 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?

> > > Signed-off-by: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> > > Fixes: ec6ae74fe8f0 ("perf report: Display average IPC and IPC coverage per symbol")
> >   > Cc: Namhyung Kim <namhyung@xxxxxxxxxx>> Cc: Ian Rogers
> > <irogers@xxxxxxxxxx>
> > > Cc: linux-perf-users@xxxxxxxxxxxxxxx
> > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > ---
> > >    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