Re: [PATCH v7 1/1] perf jevents: Check the keys of the JSON tables

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] perf test: jevents_check.sh never checks in-tree PMU JSON
  tables
- [Low] perf jevents: _check_tables() ignores mandatory arch and model
  arguments
- [Low] perf test: jevents_check.sh hardcodes python3 without
  availability check
--

commit 59714c85104e0be8e44b8587ef592e9df4d4a604
Author: TAKANO Mitsuhiro <takano32@gmail.com>

perf jevents: Check the keys of the JSON tables

This commit adds a --check option to jevents.py to report JSON event table
keys not in _TABLE_KEYS, along with a shell test for the option.

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 080e04b51b8c5..397b4fc977497 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
[ ... ]
> @@ -1450,6 +1451,38 @@ def _init_worker(std_events: dict) -> None:
[ ... ]
> +def _check_tables(root: str) -> int:
> +  """Report the keys in the tables under root that a table may not carry."""
> +  bad = 0
> +  for path in sorted(Path(root).rglob('*.json')):
> +    if path.name.endswith('metricgroups.json'):
> +      # A dictionary keyed by metric group name, not a list of events.
> +      continue
> +    for obj in json.loads(path.read_text(encoding='utf-8')):
> +      if extra := set(obj) - _TABLE_KEYS:
> +        print(f'{path}: {sorted(extra)}')
> +        bad += 1
> +  return 1 if bad else 0
> +
>  def main() -> None:
>    global _args
>  
> @@ -1494,8 +1527,13 @@ such as "arm/cortex-a34".''',
>        'output_file', type=argparse.FileType('w', encoding='utf-8'), nargs='?', default=sys.stdout)
>    ap.add_argument(
>        'output_string_file', type=argparse.FileType('w', encoding='utf-8'), nargs='?', default=None)
> +  ap.add_argument('--check', action='store_true',
> +                  help='check the keys of the tables under starting_dir and exit')
>    _args = ap.parse_args()
>  
> +  if _args.check:
> +    sys.exit(_check_tables(_args.starting_dir))

[Severity: Low]
Why does --check still require positional arch and model arguments on the
command line if _check_tables() ignores _args.arch and _args.model and walks
every JSON file under starting_dir?  Callers such as jevents_check.sh have
to pass dummy values like "x86 all" before starting_dir that have no effect
on the check.

> diff --git a/tools/perf/tests/shell/jevents_check.sh b/tools/perf/tests/shell/jevents_check.sh
> new file mode 100755
> index 0000000000000..eb3125e1486ab
> --- /dev/null
> +++ b/tools/perf/tests/shell/jevents_check.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +# jevents_check: an unknown key in a table is reported (no_mlock)
> +#
> +# A key that is spelled differently is dropped without a word, so the keys of
> +# the tables are checked against the list jevents.py reads.
> +
> +jevents=$(dirname "$0")/../../pmu-events/jevents.py
> +[ -r "${jevents}" ] || exit 2
> +
> +tmp=$(mktemp -d)
> +trap 'rm -rf "${tmp}"' EXIT
> +tables="${tmp}/arch/x86/model"
> +mkdir -p "${tables}"
> +
> +# Known keys pass.
> +printf '[{"EventName": "a", "EventCode": "0x1"}]\n' > "${tables}/events.json"
> +python3 "${jevents}" x86 all "${tmp}/arch" --check > /dev/null || exit 1

[Severity: Low]
Will jevents_check.sh fail with exit code 1 instead of skipping with exit
code 2 on systems where python3 is not installed or where a custom
interpreter is configured via PYTHON?  Sourcing lib/setup_python.sh and
invoking $PYTHON would skip cleanly when Python is unavailable.

> +
> +# A key the generator does not read is reported.
> +printf '[{"EventName": "a", "NotAKey": "x"}]\n' > "${tables}/events.json"
> +python3 "${jevents}" x86 all "${tmp}/arch" --check > /dev/null && exit 1

[Severity: Medium]
Does jevents_check.sh ever run jevents.py --check against the in-tree JSON
tables under tools/perf/pmu-events/arch?  The script only tests two
synthetic one-line files in ${tmp}/arch, so neither the build nor perf test
validates the repository's PMU event tables or checks that _TABLE_KEYS stays
in sync with them.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112162823.139419-1-takano32@gmail.com?part=1