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.

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

perf_event_open cpu should be -1 for valid PID #1464

Merged
merged 1 commit into from May 15, 2024

Conversation

brycekahle
Copy link
Contributor

This will match libbpf behavior. Relevant snippet from perf_event_open manpage

   pid > 0 and cpu == -1
         This measures the specified process/thread on any CPU.

@brycekahle brycekahle requested a review from mmat11 as a code owner May 10, 2024 20:05
Copy link
Collaborator

@mmat11 mmat11 left a comment

Choose a reason for hiding this comment

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

lgtm, this only affected Uprobes with PID>0, right?

@lmb
Copy link
Collaborator

lmb commented May 13, 2024

I'm missing context, sorry. What is currently broken? Can you extend the commit message to include the explanation? And is it possible to add a test?

@brycekahle
Copy link
Contributor Author

Theoretically, if you specify a PID then the arguments given mean it is applying only to CPU 0. In practice, it may not be affecting anything because the perf <-> tracepoint/kprobe linkage has some gaps.

@lmb
Copy link
Collaborator

lmb commented May 14, 2024

Hmm, could this be the cause of #406 ?

@mmat11
Copy link
Collaborator

mmat11 commented May 14, 2024

Hmm, could this be the cause of #406 ?

I believe not, in that test opts is nil so we trace all pids (and the cpu value is correct in that case)

@lmb
Copy link
Collaborator

lmb commented May 14, 2024

So this is a fix for a theoretical issue? Happy to carry the patch without tests as long as the commit message gets a bit of detail why we're doing this.

From perf_event_open manpage attributes documentation:

pid > 0 and cpu == -1
     This measures the specified process/thread on any CPU.

Fixes it so it measures all CPUs when a PID is specified.

Signed-off-by: Bryce Kahle <bryce.kahle@datadoghq.com>
@brycekahle
Copy link
Contributor Author

Updated commit message

@lmb lmb merged commit 6a03aad into cilium:main May 15, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants