Messages in this thread |  | | From | Dmitry Vyukov <> | | Date | Mon, 13 Jan 2025 16:39:57 +0100 | | Subject | Re: [PATCH] perf report: Fix input reload/switch functionality |
| |
On Mon, 13 Jan 2025 at 16:19, James Clark <james.clark@linaro.org> wrote: > > On 10/01/2025 12:19 pm, Dmitry Vyukov wrote: > > On Thu, 9 Jan 2025 at 15:50, James Clark <james.clark@linaro.org> wrote: > >> On 08/01/2025 3:35 pm, Dmitry Vyukov wrote: > >>> On Wed, 8 Jan 2025 at 16:23, James Clark <james.clark@linaro.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 > > By 'without existing' do you mean running perf report with no perf.data > file? It won't run like that it just says "failed to open perf.data" > > > file has changed, I guess you can also reload it and see the changes. > > > > I wasn't able to make that work. How do you reload? If I change screen > between different events I still get the old file and I didn't see > 'reload' mentioned in the keybinding popup. > > I suppose my point is maybe it's not worth fixing the sort order bug if > we can just remove input file switching. The behavior doesn't seem any > different to v5.8 in case its a regression. Maybe I'm just doing > something wrong though.
Humm... indeed, it does not seem to actually reload anything (even though the "Processing events..." progress bar progresses again).
I tried to use the reloading feature while implementing:
perf report: Add wall-clock and parallelism profiling https://lore.kernel.org/linux-perf-users/20250113134022.2545894-1-dvyukov@google.com/
to change some of the flags that can't be changed otherwise, but then dropped this idea since it was too messy, but decided to fix the failure I discovered.
So, yes, I guess we can drop this patch (I don't need it anymore).
> >>>>> Signed-off-by: Dmitry Vyukov <dvyukov@google.com> > >>>>> Fixes: ec6ae74fe8f0 ("perf report: Display average IPC and IPC coverage per symbol") > >>>> > Cc: Namhyung Kim <namhyung@kernel.org>> Cc: Ian Rogers > >>>> <irogers@google.com> > >>>>> Cc: linux-perf-users@vger.kernel.org > >>>>> Cc: linux-kernel@vger.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 > >>>> > >> >
|  |