Re: [PATCH 2/2] perf jevents: add amd specific device table entry metrics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf jevents: min() in AmdIommudte() and AmdIommuptw() turns NaN into 100%
- [Low] perf jevents: wrong description for lpm_iommu_dte_lookups in AmdIommudte()
--

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

perf jevents: add amd specific device table entry metrics

This commit adds the iommudte metric group (lpm_iommu_dte_miss_rate and
lpm_iommu_dte_lookups) for AMD Zen 2+ 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 dcffd6e268fad..912bceb57d67b 100755
> --- a/tools/perf/pmu-events/amd_metrics.py
> +++ b/tools/perf/pmu-events/amd_metrics.py
> @@ -318,6 +318,37 @@ def AmdIotlb() -> Optional[MetricGroup]:
>      )
>  
>  
> +def AmdIommudte() -> Optional[MetricGroup]:
> +    global _zen_model
> +
> +    if _zen_model < 2:
> +        return None
> +
> +    dte_hit = Event("amd_iommu/mem_dte_hit/")
> +    dte_mis = Event("amd_iommu/mem_dte_mis/")
> +    lookups = dte_hit + dte_mis
> +
> +    return MetricGroup(
> +        "iommudte",
> +        [
> +            Metric(
> +                "lpm_iommu_dte_miss_rate",
> +                "IOMMU device table cache miss rate",
> +                min(d_ratio(dte_mis, lookups), 1),
> +                "100%"
> +            ),

[Severity: Medium]
In AmdIommudte(), does wrapping d_ratio(dte_mis, lookups) in min(..., 1)
cause lpm_iommu_dte_miss_rate to report 100.0% instead of "none" on sockets
where no amd_iommu counters are scheduled?

Because the amd_iommu PMU exposes a cpumask containing only CPU 0, running
perf stat with --per-socket schedules 0 counters on other sockets (such as
S1 in the commit message's test output), causing dte_hit, dte_mis, and
d_ratio(dte_mis, lookups) to evaluate to NAN.

In tools/perf/util/expr.y, MIN evaluates:

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

Since NAN < 1 is false in IEEE 754 floating-point arithmetic, min(NAN, 1)
returns 1.0 (100.0%) instead of propagating NAN, as seen in the commit
message's output for S1:

    {"socket" : "S1", "counters" : 0,
    "lookups/s  lpm_iommu_dte_lookups" : "none",
    "%  lpm_iommu_dte_miss_rate" : "100.0"}

Since lookups is dte_hit + dte_mis, d_ratio(dte_mis, lookups) cannot exceed
1.0 for valid counter readings. Should lpm_iommu_dte_miss_rate in
AmdIommudte() (as well as lpm_iommu_ptw_nested_read_ratio and
lpm_iommu_ptw_guest_read_ratio in AmdIommuptw()) use d_ratio() directly
without min(..., 1), matching AmdIotlb()?

> +            Metric(
> +                "lpm_iommu_dte_lookups",
> +                "IOMMU device table cache miss rate",
> +                d_ratio(lookups, interval_sec),
> +                "lookups/s"
> +            )

[Severity: Low]
In AmdIommudte(), is the description for lpm_iommu_dte_lookups copy-pasted
from lpm_iommu_dte_miss_rate? It currently says "IOMMU device table cache
miss rate" instead of describing IOMMU device table cache lookups.

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