[PATCH v1 0/3] perf jevents: Deterministic build fix and mypy cleanliness

[PATCH v1 0/3] perf jevents: Deterministic build fix and mypy cleanliness

[Ian Rogers]

Nazar Kazakov reported non-deterministic builds due to the metrics
being reordered in the jevents.py output. Fix by sorting metrics on
more than just their name.

Checking the code with mypy showed a large range of type warnings. Fix
by largely adding asserts that values aren't None and by adding type
annotations.

Ian Rogers (3):
  perf jevents: Add more components to the metric sorting order
  perf jevents: Add python type annotations
  perf jevents metric: Add python type annotations

 tools/perf/pmu-events/jevents.py | 82 +++++++++++++++++++-------------
 tools/perf/pmu-events/metric.py  | 23 +++++----
 2 files changed, 62 insertions(+), 43 deletions(-)

-- 
2.55.0.795.g602f6c329a-goog

[PATCH v1 1/3] perf jevents: Add more components to the metric sorting order

[Ian Rogers]

Nazar Kazakov reported non-deterministic builds due to the metrics
being reordered in the jevents.py output. The metrics were largely
only being sorted by name, add in the expressions and descriptions.

Reported-by: Nazar Kazakov <nazar.kazakov@codethink.co.uk>
Closes: https://lore.kernel.org/linux-perf-users/20260706175624.692736-1-nazar.kazakov@codethink.co.uk/
Fixes: 40769665b63d ("perf jevents: Parse metrics during conversion")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/jevents.py | 5 +++--
 tools/perf/pmu-events/metric.py  | 6 +++++-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 376dc2d24162..3c6cfeefbd5d 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -570,13 +570,14 @@ static const struct pmu_table_entry {_pending_events_tblname}[] = {{
 def print_pending_metrics() -> None:
   """Optionally close metrics table."""
 
-  def metric_cmp_key(j: JsonEvent) -> Tuple[bool, str, str]:
+  def metric_cmp_key(j: JsonEvent) -> Tuple[str, str, str, str]:
     def fix_none(s: Optional[str]) -> str:
       if s is None:
         return ''
       return s
 
-    return (j.desc is not None, fix_none(j.pmu), fix_none(j.metric_name))
+    return (fix_none(j.pmu), fix_none(j.metric_name), j.metric_expr.ToPerfJson(),
+            fix_none(j.desc))
 
   global _pending_metrics
   if not _pending_metrics:
diff --git a/tools/perf/pmu-events/metric.py b/tools/perf/pmu-events/metric.py
index a91ccb5977f0..11c7162825f4 100644
--- a/tools/perf/pmu-events/metric.py
+++ b/tools/perf/pmu-events/metric.py
@@ -623,7 +623,11 @@ class Metric:
 
   def __lt__(self, other):
     """Sort order."""
-    return self.name < other.name
+    if self.name != other.name:
+      return self.name < other.name
+    if not self.expr.Equals(other.expr):
+      return self.expr.ToPerfJson() < other.expr.ToPerfJson()
+    return self.description < other.description
 
   def AddToMetricGroup(self, group):
     """Callback used when being added to a MetricGroup."""
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v1 1/3] perf jevents: Add more components to the metric sorting order

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Unconditional access of `j.metric_expr.ToPerfJson()` can raise `AttributeError: 'NoneType' object has no attribute 'ToPerfJson'` in `jevents.py`, causing a build failure.
--

--- Patch [1]: [PATCH v1 1/3] perf jevents: Add more components to the metric sorting order ---
commit ef069542148ff8d19abeda05cc13982058eb11d7
Author: Ian Rogers <irogers@google.com>

perf jevents: Add more components to the metric sorting order

This commit modifies the sorting order for metrics in the jevents.py script
by including expressions and descriptions. The goal is to prevent
non-deterministic builds that were occurring when metrics were sorted only
by name.

Closes: https://lore.kernel.org/linux-perf-users/20260706175624.692736-1-nazar.kazakov@codethink.co.uk/

> diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> index 376dc2d241621..3c6cfeefbd5dc 100755
> --- a/tools/perf/pmu-events/jevents.py
> +++ b/tools/perf/pmu-events/jevents.py
> @@ -570,13 +570,14 @@ static const struct pmu_table_entry {_pending_events_tblname}[] = {{
>  def print_pending_metrics() -> None:
>    """Optionally close metrics table."""
>  
> -  def metric_cmp_key(j: JsonEvent) -> Tuple[bool, str, str]:
> +  def metric_cmp_key(j: JsonEvent) -> Tuple[str, str, str, str]:
>      def fix_none(s: Optional[str]) -> str:
>        if s is None:
>          return ''
>        return s
>  
> -    return (j.desc is not None, fix_none(j.pmu), fix_none(j.metric_name))
> +    return (fix_none(j.pmu), fix_none(j.metric_name), j.metric_expr.ToPerfJson(),
> +            fix_none(j.desc))

[Severity: Low]
Will this crash with an AttributeError if an event lacks a metric expression?

If a parsed JSON event specifies a MetricName but omits MetricExpr,
j.metric_expr will be None. Calling j.metric_expr.ToPerfJson() in this case
would cause the build script to fail.

Would it be safer to check if j.metric_expr is None before calling
ToPerfJson()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707034019.241762-1-irogers@google.com?part=1

[PATCH v1 2/3] perf jevents: Add python type annotations

[Ian Rogers]

Make mypy clean.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/jevents.py | 77 +++++++++++++++++++-------------
 1 file changed, 45 insertions(+), 32 deletions(-)

diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 3c6cfeefbd5d..83a239dd69f5 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -14,27 +14,27 @@ import collections
 # Global command line arguments.
 _args = None
 # List of regular event tables.
-_event_tables = []
+_event_tables: list[str] = []
 # List of event tables generated from "/sys" directories.
-_sys_event_tables = []
+_sys_event_tables: list[str] = []
 # List of regular metric tables.
-_metric_tables = []
+_metric_tables: list[str] = []
 # List of metric tables generated from "/sys" directories.
-_sys_metric_tables = []
+_sys_metric_tables: list[str] = []
 # Mapping between sys event table names and sys metric table names.
 _sys_event_table_to_metric_table_mapping = {}
 # Map from an event name to an architecture standard
 # JsonEvent. Architecture standard events are in json files in the top
 # f'{_args.starting_dir}/{_args.arch}' directory.
-_arch_std_events = {}
+_arch_std_events: Dict[str, "JsonEvent"] = {}
 # Events to write out when the table is closed
-_pending_events = []
+_pending_events: list["JsonEvent"] = []
 # Name of events table to be written out
-_pending_events_tblname = None
+_pending_events_tblname: Optional[str] = None
 # Metrics to write out when the table is closed
-_pending_metrics = []
+_pending_metrics: list["JsonEvent"] = []
 # Name of metrics table to be written out
-_pending_metrics_tblname = None
+_pending_metrics_tblname: Optional[str] = None
 # Global BigCString shared by all structures.
 _bcs = None
 # Map from the name of a metric group to a description of the group.
@@ -210,7 +210,7 @@ class JsonEvent:
       """Convert an int to a string similar to a printf modifier of %#llx."""
       return str(x) if x >= 0 and x < 10 else hex(x)
 
-    def fixdesc(s: str) -> str:
+    def fixdesc(s: Optional[str]) -> Optional[str]:
       """Fix formatting issue for the desc string."""
       if s is None:
         return None
@@ -218,7 +218,7 @@ class JsonEvent:
                                        '. '), '.').replace('\n', '\\n').replace(
                                            '\"', '\\"').replace('\r', '\\r')
 
-    def convert_aggr_mode(aggr_mode: str) -> Optional[str]:
+    def convert_aggr_mode(aggr_mode: Optional[str]) -> Optional[str]:
       """Returns the aggr_mode_class enum value associated with the JSON string."""
       if not aggr_mode:
         return None
@@ -228,7 +228,7 @@ class JsonEvent:
       }
       return aggr_mode_to_enum[aggr_mode]
 
