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: <89e678bc-f0af-4929-a8a6-a2666f1294a4@linaro.org>
Date: Thu, 9 Jan 2025 14:50:48 +0000
From: James Clark <james.clark@...aro.org>
To: Dmitry Vyukov <dvyukov@...gle.com>
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 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?

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