| 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: <cbbc3714-3f5e-41f3-8f2a-0a3ec95552c0@linaro.org>
Date: Fri, 1 Nov 2024 11:09:44 +0000
From: James Clark <james.clark@...aro.org>
To: Ian Rogers <irogers@...gle.com>
Cc: linux-perf-users@...r.kernel.org, acme@...nel.org, namhyung@...nel.org,
tim.c.chen@...ux.intel.com, Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>,
"Liang, Kan" <kan.liang@...ux.intel.com>,
Yicong Yang <yangyicong@...ilicon.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/2] perf stat: Fix trailing comma when there is no
metric unit
On 31/10/2024 4:17 am, Ian Rogers wrote:
> On Wed, Oct 30, 2024 at 4:40 AM James Clark <james.clark@...aro.org> wrote:
>>
>> Now that printing metric-value and metric-unit is optional,
>> print_running_json() shouldn't add the comma in case it becomes
>> trailing.
>>
>> Replace all manual json comma stuff with a json_out() function that uses
>> the existing os->first tracking and auto inserts a comma if it's needed.
>> Update the test to handle that two of the fields can be missing.
>
> Thanks for the larger clean up!
>
>> This fixes the following test failure on Cortex A57 where the branch
>> misses metric is missing a required event:
>>
>> $ perf test -vvv "json output"
>>
>> 106: perf stat JSON output linter:
>> --- start ---
>> test child forked, pid 665682
>> Checking json output: no args Test failed for input:
>> ...
>> {"counter-value" : "3112.000000", "unit" : "",
>> "event" : "armv8_pmuv3_1/branch-misses/",
>> "event-runtime" : 20699340, "pcnt-running" : 100.00, }
>> ...
>> json.decoder.JSONDecodeError: Expecting property name enclosed in
>> double quotes: line 12 column 144 (char 2109)
>> ---- end(-1) ----
>> 106: perf stat JSON output linter : FAILED!
>>
>> Fixes: e1cc918b6cfd ("perf stat: Drop metric-unit if unit is NULL")
>> Signed-off-by: James Clark <james.clark@...aro.org>
>> ---
>> .../tests/shell/lib/perf_json_output_lint.py | 14 +-
>> tools/perf/util/stat-display.c | 177 ++++++++++--------
>> 2 files changed, 104 insertions(+), 87 deletions(-)
>>
>> diff --git a/tools/perf/tests/shell/lib/perf_json_output_lint.py b/tools/perf/tests/shell/lib/perf_json_output_lint.py
>> index 8ddb85586131..b066d721f897 100644
>> --- a/tools/perf/tests/shell/lib/perf_json_output_lint.py
>> +++ b/tools/perf/tests/shell/lib/perf_json_output_lint.py
>> @@ -69,16 +69,16 @@ def check_json_output(expected_items):
>> for item in json.loads(input):
>> if expected_items != -1:
>> count = len(item)
>> - if count != expected_items and count >= 1 and count <= 7 and 'metric-value' in item:
>> + if count not in expected_items and count >= 1 and count <= 7 and 'metric-value' in item:
>> # Events that generate >1 metric may have isolated metric
>> # values and possibly other prefixes like interval, core,
>> # aggregate-number, or event-runtime/pcnt-running from multiplexing.
>> pass
>> - elif count != expected_items and count >= 1 and count <= 5 and 'metricgroup' in item:
>> + elif count not in expected_items and count >= 1 and count <= 5 and 'metricgroup' in item:
>> pass
>> - elif count == expected_items + 1 and 'metric-threshold' in item:
>> + elif count - 1 in expected_items and 'metric-threshold' in item:
>> pass
>> - elif count != expected_items:
>> + elif count not in expected_items:
>> raise RuntimeError(f'wrong number of fields. counted {count} expected {expected_items}'
>> f' in \'{item}\'')
>> for key, value in item.items():
>> @@ -90,11 +90,11 @@ def check_json_output(expected_items):
>>
>> try:
>> if args.no_args or args.system_wide or args.event:
>> - expected_items = 7
>> + expected_items = [5, 7]
>> elif args.interval or args.per_thread or args.system_wide_no_aggr:
>> - expected_items = 8
>> + expected_items = [6, 8]
>> elif args.per_core or args.per_socket or args.per_node or args.per_die or args.per_cluster or args.per_cache:
>> - expected_items = 9
>> + expected_items = [7, 9]
>> else:
>> # If no option is specified, don't check the number of items.
>> expected_items = -1
>> diff --git a/tools/perf/util/stat-display.c b/tools/perf/util/stat-display.c
>> index 53dcdf07f5a2..a5d72f4a515c 100644
>> --- a/tools/perf/util/stat-display.c
>> +++ b/tools/perf/util/stat-display.c
>> @@ -114,23 +114,44 @@ static void print_running_csv(struct perf_stat_config *config, u64 run, u64 ena)
>> fprintf(config->output, "%s%" PRIu64 "%s%.2f",
>> config->csv_sep, run, config->csv_sep, enabled_percent);
>> }
>> +struct outstate {
>> + FILE *fh;
>> + bool newline;
>> + bool first;
>
> It'd be nice to have kernel-doc capturing the meaning of these
> variables. newline and first, why would something be a newline but not
> first? I know the lack of documentation is a pre-existing condition.
> Pretty much every variable in the struct below confuses me and I need
> to read the code to try to figure it out.
>
>> + const char *prefix;
>
> Prefix of what?
>
>> + int nfields;
>
> Is this the number of columns in CSV format? Why not a name like
> csv_columns then? What is a field here?
>
>> + int aggr_nr;
>> + struct aggr_cpu_id id;
>
> Something to do with aggregation, presumably for the current CSV line.
> Why two of them?
>
>> + struct evsel *evsel;
>> + struct cgroup *cgrp;
>
> Maybe the counter and cgroup being printed, but we usually pass those
> as extra arguments. This loses me.
>
> I was hoping the code here could be more like the perf list json:
> https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/builtin-list.c?h=perf-tools-next#n357
> which avoids the comma problem by printing everything in one go.
> There's so much spaghetti code in stat-display and before we had tests
> there were frequent breakages. Anyway, I don't expect a larger clean
> up, just venting. If you could do the comments and clear up the
> newline vs first it'd be great.
>
> Thanks,
> Ian
>
Yep I can do that.
Thanks for the review.
Powered by blists - more mailing lists