-    def convert_metric_constraint(metric_constraint: str) -> Optional[str]:
+    def convert_metric_constraint(metric_constraint: Optional[str]) -> Optional[str]:
       """Returns the metric_event_groups enum value associated with the JSON string."""
       if not metric_constraint:
         return None
@@ -241,7 +241,7 @@ class JsonEvent:
       }
       return metric_constraint_to_enum[metric_constraint]
 
-    def lookup_msr(num: str) -> Optional[str]:
+    def lookup_msr(num: Optional[str]) -> Optional[str]:
       """Converts the msr number, or first in a list to the appropriate event field."""
       if not num:
         return None
@@ -253,7 +253,7 @@ class JsonEvent:
       }
       return msrmap[int(num.split(',', 1)[0], 0)]
 
-    def real_event(name: str, event: str) -> Optional[str]:
+    def real_event(name: Optional[str], event: Optional[str]) -> Optional[str]:
       """Convert well known event names to an event string otherwise use the event argument."""
       fixed = {
           'inst_retired.any': 'event=0xc0,period=2000003',
@@ -269,7 +269,7 @@ class JsonEvent:
         return fixed[name.lower()]
       return event
 
-    def unit_to_pmu(unit: str) -> Optional[str]:
+    def unit_to_pmu(unit: Optional[str]) -> Optional[str]:
       """Convert a JSON Unit to Linux PMU name."""
       if not unit:
         return 'default_core'
@@ -304,21 +304,21 @@ class JsonEvent:
       return table[unit] if unit in table else f'uncore_{unit.lower()}'
 
     def is_zero(val: str) -> bool:
-        try:
-            if val.startswith('0x'):
-                return int(val, 16) == 0
-            else:
-                return int(val) == 0
-        except e:
-            return False
+      try:
+        if val.startswith('0x'):
+          return int(val, 16) == 0
+        else:
+          return int(val) == 0
+      except:
+        return False
 
     def canonicalize_value(val: str) -> str:
-        try:
-            if val.startswith('0x'):
-                return llx(int(val, 16))
-            return str(int(val))
-        except e:
-            return val
+      try:
+        if val.startswith('0x'):
+          return llx(int(val, 16))
+        return str(int(val))
+      except:
+        return val
 
     eventcode = 0
     if 'EventCode' in jd:
@@ -370,7 +370,8 @@ class JsonEvent:
     if precise and self.desc and '(Precise Event)' not in self.desc:
       extra_desc += ' (Must be precise)' if precise == '2' else (' (Precise '
                                                                  'event)')
-    event = None
+    self.event: Optional[str] = None
+    event: Optional[str] = None
     if configcode is not None:
       event = f'config={llx(configcode)}'
     elif eventidcode is not None:
@@ -455,6 +456,7 @@ class JsonEvent:
         return f'\t/* {s} */\n' if len(s) < 80 else f'\t/* {s[0:80]}... */\n'
 
     s = self.build_c_string(metric)
+    assert _bcs is not None
     return f'{make_comment(s)}\t{{ { _bcs.offsets[s] } }},\n'
 
 
@@ -524,6 +526,7 @@ def print_pending_events() -> None:
     return
 
   global _pending_events_tblname
+  assert _pending_events_tblname is not None
   if _pending_events_tblname.endswith('_sys'):
     global _sys_event_tables
     _sys_event_tables.append(_pending_events_tblname)
@@ -534,7 +537,8 @@ def print_pending_events() -> None:
   first = True
   last_pmu = None
   last_name = None
-  pmus = set()
+  pmus: Set[Tuple[str, str]] = set()
+  assert _args is not None
   for event in sorted(_pending_events, key=event_cmp_key):
     if last_pmu and last_pmu == event.pmu:
       assert event.name != last_name, f"Duplicate event: {last_pmu}/{last_name}/ in {_pending_events_tblname}"
@@ -576,6 +580,7 @@ def print_pending_metrics() -> None:
         return ''
       return s
 
+    assert j.metric_expr is not None
     return (fix_none(j.pmu), fix_none(j.metric_name), j.metric_expr.ToPerfJson(),
             fix_none(j.desc))
 
@@ -584,6 +589,7 @@ def print_pending_metrics() -> None:
     return
 
   global _pending_metrics_tblname
+  assert _pending_metrics_tblname is not None
   if _pending_metrics_tblname.endswith('_sys'):
     global _sys_metric_tables
     _sys_metric_tables.append(_pending_metrics_tblname)
@@ -593,7 +599,8 @@ def print_pending_metrics() -> None:
 
   first = True
   last_pmu = None
-  pmus = set()
+  pmus: Set[Tuple[str, str]] = set()
+  assert _args is not None
   for metric in sorted(_pending_metrics, key=metric_cmp_key):
     if metric.pmu != last_pmu:
       if not first:
@@ -629,7 +636,6 @@ def get_topic(topic: str) -> str:
   return removesuffix(topic, '.json').replace('-', ' ')
 
 def preprocess_one_file(parents: Sequence[str], item: os.DirEntry) -> None:
-
   if item.is_dir():
     return
 
@@ -643,6 +649,7 @@ def preprocess_one_file(parents: Sequence[str], item: os.DirEntry) -> None:
   if not item.is_file() or not item.name.endswith('.json'):
     return
 
+  assert _bcs is not None
   if item.name.endswith('metricgroups.json'):
     metricgroup_descriptions = json.load(open(item.path))
     for mgroup in metricgroup_descriptions:
@@ -704,6 +711,7 @@ def process_one_file(parents: Sequence[str], item: os.DirEntry) -> None:
 
 def print_mapping_table(archs: Sequence[str]) -> None:
   """Read the mapfile and generate the struct from cpuid string to event table."""
+  assert _args is not None
   _args.output_file.write("""
 /* Struct used to make the PMU event table implementation opaque to callers. */
 struct pmu_events_table {
@@ -821,6 +829,7 @@ static const struct pmu_events_map pmu_events_map[] = {
 
 
 def print_metric_table_functions() -> None:
+  assert _args is not None
   _args.output_file.write("""
 const char *pmu_metrics_table__name(const struct pmu_metrics_table *table)
 {
@@ -864,6 +873,7 @@ int pmu_metrics_table__iterate_tables(pmu_metrics_table_iter_t fn, void *data)
 
 
 def print_system_mapping_table() -> None:
+  assert _args is not None
   """C struct mapping table array for tables from /sys directories."""
   _args.output_file.write("""
 struct pmu_sys_events {
@@ -1400,6 +1410,7 @@ int pmu_for_each_sys_metric(pmu_metric_iter_fn fn, void *data)
 """)
 
 def print_metricgroups() -> None:
+  assert _args is not None
   _args.output_file.write("""
 static const int metricgroups[][2] = {
 """)
@@ -1443,6 +1454,7 @@ def main() -> None:
   def ftw(path: str, parents: Sequence[str],
           action: Callable[[Sequence[str], os.DirEntry], None]) -> None:
     """Replicate the directory/file walking behavior of C's file tree walk."""
+    assert _args is not None
     for item in sorted(os.scandir(path), key=lambda e: e.name):
       if _args.model != 'all' and item.is_dir():
         # Check if the model matches one in _args.model.
@@ -1513,6 +1525,7 @@ struct pmu_table_entry {
     preprocess_arch_std_files(arch_path)
     ftw(arch_path, [], preprocess_one_file)
 
+  assert _bcs is not None
   _bcs.compute()
   _args.output_file.write('/* clang-format off */\n')
   if not _args.output_string_file:
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v1 3/3] perf jevents metric: Add python type annotations

[Ian Rogers]

Make mypy clean.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/pmu-events/metric.py | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/tools/perf/pmu-events/metric.py b/tools/perf/pmu-events/metric.py
index 11c7162825f4..ce025675898c 100644
--- a/tools/perf/pmu-events/metric.py
+++ b/tools/perf/pmu-events/metric.py
@@ -276,7 +276,7 @@ class Operator(Expression):
     lhs = self.lhs.Simplify()
     rhs = self.rhs.Simplify()
     if isinstance(lhs, Constant) and isinstance(rhs, Constant):
-      return Constant(ast.literal_eval(lhs + self.operator + rhs))
+      return Constant(ast.literal_eval(lhs.value + self.operator + rhs.value))
 
     if isinstance(self.lhs, Constant):
       if self.operator in ('+', '|') and lhs.value == '0':
@@ -298,7 +298,7 @@ class Operator(Expression):
       if self.operator == '*' and rhs.value == '0':
         return Constant(0)
 
-      if self.operator == '*' and self.rhs.value == '1':
+      if self.operator == '*' and rhs.value == '1':
         return lhs
 
     return Operator(self.operator, lhs, rhs)
@@ -316,9 +316,7 @@ class Operator(Expression):
     if self.Equals(expression):
       return Event(name)
     lhs = self.lhs.Substitute(name, expression)
-    rhs = None
-    if self.rhs:
-      rhs = self.rhs.Substitute(name, expression)
+    rhs = self.rhs.Substitute(name, expression)
     return Operator(self.operator, lhs, rhs)
 
 
@@ -382,7 +380,9 @@ class Function(Expression):
                rhs: Optional[Union[int, float, Expression]] = None):
     self.fn = fn
     self.lhs = _Constify(lhs)
-    self.rhs = _Constify(rhs)
+    self.rhs = None
+    if rhs is not None:
+      self.rhs = _Constify(rhs)
 
   def ToPerfJson(self):
     if self.rhs:
@@ -407,7 +407,8 @@ class Function(Expression):
     return Function(self.fn, lhs, rhs)
 
   def HasExperimentalEvents(self) -> bool:
-    return self.lhs.HasExperimentalEvents() or (self.rhs and self.rhs.HasExperimentalEvents())
+    return (self.lhs.HasExperimentalEvents() or
+            (self.rhs is not None and self.rhs.HasExperimentalEvents()))
 
   def Equals(self, other: Expression) -> bool:
     if isinstance(other, Function):
@@ -683,7 +684,7 @@ class MetricGroup:
 
   def Flatten(self) -> Set[Metric]:
     """Returns a set of all leaf metrics."""
-    result = set()
+    result: Set[Metric] = set()
     for x in self.metric_list:
       result = result.union(x.Flatten())
 
-- 
2.55.0.795.g602f6c329a-goog