Re: [PATCH 4/6] perf test attr: Fix legacy event encodings in group tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] perf test attr: test-record-group-sampling fails on bare cycles
- [Medium] perf: test attr fails on hybrid due to PMU type shift in legacy-hardware-config
--

commit 89d023b229e1044502e25f88b4301ecff3715694
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf test attr: Fix legacy event encodings in group tests

Since b1c5efbfd92eb84d ("perf parse-events: Remove hard coded legacy
hardware and cache parsing"), event names are resolved with sysfs/JSON
priority over the legacy encodings, so bare cache-misses and
instructions now become the core PMU sysfs/JSON events. This patch
requests the legacy encodings explicitly with the legacy-hardware-config
term.

> diff --git a/tools/perf/tests/shell/attr/test-record-group-sampling1 b/tools/perf/tests/shell/attr/test-record-group-sampling1
> index 4748ab7bf6843..5be9a1a2a4ff0 100644
> --- a/tools/perf/tests/shell/attr/test-record-group-sampling1
> +++ b/tools/perf/tests/shell/attr/test-record-group-sampling1
> @@ -1,6 +1,6 @@
>  [config]
>  command = record
> -args    = --no-bpf-event -e '{cycles,cache-misses}:S' kill >/dev/null 2>&1
> +args    = --no-bpf-event -e '{cycles,default_core/legacy-hardware-config=3/}:S' kill >/dev/null 2>&1

[Severity: Medium]
Since "cycles" is left as a bare event, wouldn't this still fail the strict
type=0 and config=0 requirements later in this file? 

If "cycles" resolves dynamically via sysfs/JSON to a core PMU event (typically
type=4), attr.py will read a non-zero type and fail the equality check. Should
"cycles" also be updated to "default_core/legacy-hardware-config=0/" to match
these expectations?

>  ret     = 1
>  kernel_since = 6.12

[ ... ]

> diff --git a/tools/perf/tests/shell/attr/test-record-group-sampling2 b/tools/perf/tests/shell/attr/test-record-group-sampling2
> index e0432244a0eb7..c05225167ec34 100644
> --- a/tools/perf/tests/shell/attr/test-record-group-sampling2
> +++ b/tools/perf/tests/shell/attr/test-record-group-sampling2
> @@ -1,6 +1,6 @@
>  [config]
>  command = record
> -args    = --no-bpf-event -c 10000 -e '{cycles,cache-misses}:S' kill >/dev/null 2>&1
> +args    = --no-bpf-event -c 10000 -e '{cycles,default_core/legacy-hardware-config=3/}:S' kill >/dev/null 2>&1

[Severity: Medium]
Similarly, will leaving "cycles" as a bare event here cause a failure against
the strict expectations?

>  ret     = 1
>  kernel_since = 6.12

[ ... ]

> diff --git a/tools/perf/tests/shell/attr/test-record-group1 b/tools/perf/tests/shell/attr/test-record-group1
> index eeb1db392bc9c..0437ea4939e88 100644
> --- a/tools/perf/tests/shell/attr/test-record-group1
> +++ b/tools/perf/tests/shell/attr/test-record-group1
> @@ -1,6 +1,6 @@
>  [config]
>  command = record
> -args    = --no-bpf-event -e '{cycles,instructions}' kill >/dev/null 2>&1
> +args    = --no-bpf-event -e '{cycles,default_core/legacy-hardware-config=1/}' kill >/dev/null 2>&1

[Severity: Medium]
Will this fail on hybrid architectures like Intel Alder Lake?

On hybrid platforms, perf_pmus__supports_extended_type() evaluates to true.
This causes config_term_pmu() in tools/perf/util/parse-events.c to shift the
dynamic PMU type into the upper 32 bits of attr->config when using
legacy-hardware-config:

tools/perf/util/parse-events.c:config_term_pmu() {
	...
	attr->config = term->val.num;
	if (perf_pmus__supports_extended_type())
		attr->config |= (__u64)attr->type << PERF_PMU_TYPE_SHIFT;
	attr->type = PERF_TYPE_HARDWARE;
	...
}

This could yield a config value of (4 << 32) | 1 instead of 1.

Since the test script keeps the hardcoded expected attribute config=1 and
tools/perf/tests/shell/lib/attr.py uses strict string equality, will this
result in an unavoidable test failure?

>  ret     = 1
>  
>  [event-1:base-record]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006161309.3078488-1-acme@kernel.org?part=4