| 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
| ||
|
Message-ID: <CACT4Y+b8X7PnUwQtuU2QXSqumNDbN8pWDm8EX+wnvgNAKbW0xw@mail.gmail.com>
Date: Mon, 13 Jan 2025 16:39:57 +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 Mon, 13 Jan 2025 at 16:19, James Clark <james.clark@...aro.org> wrote:
>
> On 10/01/2025 12:19 pm, Dmitry Vyukov wrote:
> > 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
>
> 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@...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
> >>>>
> >>
>
Powered by blists - more mailing lists