Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

linux_perf_event: exclude_idle only on x86_64 #2295

Merged
merged 2 commits into from
Jul 4, 2024
+14 −13
Merged

linux_perf_event: exclude_idle only on x86_64 #2295

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d4e8ff
linux_perf_event: add make_linux_perf_event
bhalevy Jun 18, 2024
041523e
linux_perf_event: exclude_idle only on x86_64
bhalevy Jun 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions tests/perf/linux_perf_event.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,28 +81,29 @@ linux_perf_event::disable() {
::ioctl(_fd, PERF_EVENT_IOC_DISABLE, 0);
}

linux_perf_event
linux_perf_event::user_instructions_retired() {
static linux_perf_event
make_linux_perf_event(unsigned config, pid_t pid = 0, int cpu = -1, int group_fd = -1, unsigned long flags = 0) {
return linux_perf_event(perf_event_attr{
.type = PERF_TYPE_HARDWARE,
.size = sizeof(struct perf_event_attr),
.config = PERF_COUNT_HW_INSTRUCTIONS,
.config = config,
.disabled = 1,
.exclude_kernel = 1,
.exclude_hv = 1,
#if defined(__x86_64__)
// exclude_idle is not supported on all architectures (e.g. aarch64)
// so enable it selectively only on architectures that support it.
.exclude_idle = 1,
}, 0, -1, -1, 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're replacing an easy-to-read list of settings with a function that takes a large number of anonymous int parameters with lots of defaults, why?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because it's practically common code.
Other than the config field, all other members are the same,
so maintaining 2 open-coded copies require remembering to sync the changes (if needed).

Also, why do you consider specifying pid_t pid = 0, int cpu = -1, int group_fd = -1, unsigned long flags = 0 in the interface as anonymous int parameters while today we pass 0, -1, -1, 0 which is completely cryptic?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right about pid/cpu/group_fd. I was going to say that the commonality is accidental and the parameters can change independently, but something like disabled/exclude_kernel/exclude_hv isn't so likely to change.

#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is it documented? Perhaps we should opt-in where it is supported?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, need some comment, it looks completely obscure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://man7.org/linux/man-pages/man2/perf_event_open.2.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

       EPERM  Returned on many (but not all) architectures when an
              unsupported exclude_hv, exclude_idle, exclude_user, or
              exclude_kernel setting is specified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we've lived without it until 93f19d3 we can opt-in and include it only on x86_64 where we know it works (and add a comment about that)

}, pid, cpu, group_fd, flags);
}

linux_perf_event
linux_perf_event::user_instructions_retired() {
return make_linux_perf_event(PERF_COUNT_HW_INSTRUCTIONS);
}

linux_perf_event
linux_perf_event::user_cpu_cycles_retired() {
return linux_perf_event(perf_event_attr{
.type = PERF_TYPE_HARDWARE,
.size = sizeof(struct perf_event_attr),
.config = PERF_COUNT_HW_CPU_CYCLES,
.disabled = 1,
.exclude_kernel = 1,
.exclude_hv = 1,
.exclude_idle = 1,
}, 0, -1, -1, 0);
return make_linux_perf_event(PERF_COUNT_HW_CPU_CYCLES);
}