Re: [PATCH 1/2] perf jevents: add iommu page table walk metrics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf jevents: unnecessary global in AmdIommuptw() breaks pylint W0602 build
- [Medium] perf jevents: min() in AmdIommuptw() turns NaN into 100% on uncounted sockets
--

commit 421e4310ee31d5eae1bdf50f6ef3739f62cf56c1
Author: Jacob Lefkowitz <jlefkowi@akamai.com>

perf jevents: add iommu page table walk metrics

This commit adds an iommuptw metric group to tools/perf/pmu-events/amd_metrics.py
for Zen 2 and newer AMD processors using amd_iommu PMU events.

> diff --git a/tools/perf/pmu-events/amd_metrics.py b/tools/perf/pmu-events/amd_metrics.py
> index 42569b9153f45..dcffd6e268fad 100755
> --- a/tools/perf/pmu-events/amd_metrics.py
> +++ b/tools/perf/pmu-events/amd_metrics.py

[ ... ]

> @@ -318,6 +318,52 @@ def AmdIotlb() -> Optional[MetricGroup]:
>      )
>  
>  
> +def AmdIommuptw() -> Optional[MetricGroup]:
> +    global _zen_model
> +
> +    if _zen_model < 2:
> +        return None

[Severity: Medium]
Does declaring global _zen_model in AmdIommuptw() break the perf build when
pylint (>= 2.16.0) is installed on the host?

In tools/perf/Makefile.perf, pylint is enabled by default when present:

    override PYLINT := PYTHONPATH=$(srctree)/tools/perf/python:$(srctree)/tools/perf/pmu-events \
      $(PYLINT) --disable=all --enable=E,W --disable=W0123,W0311,W0511,W0603,W0622,W0718 \
      --ignored-modules=perf,textual,setproctitle

And tools/perf/pmu-events/Build fails the build if pylint returns non-zero:

    $(OUTPUT)%.pylint_log: %
    	$(call rule_mkdir)
    	$(Q)$(call echo-cmd,test)$(PYLINT) "$<" > $@ || (cat $@ && rm $@ && false)

Because AmdIommuptw() only reads _zen_model and never assigns to it, pylint
emits W0602 (global-variable-not-assigned), which is not in the disabled list.
Other functions in tools/perf/pmu-events/amd_metrics.py such as AmdDtlb(),
AmdIotlb(), and AmdItlb() had global _zen_model removed in commit db5a60750133
("perf pmu-events: Clean up mypy and pylint issues") to avoid this warning.
Should global _zen_model be omitted here as well?

> +
> +    walks = Event("amd_iommu/mem_iommu_tlb_pte_mis/") + Event(
> +        "amd_iommu/mem_iommu_tlb_pde_mis/"
> +    )
> +    pt_rd = Event("amd_iommu/page_tbl_read_tot/")
> +    pt_rd_nst = Event("amd_iommu/page_tbl_read_nst/")
> +    pt_rd_gst = Event("amd_iommu/page_tbl_read_gst/")
> +    trans = Event("amd_iommu/mem_trans_total/")
> +
> +    return MetricGroup(
> +        "iommuptw",
> +        [
> +            Metric(
> +                "lpm_iommu_ptw_reads_per_tlb_miss",
> +                "Average IOMMU page table reads per IOTLB miss",
> +                d_ratio(pt_rd, walks),
> +                "reads/tlb_miss"
> +            ),
> +            Metric(
> +                "lpm_iommu_ptw_nested_read_ratio",
> +                "Share of IOMMU page table reads from nested (host) tables", # 0 without nested translation
> +                min(d_ratio(pt_rd_nst, pt_rd), 1),
> +                "100%"
> +            ),
> +            Metric(
> +                "lpm_iommu_ptw_guest_read_ratio",
> +                "Share of IOMMU page table reads from guest tables", # 0 without nested translation
> +                min(d_ratio(pt_rd_gst, pt_rd), 1),
> +                "100%"
> +            ),

[Severity: Medium]
Why do lpm_iommu_ptw_nested_read_ratio and lpm_iommu_ptw_guest_read_ratio both
evaluate to 100.0% on Socket S1 in the commit message example where 0 counters
ran?

The amd_iommu PMU driver registers its cpumask on CPU 0 only, so under
--per-socket aggregation, sockets without CPU 0 have aggr->counts.run == 0.
In tools/perf/util/stat-shadow.c:prepare_metric(), uncounted events are
assigned NAN:

    if (aggr->counts.run == 0) {
    	val = NAN;
    	source_count = 0;
    	aggr_nr = 0;
    }

While d_ratio(pt_rd_nst, pt_rd) and d_ratio(pt_rd_gst, pt_rd) evaluate to NAN,
wrapping them in min(..., 1) hits the MIN rule in tools/perf/util/expr.y:

    | MIN '(' expr ',' expr ')'
    {
    	if (!compute_ids) {
    		$$.val = $3.val < $5.val ? $3.val : $5.val;
    		$$.ids = NULL;
    	} else {
    		$$ = union_expr($3, $5);
    	}
    }

Because NAN < 1.0 is false in IEEE 754 floating-point arithmetic, min(NAN, 1)
returns 1.0 (100.0%) instead of propagating NAN ("none") like the other two
metrics in AmdIommuptw() do. Could min(..., 1) be dropped or replaced with a
construct that preserves NAN when the events were not counted?

Also, on lpm_iommu_ptw_nested_read_ratio, is the comment "# 0 without nested
translation" inverted? Without nested (guest) translation, all IOMMU page
table reads come from the host page tables (page_tbl_read_nst), which matches
the 100.0% value shown on Socket S0 in the commit message.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029-perf-tools-next-v1-0-a75a2b64a15e@akamai.com?part=1