Repository navigation
Fall back to plain hardware perf events when extended types are rejected - #2367
Merged
Merged
Conversation
Since "Replicate generic hardware events on all CPU PMUs" (google#2123), generic hardware events are opened on every CPU PMU with the PMU type in the upper bits of attr.config (PERF_PMU_TYPE_SHIFT). That needs the PMU driver to advertise PERF_PMU_CAP_EXTENDED_HW_TYPE, which e.g. arm_pmu in Linux 6.1 does not, so the kernel rejects such events (with EINVAL on an arm64 emulator, ENOENT on a Pixel 9). There was no fallback, so every hardware counter was dropped with "Failed to get a file descriptor for performance counter" and Android benchmarks reported no PMU data. If a hardware event cannot be opened on any CPU PMU with an extended type, open it as a plain hardware event instead, and stop trying extended types for the remaining counters. Failures on extended types are logged at --v=1 rather than as warnings, since the fallback may succeed. On heterogeneous systems without extended type support, the kernel opens the plain event on a single CPU PMU of its choosing, so it only counts while the benchmark runs on that PMU's CPUs (as before google#2123). Also log CPU PMU discovery and perf_event_open results at --v=1: perf_counters: found CPU PMU armv8_pmuv3 type 8 perf_counters: could not open CYCLES (pmu 8, type 0, config 0x800000000): Invalid argument perf_counters: extended hardware event types appear to be unsupported, falling back to plain hardware events perf_counters: opened CYCLES (pmu 0) as fd 3 AI disclosure: this change was written with the help of an AI coding tool.
LebedevRI
reviewed
Nov 5, 2026
| pmus.push_back(0); | ||
| } | ||
| continue; | ||
| } |
Collaborator
There was a problem hiding this comment.
Can this function be refactored somehow to avoid this try_extended_hw_type thing?
This approach looks convoluted.
Collaborator
There was a problem hiding this comment.
I should have clarified. Can this be refactored to avoid doing perf_event_open() just to close it again,
and then immediately reopen it again?
Contributor
Author
There was a problem hiding this comment.
Does the new commit look like what you're expecting?
Collaborator
There was a problem hiding this comment.
I think this is much more reasonable, yes. Thank you.
LebedevRI
reviewed
Nov 5, 2026
LebedevRI
reviewed
Nov 5, 2026
LebedevRI
requested changes
Nov 5, 2026
Instead of falling back to a plain hardware event inside the open loop (which needed the try_extended_hw_type flag, an index-based loop that appends to the PMU list, and restoring group_id), move the code that opens a counter (with retries and the fallback to a new group) into a helper that returns 0 if the counter could be opened, or else the errno of the failed perf_event_open call. Use it to first try opening each generic hardware event on all CPU PMUs with extended types and, if none of these copies can be opened, to open it as a plain hardware event, as before "Replicate generic hardware events on all CPU PMUs" (google#2123). Each event is only opened once, and no state is carried from one counter to the next. Since the helper only updates group_id when a counter is opened, a counter that cannot be opened no longer forces the following counters into a new group. The --v=1 logging of every perf_event_open call is replaced by logging the copies that cannot be opened, e.g.: perf_counters: found CPU PMU armv8_pmuv3 type 8 perf_counters: cannot open CYCLES on PMU type 8: Invalid argument AI disclosure: this change was written with the help of an AI coding tool.
charlesmunger
force-pushed
the
fix_android_pmu
branch
from
November 5, 2026 20:46
fea3016 to
b9d230a
Compare
LebedevRI
reviewed
Nov 5, 2026
Comment on lines
+271
to
+272
| } else { | ||
| opened = true; |
Collaborator
There was a problem hiding this comment.
What is the expected behaviour?
Should the fallback happen if none of GetPMUTypesForEvent() succeeded,
or if at least one failed?
Contributor
Author
There was a problem hiding this comment.
Added clarifying comment
LebedevRI
reviewed
Nov 5, 2026
* Keep GetPMUTypesForEvent() as before, only dropping the default {0}
(non-replicated events now take the plain-event path in the caller).
* Document that the plain event is only opened if the event could not
be opened on any CPU PMU (or is not replicated): also opening it on
some PMUs would count twice on the PMU that the kernel opens the plain
event on, since the counts of all PMUs are summed.
No functional change.
AI disclosure: this change was written with the help of an AI coding tool.
LebedevRI
previously approved these changes
Nov 7, 2026
LebedevRI
left a comment
LebedevRI
left a comment
Collaborator
There was a problem hiding this comment.
Assuming add_counter is a NFC code extraction modulo PERF_PMU_TYPE_SHIFT, LG.
The previous change accidentally reformatted `struct perf_event_attr
attr {};` to `attr{}`, as newer clang-format versions do. The
clang-format-lint check uses clang-format 18, which formats it as
`attr {}`.
AI disclosure: this change was written with the help of an AI coding tool.
Contributor
Author
|
Fixed clang format issue, should be ready for merge now |
LebedevRI
approved these changes
Nov 10, 2026
LebedevRI
left a comment
LebedevRI
left a comment
Collaborator
There was a problem hiding this comment.
As long as there's nothing funny hidden in the diff, seems fine.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Since "Replicate generic hardware events on all CPU PMUs" (#2123), generic hardware events are opened on every CPU PMU with the PMU type in the upper bits of attr.config (PERF_PMU_TYPE_SHIFT). That needs the PMU driver to advertise PERF_PMU_CAP_EXTENDED_HW_TYPE, which e.g. arm_pmu in Linux 6.1 does not, so the kernel rejects such events (with EINVAL on an arm64 emulator, ENOENT on a Pixel 9). There was no fallback, so every hardware counter was dropped with "Failed to get a file descriptor for performance counter" and Android benchmarks reported no PMU data.
If a hardware event cannot be opened on any CPU PMU with an extended type, open it as a plain hardware event instead, and stop trying extended types for the remaining counters. Failures on extended types are logged at --v=1 rather than as warnings, since the fallback may succeed.
On heterogeneous systems without extended type support, the kernel opens the plain event on a single CPU PMU of its choosing, so it only counts while the benchmark runs on that PMU's CPUs (as before #2123).
Also log CPU PMU discovery and perf_event_open results at --v=1:
perf_counters: found CPU PMU armv8_pmuv3 type 8
perf_counters: could not open CYCLES (pmu 8, type 0, config 0x800000000): Invalid argument
perf_counters: extended hardware event types appear to be unsupported, falling back to plain hardware events
perf_counters: opened CYCLES (pmu 0) as fd 3
AI disclosure: this change was written with the help of an AI coding tool.